Tokens: add CLASS_MODIFIERS and PROPERTY_MODIFIERS constants

[jrchamp]

Description

Several core sniffs and many custom sniffs rely heavily on the Tokens class to simplify code, particularly as new features (like readonly) are added to PHP. This change adds class and property modifier constants and implements those constants in sniffs that were already hardcoding those collections of values. Semantically, these constants also do a better job of communicating what is being searched for. In one or two cases, I decided to continue including SCOPE_MODIFIERS even though all of the tokens are currently part of the PROPERTY_MODIFIERS constant. I do not know what the future holds, so I do not want to assume that will not change in the future.

There should not be any changes in behavior. PSR2.Classes.PropertyDeclaration was not including T_STATIC and it's unclear to me if that was intentional or not.

Suggested changelog entry

Tokens: add CLASS_MODIFIERS and PROPERTY_MODIFIERS constants

Related issues/external references

May help fix moodlehq/moodle-cs#213

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.
• I have opened a sister-PR in the documentation repository to update the Wiki.

[jrfnl]

@jrchamp Thanks for this PR.

Before I even look at this - why should this functionality be in PHP_CodeSniffer itself ?

Note: this functionality is already available - and has been for years - via the PHPCSUtils tokens Collections class (which also includes an OO constant modifier keywords collection):

• https://phpcsutils.com/phpdoc/classes/PHPCSUtils-Tokens-Collections.html#method_classModifierKeywords
• https://phpcsutils.com/phpdoc/classes/PHPCSUtils-Tokens-Collections.html#method_propertyModifierKeywords
• https://phpcsutils.com/phpdoc/classes/PHPCSUtils-Tokens-Collections.html#method_constantModifierKeywords

[jrfnl]

There should not be any changes in behavior. PSR2.Classes.PropertyDeclaration was not including T_STATIC and it's unclear to me if that was intentional or not.

Nor me. I'd recommend doing a backtrace using git blame to see if the historic reason can be found and if there is none, to fix that in a separate PR anyway.

[jrchamp]

Before I even look at this - why should this functionality be in PHP_CodeSniffer itself ?

Note: this functionality is already available - and has been for years - via the PHPCSUtils tokens Collections class (which also includes an OO constant modifier keywords collection):

I think that's exactly the reason.

1. The Tokens class already exists in PHP_CodeSniffer because the core tool and the Sniffs it contains needed some way to avoid unmaintainable repetition which led to the original arrays which you recently converted to constants.
2. Within PHPCSUtils, I believe that you saw shortcomings in the existing PHP_CodeSniffer Tokens collections and created your own. In that sense, not only were these collections needed, but an enhanced version of them was needed because the core ones were insufficient.
3. Even if all Standards and tests were removed from the PHP_CodeSniffer tool, there would be more than 80 uses of the token constants in the Tokenizers and Files directories. Put simply, the core tool needs token constant collections as part of core.
4. External/Custom Standards and Sniffs need reliable token collections to maximize their maintainability, especially as changes occur within PHP. Should they choose the Tokens in core or the Collections in PHPCSUtils?

Thus, if we can accept that the core tool needs token constants itself, then the core tool should either provide the token constants or have a hard dependency on a library that provides the token constants. This provides reliability and consistency for Standards/Sniffs that use the tool. I feel that having competing token collection implementations is a problem, particularly when they are both maintained by the same person. I believe in you, I trust you, so I guess what I want is for you to pick a winner. That way the other option can be deprecated and people can migrate over time.

With v4, there was already a breaking change from static variables to class constants. I don't know how many people have already upgraded, but sooner may be better than later if more changes will be needed. For PHPCSUtils, it's still using static variables and I'm not sure how many projects have that as a dependency. So if the PHPCSUtils collections are better, but PHP_CodeSniffer is the core tool, then I would lean towards moving the better collections into the core tool and providing temporary backwards compatibility for people who are using the constants and static variables that exist today (which mirrors what you've already done in the core tool).

I hope that is helpful - I somewhat feel like I'm saying the things that you already know. You're a hero and deserve so much credit for the work that you've done. I just hope that I haven't wasted your time. ❤️

[jrfnl]

@jrchamp I appreciate your thoughts on this.

PHPCSUtils was born out of a historical necessity (PHPCS itself not merging fast enough, development being stalled, resistance to adding more utility functionality to the Core) and we cannot ignore that it exists now, especially as both projects now live in the same organisation.

Duplicating existing functionality from Utils in PHPCS itself will increase the maintenance burden, which, with PHPCS in its current state, is already too high.
Also keep in mind that PHPCSUtils would also need to provide a BC-layer (the BackCompat functionality), making this a three-fold duplication, with all the mental overhead for contributors who don't understand the reasons for such three-fold duplication.

In your response you already touched upon a direction I'm exporing for the future:

have a hard dependency on a library that provides the token constants

So, I suggest looking at PHPCS + PHPCSUtils as a tandem tooling which - for now - are still distributed separately.
However, I imagine that, at some point in the future, PHPCSUtils may well become a dependency for PHPCS, especially as it provides solutions for problems PHPCS itself has not been able to solve (like short arrays vs short lists).
Not committing to anything yet, but it's something to consider for the future.

[jrfnl]

@jrchamp I have opened a new issue about exploring the option to have closer ties between PHPCS and PHPCSUtils: #1364

In light of this, are you okay with using PHPCSUtils for these Token collections for now ? If so, I believe this PR should be closed until there is more clarity on what direction things will go in per the discussion to be had in #1364.

[jrchamp]

Yes, thank you! Closing as discussed. ❤️