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.

Sign up for GitHub

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 #3870

Closed
wants to merge 1 commit into from
Closed

Tokenizer::$eolChar: change default value #3870

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Tokenizers/Tokenizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ abstract class Tokenizer
*
* @var string
*/
protected $eolChar = [];
protected $eolChar = '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jrfnl jrfnl Aug 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. This all makes sense. 👍


/**
* A token-based representation of the content.
Expand Down