Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tokenizer::$eolChar: change default value #81

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Nov 11, 2023

Description

Recreation of upstream PR squizlabs/PHP_CodeSniffer#3870:

Tokenizer::$eolChar: change default value to be a more logical one. The property is supposed to be a string, so having an array as a default value is confusing.


Additional context from the discussion on the ticket:

@fredden asked:

Should the default value be non-empty? What are your thoughts on using PHP_EOL here instead of an empty string. Or perhaps null or unset.

@jrfnl answered:

I did consider those options, but those were all rejected out of hand.

  • The property is documented as a string, so the default should also be a string.
  • Setting the default to null may lead to PHP 8.1 "passing null to non-nullable" deprecations in unexpected places.
  • Setting the default to PHP_EOL is plain wrong as that depends on the OS on which PHPCS is run, while the property is about the EOL character of the file under scan.

Setting it to an empty string prevents any problems with a wrong value being used and still complies with the documented property type.

@jrfnl added:

In any case, the property is the first thing which will be set once the class is instantiated via the contructor, so the default value isn't even that interesting, though it should definitely not be one of an incorrect type.

Suggested changelog entry

N/A

... to be a more logical one. The property is supposed to be a `string`, so having an array as a default value is confusing.
@jrfnl jrfnl force-pushed the feature/tokenizer-fix-default-property-value branch from 7009f61 to 7ed1591 Compare December 4, 2023 16:19
@jrfnl jrfnl merged commit 3f769b9 into master Dec 4, 2023
65 checks passed
@jrfnl jrfnl deleted the feature/tokenizer-fix-default-property-value branch December 4, 2023 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant