PHP 8.4 | Add tokenization of asymmetric visibility

[DanielEScherzer]

Description

Set up tokenization of asymmetric visibility modifiers:

• Add tokens T_PUBLIC_SET, T_PROTECTED_SET, and T_PRIVATE_SET if not already defined
• Consider them scope modifiers (PHP_CodeSniffer\Util\Tokens::$scopeModifiers)
• Mark them as context sensitive (PHP_CodeSniffer\Util\Tokens::$contextSensitiveKeywords)
• Store their known lengths (PHP_CodeSniffer\Tokenizers\PHP::$knownLengths)
• Backfill tokenization for older versions of PHP

Suggested changelog entry

PHP 8.4 | Support tokenization of asymmetric visibility modifiers

Related issues/external references

Related to #851

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] 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

• [x] I have checked there is no other PR open for the same change.
• [x] I have read the Contribution Guidelines.
• [x] 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).
• [x] I have added tests to cover my changes.
• [x] I have verified that the code complies with the projects coding standards.
• [ ] [Required for new sniffs] I have added XML documentation for the sniff.

[DanielEScherzer]

One additional question: how did you determine where in the (huge) PHP::tokenize() method, the polyfill should go ? It is currently between the tokenization blocks for T_ENUM_CASE and PHP 8.0 namespaced names handling. Is there a particular reason for that ?

No idea, sorry

[DanielEScherzer]

@jrfnl my understanding is that this is waiting for re-review, just want to check that you aren't waiting for me to do something and have us both just wait

[jrfnl]

@jrfnl my understanding is that this is waiting for re-review, just want to check that you aren't waiting for me to do something and have us both just wait

Sorry, yes, been busy with some other things. Let me have a look now.

[DanielEScherzer]

@DanielEScherzer Thanks for the updates you've made.

While looking at this PR again - and in particularly, looking at the changes made to the predefined Tokens::$* properties, I realized more changes are needed. In particular related to the correct handling by the tokenizer of |, & and parenthesis in union, intersection and DNF types. See #907 as a loosely related example. And while with the current changes, I believe the tokenization of ? in nullable types in combination with asym visibility will be handled correctly, this will need to be safeguarded by tests. See #908 for another loosely related example.

Done for matching 907, after 908 is merged I'll add test cases there

I've also looked at the tests again. I would prefer for the "valid" asym visibility tests and the "invalid" tests to be split into two test methods. I believe the testAsymmetricVisibility() method should, in that case, not need the $testContent parameter.

I split them, and kept $testContent to assert that the token content is correct

class Foo {
    function protected() {}
    function public(Set $setter) {}
}

Done

[jrfnl]

Done for matching 907, after 908 is merged I'll add test cases there

@DanielEScherzer I've merged both #907 as well as #908 now, so you should be able to make that update now.

[DanielEScherzer]

Failure is from HTTP 405 error when uploading code coverage, which I think is unrelated to the actual changes

[jrfnl]

Failure is from HTTP 405 error when uploading code coverage, which I think is unrelated to the actual changes

💯 Coveralls is doing some disruptive maintenance: https://status.coveralls.io/ (also the reason my own recent PRs are all marked as failing). I will retrigger the relevant builds once the maintenance has finished.

[DanielEScherzer]

Done for matching 907, after 908 is merged I'll add test cases there

@DanielEScherzer I've merged both #907 as well as #908 now, so you should be able to make that update now.

This was done, but I can't remove the Status: awaiting feedback tag myself

[jrfnl]

Thanks @DanielEScherzer. I'm hoping to take another look over the next few days, but what with the PRs for 4.0 being lined up (see #924), I may delay this PR until after next week (though it would still go into 3.x). Please bear with me. It is on my radar though.

[jrfnl]

P.S.: I see there is another merge conflict. Sorry about that. PR #948 also touches that same sniff, so you may want to hold off resolving the conflict until that PR has been merged.

[DanielEScherzer]

P.S.: I see there is another merge conflict. Sorry about that. PR #948 also touches that same sniff, so you may want to hold off resolving the conflict until that PR has been merged.

Done the holding off, and done the subsequent resolving of the conflict