PHP 8.4 | Report final properties in File::getMemberProperties()

[DanielEScherzer]

Description

Update File::getMemberProperties() to report when a property is marked as final. Update existing tests (which are all for non-final properties) to expect that the property not be marked as final, and add a new test case for a final that should be, and is, marked as final.

Suggested changelog entry

PHP 8.4 | Report final properties in File::getMemberProperties()

Related issues/external references

Related to #734

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.

[DanielEScherzer]

From #734

• Property hooks - final properties
  • Tokenizer changes needed ?
  • Updates needed to utility functions - getMemberProperties(), getMethodParameters() (for constructor property promotion)
  • Sniff updates needed

It turns out we don't actually need to update getMethodParameters(), because final properties don't seem to work for constructor promotion, I just filed php/php-src#17860

[jrfnl]

@DanielEScherzer Thank you for working on this and for your willingness to contribute to PHP_CodeSniffer.

Code-wise this PR is looking good. ✅

If you don't mind, I'd like to see a few more tests added though.
While I'm pretty certain, based on the code, that things will work correctly, I'd still like to safeguard that for the future.
I'm thinking along the lines of:

• final property without other modifiers or type
• final property without other modifiers, but with type declaration
• property with multiple modifiers with some variations in the order of the keywords (final as first, middle, last keyword)

Other than that I've left two nitpick remarks inline and CI is pointing to a tiny CS fix which is needed.

getMethodParameters()

It turns out we don't actually need to update getMethodParameters(), because final properties don't seem to work for constructor promotion

Nice find. I'd be curious to see some more stress testing of constructor property promotion:

• Does it also not work when there is a hook attached ?

As for PHPCS, I'm in two minds about the consequences of this for the getMethodParameters() method though.

• One the one hand: agreed - let's not support it in that method until PHP itself does. I.e. let's not handle it.
• On the other hand: PHPCS is generally speaking parse error/live coding tolerant, so things shouldn't break on coding mistakes, so even though getMethodParameters() may not need to support the final keyword in method declarations (yet), we may still want to be tolerant to final keywords being used in constructor property promotion, i.e. we may want to silently ignore it.
  And if the method would already do so, maybe we should just add a test for the method to document that behaviour.

Having said that, let's move this discussion to #734/another PR, as it shouldn't block this PR and doesn't have to be solved at the same time.

Follow up actions

I've updated #734 to mention this PR and added your name to the second task for final properties.

Please let me know if you would also like to take on the third task for adding support for final properties (sniff updates), in which case, I'll add your name to it to "claim" the task (and prevent multiple people working on the same thing). I can already think of a number of sniffs which will need updates.

I'd also like to invite you to make the same changes as made in this PR for PHPCSUtils (BCFile::getMemberProperties() and ObjectDeclarations::getMemberProperties() Variables::getMemberProperties()). Feel free to say no, but I just wanted to mention it in case you're interested.

[jrfnl]

For reference in case anyone is looking for it - these changes are based on the "Final Hooks" section in this PHP 8.4 RFC: https://wiki.php.net/rfc/property-hooks#interfaces

[DanielEScherzer]

@DanielEScherzer Thank you for working on this and for your willingness to contribute to PHP_CodeSniffer.

Happy to help - I've been using codesniffer for years, and I figure instead of just asking when this would be done I could try and help out

If you don't mind, I'd like to see a few more tests added though. While I'm pretty certain, based on the code, that things will work correctly, I'd still like to safeguard that for the future. I'm thinking along the lines of:

• final property without other modifiers or type
• final property without other modifiers, but with type declaration
• property with multiple modifiers with some variations in the order of the keywords (final as first, middle, last keyword)

done

getMethodParameters()

It turns out we don't actually need to update getMethodParameters(), because final properties don't seem to work for constructor promotion

Nice find. I'd be curious to see some more stress testing of constructor property promotion:

• Does it also not work when there is a hook attached ?

Promoted hooks work fine from my testing

As for PHPCS, I'm in two minds about the consequences of this for the getMethodParameters() method though.

...

I figured I would start small, so I didn't do getMethodParameters() at first, but I'll go work on php/php-src#17860 next, and once that merges I'm happy to write an update to getMethodParameters() too, but figured that could be its own patch.

Follow up actions

I've updated #734 to mention this PR and added your name to the second task for final properties.

Please let me know if you would also like to take on the third task for adding support for final properties (sniff updates), in which case, I'll add your name to it to "claim" the task (and prevent multiple people working on the same thing). I can already think of a number of sniffs which will need updates.

If you can list the sniffs, I can take a look

I'd also like to invite you to make the same changes as made in this PR for PHPCSUtils (BCFile::getMemberProperties() and ObjectDeclarations::getMemberProperties()). Feel free to say no, but I just wanted to mention it in case you're interested.

I can take a look, I don't think I had ever seen those utilities before

[jrfnl]

@DanielEScherzer

If you don't mind, I'd like to see a few more tests added though.

done

Thank you for that!

getMethodParameters()

• Does it also not work when there is a hook attached ?

Promoted hooks work fine from my testing

I meant the combination of constructor property promotion with a final property with a hook attached, but I just tested it myself and it looks like that also doesn't work: https://3v4l.org/fvOaG

As for PHPCS, I'm in two minds about the consequences of this for the getMethodParameters() method though.

I figured I would start small, so I didn't do getMethodParameters() at first, but I'll go work on php/php-src#17860 next, and once that merges I'm happy to write an update to getMethodParameters() too, but figured that could be its own patch.

Sounds like a plan and yes, I agree, a separate patch is best.

I have a suspicion that #17860 may be rejected (or rather: was not implemented) for technical reasons. I also wouldn't surprised if changing it would need an RFC.
I'm following the ticket to see the responses.

Follow up actions

Please let me know if you would also like to take on the third task for adding support for final properties (sniff updates), in which case, I'll add your name to it to "claim" the task. I can already think of a number of sniffs which will need updates.

If you can list the sniffs, I can take a look

I haven't got a list, but the following sniffs come to mind, though I'd recommend doing a code base wide search on the use of getMemberProperties(), Tokens::$scopeModifiers/Tokens::$methodPrefixes (yeah, I know, methods, but I've seen property related sniffs used that collection too), T_VARIABLE (particularly in register() or constructor methods), T_READONLY, T_FINAL (some sniffs may also need adjusting for the final keyword now being allowed in a different context than before).

These are some sniffs which come to mind straight away - they may need updates, and even if the code doesn't need to change, they are likely to need tests safeguarding that they handle the new syntax correctly anyway:

• PSR2.Classes.PropertyDeclaration
• Squiz.Commenting.VariableComment
• Squiz.Scope.MemberVarScope
• Squiz.WhiteSpace.MemberVarSpacing
• Squiz.WhiteSpace.ScopeKeywordSpacing

I'd also like to invite you to make the same changes as made in this PR for PHPCSUtils (BCFile::getMemberProperties() and ObjectDeclarations::getMemberProperties()). Feel free to say no, but I just wanted to mention it in case you're interested.

I can take a look, I don't think I had ever seen those utilities before

PHPCSUtils is a utility layer for PHPCS and is used, amongst others by the PHPCSExtra and PHPCompatibility standards for higher accuracy and faster results.
Considering you are using PHPCSExtra in common-phpcs, you are already using it ;-)

[DanielEScherzer]

I have a suspicion that #17860 may be rejected (or rather: was not implemented) for technical reasons. I also wouldn't surprised if changing it would need an RFC.

What technical reasons? It didn't take me too long to implement, and since PHP allows promoting hooks, why not final properties? But we can continue the discussion there (or on php/php-src#17861 with my patch)

I haven't got a list, but the following sniffs come to mind,...

I'll take a look, but not today - I was only planning to read through #734 and see if there was anything I might understand enough to contribute, but then I got an itch to code and sent this patch, and then I discovered the php-src bug and sent a patch for that, and I didn't really have the time today for any of that :)

[jrfnl]

I have a suspicion that #17860 may be rejected (or rather: was not implemented) for technical reasons. I also wouldn't surprised if changing it would need an RFC.

What technical reasons? It didn't take me too long to implement, and since PHP allows promoting hooks, why not final properties? But we can continue the discussion there (or on php/php-src#17861 with my patch)

I honestly don't know, but quite often these type of things are left out for technical reasons, as in: there is something in the engine which make it complicated or a suspicion that it might block a future iteration. Glad to hear you got it working easily though. I'm following the ticket.

[jrfnl]

@DanielEScherzer Thank you for your work on this. I'll merge it once the build has passed.

[DanielEScherzer]

@DanielEScherzer Thank you for your work on this. I'll merge it once the build has passed.

Thanks - turns out the build didn't past, I think I fixed it now - I've been having trouble running the test suite on my computer, so that is why I didn't catch it.

Do you want me to squash this, or can you do it when merging?

[jrfnl]

Thanks - turns out the build didn't past, I think I fixed it now - I've been having trouble running the test suite on my computer, so that is why I didn't catch it.

@DanielEScherzer Thanks for fixing that up. Out of curiousity - what issue are you running into when running the tests locally ? It would help to understand this, to see if there are any potential improvements to be made to the contributing guide and/or Composer scripts.

Do you want me to squash this, or can you do it when merging?

Thanks for asking, I'll do a squash-merge, so all good.

[DanielEScherzer]

Thanks - turns out the build didn't past, I think I fixed it now - I've been having trouble running the test suite on my computer, so that is why I didn't catch it.

@DanielEScherzer Thanks for fixing that up. Out of curiousity - what issue are you running into when running the tests locally ? It would help to understand this, to see if there are any potential improvements to be made to the contributing guide and/or Composer scripts.

Errors about classes being redeclared, but in the same place - for both NullableTypeDeclarationSniff and ReturnTypeDeclarationSniff locally I have the class declaration wrapped in if ( !class_exists( ... ). There might have been something else that isn't clear in my git status though

[jrfnl]

Thanks - turns out the build didn't past, I think I fixed it now - I've been having trouble running the test suite on my computer, so that is why I didn't catch it.

@DanielEScherzer Thanks for fixing that up. Out of curiousity - what issue are you running into when running the tests locally ? It would help to understand this, to see if there are any potential improvements to be made to the contributing guide and/or Composer scripts.

Errors about classes being redeclared, but in the same place - for both NullableTypeDeclarationSniff and ReturnTypeDeclarationSniff locally I have the class declaration wrapped in if ( !class_exists( ... ). There might have been something else that isn't clear in my git status though

Hmm... curious.... I'll be happy to jump on a call with you to try and figure out what's going on there as this is a new one for me, but let's move that out of this ticket. Feel free to ping me on Mastodon or X (@jrf_nl on both) to get a call sorted.