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.

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

PHP 8.1 | PSR2/PropertyDeclaration: add check for visibility before readonly keyword #3637

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Jul 23, 2022

The PSR2.Classes.PropertyDeclaration sniff includes a check to verify that the static keyword is after the visibility in a property declaration.

PHP 8.1 introduced the readonly keyword for properties, but as PSR2/PSR12 were both released before PHP 8.1, no PSR rules have (yet) been defined for the modifier keyword order for readonly properties.

++PSR PER 2.0.0 dictates that visibility is declared before the readonly keyword.++

Having said that, all code samples in both the RFC as well as the PHP manual use the visibility - readonly modifier keyword order, so it is likely that this will become the standard modifier keyword order.

A search for PHP projects which have started to use the readonly keyword, also shows these predominantly use the visibility - readonly modifier keyword order.

With that in mind, I'm proposing to add a check for this order to the PSR2.Classes.PropertyDeclaration sniff - this being the only PHPCS native sniff which checks the modifier keyword order for properties.

As PSR2/PSR12 do not formally have rules for the order (yet), the new error code being introduced is excluded for those rulesets (for the time being).

++As this sniff is included in PSR2 and PSR12, the new check will automatically apply the PSR-PER property modifier keyword order rules to PSR2/PSR12.++

Includes unit tests.

Ref:

@gsherwood gsherwood added this to the 3.8.0 milestone Sep 15, 2022
@jrfnl jrfnl mentioned this pull request Oct 21, 2022
@jrfnl jrfnl force-pushed the feature/psr2-propertydeclaration-add-visibility-readonly-modifier-order-check branch from a6de515 to 1f140ab Compare March 2, 2023 04:14
@jrfnl
Copy link
Contributor Author

jrfnl commented May 4, 2023

@gsherwood and me discussed this. This needs checking against PSR-PER.

…eadonly keyword

The `PSR2.Classes.PropertyDeclaration` sniff includes a check to verify that the `static` keyword is _after_ the visibility in a property declaration.

PHP 8.1 introduced the `readonly` keyword for properties.

[PSR PER 2.0.0](https://www.php-fig.org/per/coding-style/#46-modifier-keywords) dictates that visibility is declared before the `readonly` keyword.

All code samples in both the RFC as well as the PHP manual also use the `visibility - readonly` modifier keyword order, so it is likely that this will become the standard modifier keyword order.

A search for PHP projects which have started to use the `readonly` keyword, also shows these predominantly use the `visibility - readonly` modifier keyword order.

With that in mind, I'm proposing to add a check for this order to the `PSR2.Classes.PropertyDeclaration` sniff - this being the only PHPCS native sniff which checks the modifier keyword order for properties.

As this sniff is included in PSR2 and PSR12, the new check will automatically apply the PSR-PER property modifier keyword order rules to PSR2/PSR12.

Includes unit tests.

Ref:
* https://wiki.php.net/rfc/readonly_properties_v2
* https://www.php.net/manual/en/language.oop5.properties.php#language.oop5.properties.readonly-properties
* https://sourcegraph.com/search?q=context:global+%5Cs%28public%7Cprotected%7Cprivate%29%5Cs%2Breadonly%5Cs%2B+lang:php+-file:%28%5E%7C/%29%28vendor%7Ctests%3F%29/+fork:no+archived:no&patternType=regexp (searching `visibility - readonly` order - > 500 results)
* https://sourcegraph.com/search?q=context:global+%5Csreadonly%5Cs%2B%28public%7Cprotected%7Cprivate%29%5Cs%2B+lang:php+-file:%28%5E%7C/%29%28vendor%7Ctests%3F%29/+fork:no+archived:no&patternType=regexp (searching `readonly - visibility` order - 41 (non false positive) results)
@jrfnl jrfnl force-pushed the feature/psr2-propertydeclaration-add-visibility-readonly-modifier-order-check branch from 1f140ab to 8cdc122 Compare May 4, 2023 21:06
@jrfnl
Copy link
Contributor Author

jrfnl commented May 4, 2023

I've verified the current implementation against PSR-PER and the change proposed in this PR is 100% in line with PSR-PER.

4.6 Modifier Keywords

Classes, properties, and methods have numerous keyword modifiers that alter how the engine and language handles them. When present, they MUST be in the following order:

  • Inheritance modifier: abstract or final
  • Visibility modifier: public, protected, or private
  • Scope modifier: static
  • Mutation modifier: readonly
  • Type declaration
  • Name

Ref: https://www.php-fig.org/per/coding-style/#46-modifier-keywords

At this time (PHP 8.2), inheritance modifiers cannot be applied to properties and the static and readonly modifiers are mutually exclusive and cannot be used together.

Based on that, the implementation of the modifier keyword order checks in this PR are sufficient (for now).

Having checked the above, I have now rebased the PR, added a comment to the sniff above the modifier keyword order checks to summarize the above findings and I have removed the exclusions for these new checks from the PSR2 and PSR12 rulesets.

I will also update the PR description to match these findings.

@jrfnl
Copy link
Contributor Author

jrfnl commented May 4, 2023

Side-note: the example code in PSR-PER for this section contains parse errors/invalid code. I have opened a PR to PSR-PER to fix this: php-fig/per-coding-style#73

gsherwood added a commit that referenced this pull request May 7, 2023
@gsherwood gsherwood merged commit 8cdc122 into squizlabs:master May 7, 2023
@jrfnl jrfnl deleted the feature/psr2-propertydeclaration-add-visibility-readonly-modifier-order-check branch May 7, 2023 04:48
@jrfnl
Copy link
Contributor Author

jrfnl commented Dec 8, 2023

FYI: this fix is included in today's PHP_CodeSniffer 3.8.0 release.

As per #3932, development on PHP_CodeSniffer will continue in the PHPCSStandards/PHP_CodeSniffer repository. If you want to stay informed, you may want to start "watching" that repo (or watching releases from that repo).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants