Define polyfilled T_* constants from Tokenizer as int #1292
Conversation
|
@jrfnl the 'validate' step for "Find use of Tokens properties" check is failing on this code. Would you like me to rewrite the check or the code to avoid this? |
What gets flagged is this: I think updating the check to exclude this particular use would be most appropriate, though I'm not sure how straight-forward that would be. |
fredden
force-pushed
the
issue-1286/polyfill-tokenizer-const-as-int
branch
from
e862ab2 to
9e01bd1
Compare
The value of these constants are not stable, and therefore already cannot be relied upon. This is because the specific values that PHP assigns can change with different versions of PHP. PHPCS does not use the values of these constants (other than to look up their name using the Tokens::tokenName() method). There are other tools which also polyfill these constants. Some of those tools also perform validation on the value for these constants. In order to play nicely with the arbitrary validation that other tools perform on these constants, we are switching from string values to integer values. All PHPCS 'native' tokens currently have reliable values. In line with PHP T_* constants, the values of these tokens should never be relied upon. In a future version of PHPCS, the values for these tokens will switch from strings to integers. Existing tests already cover the use of these constants and do not require adjustment for the code being changed here.
fredden
force-pushed
the
issue-1286/polyfill-tokenizer-const-as-int
branch
from
9e01bd1 to
f9e71ea
Compare
|
I have worked out a way to use a static method after all. This means we don't need to change the CI/CD check for external use of deprecated properties. |
jrfnl
left a comment
jrfnl
left a comment
There was a problem hiding this comment.
Hi @fredden, thanks for that update and sorry for my slow response.
Creative solution and I appreciate the inline comments about the timing aspect of the static method and the fragility of the setup. That should hopefully prevent breakage when this method is touched in the future.
The fragility is a little concerning, but if it works for normal PHPCS CLI use and the PHPCS native test setup (which it does), I'm okay with it (though I wonder which user of an external dependency will complain first about something breaking because of this change...)
Conceptually, I have two questions:
-
The assignment to
$polyfillMappingTable[constant($tokenName)]does not contain any protection against overwriting an existing array index.
While token constants should be unique, if another external tool does this incorrectly and that code would run before the PHPCS polyfill code, it will cause us problems: https://3v4l.org/OCE9X#veol (see the output for PHP 7.2 - 8.0)
We may need to throw an Exception and just block the run if this happens. What do you think ? -
Should the method include protection against being called twice ? (as it is a
public staticmethod defining crucial information) ?
I've not been able to come up with a scenario in which this becomes problematic, but it still feels risky.
Also see https://3v4l.org/bdlTc - when called the second time, each polyfilled token would overwrite its previously created own entry in theTokens::$polyfillMappingTable.
Nitpicky things:
polyfillTokenizerConstants()- there is the PHPTokenizerand the PHPCSTokenizer, purely based on the method name, this could confuse people.
polyfillPHPNativeTokenizerConstants()may be a bit wordy, but might be clearer ?
You might also want to update the method doc block ? (add "PHP native" to the summary, remove "PHP native" from the@internalnote)
| // any class constants or properties before this method has fully run, | ||
| // PHP will intitialise the class, leading to warnings about undefined | ||
| // T_* constants. | ||
| $tokensToPolyfill = [ |
There was a problem hiding this comment.
I can see that these are ordered alphabetically.
To me as a maintainer, it makes more sense to order them by the PHP version which introduced the token (with comments between the sets annotating the PHP version).
With the current ordering, this would mean that every time there is a PHP version drop, someone would need to go through the list to figure out in which PHP version each token was introduced, while if the list is ordered by PHP version, handling a version drop is more straight forward.
| /** | ||
| * Mapping table for polyfilled constants | ||
| * | ||
| * @var array<int, string> |
There was a problem hiding this comment.
| * @var array<int, string> | |
| * @var array<int, string> |
No need to change anything, but just pointing out that we can't actually be sure/guarantee that the key will always be an int as - just like PHPCS did - other tooling could have polyfilled the tokens with some other type of value.
2 participants
Description
The value of these constants are not stable, and therefore already cannot be relied upon. This is because the specific values that PHP assigns can change with different versions of PHP. PHPCS does not use the values of these constants (other than to look up their name using the Tokens::tokenName() method).
There are other tools which also poly-fill these constants. Some of those tools also perform validation on the value for these constants. In order to play nicely with the arbitrary validation that other tools perform on these constants, we are switching from string values to integer values.
The PHP manual suggests "using big numbers like 10000" for poly-filled T_* constants. We have arbitrarily chosen to start our numbering scheme from 135_000.
All PHPCS 'native' tokens currently have reliable values. In line with PHP T_* constants, the values of these tokens should never be relied upon. In a future version of PHPCS, the values for these tokens will switch from strings to integers.
Existing tests already cover the use of these constants and do not require adjustment for the code being changed here.
Suggested changelog entry
Related issues/external references
Fixes #1286
Types of changes
PR checklist