PHP 8.2 | Ruleset: prevent notices about dynamic properties being set #3629

jrfnl · 2022-07-09T03:11:11Z

⚠️ This PR contains two - albeit small - BC-breaks! ⚠️

The BC-breaks can be broken down as follows:

For end-users

If a property is set via a ruleset for an individual sniff and that sniff does not have the property explicitly declared, nor does the sniff declare the PHP magic __set() method or extend stdClass, the ruleset will now be regarded as invalid.

In practice, this means the PHPCS run will halt and will show an informative notice to the end-user about the invalid property setting and the PHPCS run will exit with exit code 3.

For properties being set for a complete category of sniffs or a complete standard, the PHPCS run will not halt, nor show an error when the property isn't supported for all sniffs in the category/standard.
In that case, the property will only be set on those sniffs which support it (i.e. either have the property explicitly declared on the sniff, or have the magic __set() method declared on the sniff, or extend stdClass) and will be silently ignored for all other sniffs.

👉 Aside from possibly surfacing some incidental typos in property settings in rulesets, I expect this change to go mostly unnoticed by end-users.

For developers of PHPCS standards/integrations

The format of the $ruleset['sniff code']['properties']['property name'] sub-array has changed from a array|string type property value to a sub-array containing two entries:
1. 'value' - containing the array|string type property value.
2. 'scope' - containing a string, either sniff or standard, to indicate whether the ruleset property directive was for an individual sniff or for a whole standard/category of sniffs.
The third parameter for the Ruleset::setSniffProperty() method has been changed to expect the above mentioned array of $settings instead of just a property value.

This change is necessary to allow for throwing the error notice when an invalid property is being set on an invalid sniff versus silently ignoring the invalid property if the ruleset specified it on a standard/category.

👉 The impact of this BC-break is expected to be small, as it would require for an external standard or integration of PHPCS to:

extend the Ruleset class and overload the setSniffProperty() method to have any impact;
or to call the setSniffProperty() method on the Ruleset object directly (which may happen in custom test frameworks for external standards).

Note:

The change to the processRule() method cannot be overloaded as it is a private method.
The change in the populateTokenListeners() method is only cosmetic and has no functional impact.

Technical choices made:

For consistent handling of properties set in a (custom) ruleset across PHP versions, I have chosen to only support setting properties when:

A property is explicitly declared on a sniff class.
A sniff class explicitly declares (or inherits) the magic __set() method.
A sniff class extends stdClass.

Note: no additional check has been added to verify that a declared property is public. If that is not the case a PHP native error would be thrown previously and still will be now. This behaviour has not changed.

I have chosen not to respect the #[\AllowDynamicProperties] attribute as it is not possible (without tokenizing the sniff files which would create a lot of overhead) to determine whether that attribute exists on a class when PHPCS runs on PHP < 8.0 as the Reflection method needed to detect the attribute is only available on PHP 8.0+.

In other words, adding support for the attribute would introduce an inconsistency in how properties set in a ruleset are handled based on the PHP version on which PHPCS is being run.
It would also mean that the error notice for invalid properties set on individual sniffs would only be available on PHP 8.2+, which would greatly reduce the usefulness of the error notice.

In my opinion, consistency here is more important than supporting the attribute, especially as there are three fully supported ways to allow for supporting properties to be set from a ruleset.
The three above mentioned ways to allow for setting properties from a ruleset for a sniff are all fully supported on all PHP versions currently supported by PHPCS, so there is no compelling reason to also allow for the attribute.

Suggested changelog entry

PHP 8.2: prevent deprecation notices for properties set in a (custom) ruleset for complete standards/complete sniff categories

Invalid properties set for individual sniffs will now result in an error and halt the execution of PHPCS.

Properties set for complete standards/complete sniff categories will now only be set on sniffs which explicitly support the property.
The property will be silently ignored for those sniffs which do not support the property.

For sniff developers: it is strongly recommended for sniffs to explicitly declare any user-adjustable public properties.
If dynamic properties need to be supported for a sniff, either declare the magic __set()/__get()/__isset()/__unset() methods on the sniff or let the sniff extend stdClass.
Note: The #[\AllowDynamicProperties] attribute will have no effect for properties which are being set in rulesets.

Sniff developers/integrators of PHPCS may need to make some small adjustments to allow for changes to the PHPCS internals. See the PR description of PR PHP 8.2 | Ruleset: prevent notices about dynamic properties being set #3629 for full details.

Other notes

The changes are accompanied by a full set of tests covering the change.
There is a little overlap between the RuleInclusionTest class and the new SetSniffPropertyTest class, but for the most part, the tests compliment each other.

Includes:

Correct handling of properties being set via phpcs:set annotations.
The RuleInclusionTest XML fixture and test expectations have been adjusted to allow for these changes.
1. As we now need "settable" properties to exist on sniffs to use for the testSettingProperties() test, the PSR1 ruleset is not sufficient as the sniffs in that ruleset don't have public properties.
  For that reason, the "set property for complete standard" test has been changed to use PSR2 instead and set the indent property.
  The test expectations has been adjusted to match.
2. Along the same lines, the Zend.NamingConventions sniffs don't have public properties, so the "setting property for complete category" test has been switched to PSR12.Operators and set the ignoreSpacingBeforeAssignments property.
  The test expectations has been adjusted to match.
3. In both these cases, the XML <rule> now contains both a property which is valid for at least some sniffs in the standard/category and a property which is invalid in for all sniffs.
  For the invalid properties, a separate testSettingInvalidPropertiesOnStandardsAndCategoriesSilentlyFails() test with data provider has been added to verify these will not be set.
4. As the ruleset has changed, the expectations for the testHasSniffCodes() test and the testRegisteredSniffCodes() test have been updated to match.
5. The test descriptions for the test cases in the dataSettingProperties() have also been updated to make it more explicit what each test case is testing.
The PHPCS ruleset has been updated to exclude sniffs which function as test fixtures from the PHPCS check for this repo.
A minor tweak has been made to the ValidatePEARPackageXML script to prevent a warning about sniffs which function as test fixtures.
These test fixtures should receive the role="test" attribute, not a role="php" attribute.

Fixes #3489

jrfnl · 2022-07-09T12:37:34Z

or to call the setSniffProperty() method on the Ruleset object directly (which may happen in custom test frameworks for external standards).

I could possibly make this BC-break smaller if so desired.

My thoughts on this:

Check if the received param is an array in the expected (new) format or a string/array in another format (= value).
If not the expected (new) format, use the parameter value as the "value" and set the "scope" to standard + throw a deprecation notice to alert integrators.
The deprecation notice + code handling the "old" parameter format could then be removed in PHPCS 4.0.0.

Would this be desired ?

A code search shows me that at least the following external standards/integrations would/will be impacted by the change:

gmazzap · 2022-07-09T15:26:15Z

Thank you @jrfnl for the excellent work on this and the ping.

As per the change, this will affect us on the tests for the custom sniffs, so it will not impact CI for production libraries. I will monitor this issue and fix the tests when this is merged.

Thanks again.

jrfnl · 2022-07-26T13:09:10Z

Ha! Managed to still find one bug in the original PR. (support for array properties with extend="true" was broken)

As it looks like no-one has started reviewing this PR yet, I've fixed that up in the original commit and made a small tweak to the tests to make sure that situation will always be checked as well.

benjaminprojas · 2023-04-07T16:15:28Z

Any traction on this? I'm not 100% sure if this will fix it, but VS Code no longer runs PHPCS because of the PHP 8.2 errors thrown through these files. My specific error is:

PHP Deprecated:  Creation of dynamic property WordPressCS\WordPress\Sniffs\WP\I18nSniff::$check_translator_comments is deprecated in /Users/benjamin/Work/Awesome Motive/AIOSEO/git/aioseo/vendor/squizlabs/php_codesniffer/src/Ruleset.php on line 1331

jrfnl · 2023-04-07T16:52:07Z

Any traction on this? I'm not 100% sure if this will fix it, but VS Code no longer runs PHPCS because of the PHP 8.2 errors thrown through these files. My specific error is:

This PR would turn that notice into a ruleset error as described in the "For end-users" section above.

Off-topic for this PR:
I presume you are getting that error because you are using the develop branch of WPCS. If you use develop, you need to keep yourself informed of changes as it is by nature an "unstable" branch.
In this case, you are getting the notification as you are setting a property in your ruleset which was always discouraged to be set and which has now been removed, so you need to fix your ruleset.
See the commit message here: WordPress/WordPress-Coding-Standards@31064cf

benjaminprojas · 2023-04-07T17:24:49Z

Thanks for the input on that. Looks like I was confused by the purpose of this PR.

But your comment pointed me in the right direction. Turns out this is added by the HM-Minimum ruleset we are using to extend the WPCS rules: https://github.com/humanmade/coding-standards/blob/e8ec095e7128a3819e254a238902746ec00ece0d/HM-Minimum/ruleset.xml#L160-L163

We already have a forked copy for PHP 8.x purposes so the fix was easy for us. I'll open an issue there as well.

jrfnl · 2023-05-04T07:14:45Z

@gsherwood and me discussed this and I'll make the BC break for extenders smaller as per comment #3629 (comment)

### ⚠️ This PR contains two - albeit small - BC-breaks! ⚠️ The BC-breaks can be broken down as follows: ### For end-users If a property is set via a ruleset for an **_individual_** sniff and that sniff does not have the property explicitly declared, nor does the sniff declare the PHP magic `__set()` method or `extend stdClass`, the ruleset will now be regarded as invalid. In practice, this means the PHPCS run will halt and will show an informative notice to the end-user about the invalid property setting and the PHPCS run will exit with exit code `3`. For properties being set for a complete category of sniffs or a complete standard, the PHPCS run will not halt, nor show an error when the property isn't supported for all sniffs in the category/standard. In that case, the property will only be set on those sniffs which support it (i.e. either have the property explicitly declared on the sniff, or have the magic `__set()` method declared on the sniff, or `extend stdClass`) and will be silently ignored for all other sniffs. 👉 Aside from possibly surfacing some incidental typos in property settings in rulesets, I expect this change to go mostly unnoticed by end-users. ### For developers of PHPCS standards/integrations 1. The format of the `$ruleset['sniff code']['properties']['property name']` sub-array has changed from a `array|string` type property value to a sub-array containing two entries: 1. `'value'` - containing the `array|string` type property value. 2. `'scope'` - containing a string, either `sniff` or `standard`, to indicate whether the ruleset property directive was for an individual sniff or for a whole standard/category of sniffs. 2. The third parameter for the `Ruleset::setSniffProperty()` method has been changed to expect the above mentioned array of `$settings` instead of just a property value. This change is necessary to allow for throwing the error notice when an invalid property is being set on an invalid sniff versus silently ignoring the invalid property if the ruleset specified it on a standard/category. 👉 The impact of this BC-break is expected to be small, as it would require for an external standard or integration of PHPCS to: * `extend` the `Ruleset` class **and** overload the `setSniffProperty()` method to have any impact; * or to call the `setSniffProperty()` method on the `Ruleset` object directly (which _may_ happen in custom test frameworks for external standards). Note: * The change to the `processRule()` method cannot be overloaded as it is a `private` method. * The change in the `populateTokenListeners()` method is only cosmetic and has no functional impact. --- ### Technical choices made: For consistent handling of properties set in a (custom) ruleset across PHP versions, I have chosen to only support setting properties when: * A property is explicitly declared on a sniff class. * A sniff class explicitly declares (or inherits) the magic `__set()` method. * A sniff class `extends` `stdClass`. Note: no additional check has been added to verify that a declared property is `public`. If that is not the case a PHP native error would be thrown previously and still will be now. This behaviour has not changed. I have chosen **not** to respect the `#[\AllowDynamicProperties]` attribute as it is not possible (without tokenizing the sniff files which would create a lot of overhead) to determine whether that attribute exists on a class when PHPCS runs on PHP < 8.0 as the Reflection method needed to detect the attribute is only available on PHP 8.0+. In other words, adding support for the attribute would introduce an inconsistency in how properties set in a ruleset are handled based on the PHP version on which PHPCS is being run. It would also mean that the error notice for invalid properties set on individual sniffs would only be available on PHP 8.2+, which would greatly reduce the usefulness of the error notice. In my opinion, consistency here is more important than supporting the attribute, especially as there are three fully supported ways to allow for supporting properties to be set from a ruleset. The three above mentioned ways to allow for setting properties from a ruleset for a sniff are all fully supported on all PHP versions currently supported by PHPCS, so there is no compelling reason to also allow for the attribute. ### Suggested changelog entry > PHP 8.2: prevent deprecation notices for properties set in a (custom) ruleset for complete standards/complete sniff categories > - Invalid properties set for individual sniffs will now result in an error and halt the execution of PHPCS. > - Properties set for complete standards/complete sniff categories will now only be set on sniffs which explicitly support the property. > The property will be silently ignored for those sniffs which do not support the property. > - For sniff developers: it is strongly recommended for sniffs to explicitly declare any user-adjustable `public` properties. > If dynamic properties need to be supported for a sniff, either declare the magic `__set()`/`__get()`/`__isset()`/`__unset()` methods on the sniff or let the sniff extend `stdClass`. > Note: The `#[\AllowDynamicProperties]` attribute will have **no effect** for properties which are being set in rulesets. > - Sniff developers/integrators of PHPCS may need to make some small adjustments to allow for changes to the PHPCS internals. See the description of PR 3629 for full details. ### Other notes The changes are accompanied by a full set of tests covering the change. There is a little overlap between the `RuleInclusionTest` class and the new `SetSniffPropertyTest` class, but for the most part, the tests compliment each other. Includes: * Correct handling of properties being set via `phpcs:set` annotations. * The `RuleInclusionTest` XML fixture and test expectations have been adjusted to allow for these changes. 1. As we now need "settable" properties to exist on sniffs to use for the `testSettingProperties()` test, the `PSR1` ruleset is not sufficient as the sniffs in that ruleset don't have public properties. For that reason, the "set property for complete standard" test has been changed to use `PSR2` instead and set the `indent` property. The test expectations has been adjusted to match. 2. Along the same lines, the `Zend.NamingConventions` sniffs don't have public properties, so the "setting property for complete category" test has been switched to `PSR12.Operators` and set the `ignoreSpacingBeforeAssignments` property. The test expectations has been adjusted to match. 3. In both these cases, the XML `<rule>` now contains both a property which is valid for at least some sniffs in the standard/category and a property which is invalid in for all sniffs. For the _invalid_ properties, a separate `testSettingInvalidPropertiesOnStandardsAndCategoriesSilentlyFails()` test with data provider has been added to verify these will not be set. 4. As the ruleset has changed, the expectations for the `testHasSniffCodes()` test and the `testRegisteredSniffCodes()` test have been updated to match. 5. The test descriptions for the test cases in the `dataSettingProperties()` have also been updated to make it more explicit what each test case is testing. * The PHPCS ruleset has been updated to exclude sniffs which function as test fixtures from the PHPCS check for this repo. * A minor tweak has been made to the `ValidatePEARPackageXML` script to prevent a warning about sniffs which function as test fixtures. These test fixtures should receive the `role="test"` attribute, not a `role="php"` attribute. Fixes 3489

jrfnl · 2023-05-04T20:11:52Z

I've rebased the original commit without changes and added a second commit to this PR to implement the BC-layer as per #3629 (comment).

⚠️ Note: this new, second commit should NOT be ported to PHPCS 4.x.

Ruleset::setSniffProperty(): add BC-layer for old format property values

This commit adds a BC layer to handle property values passed to Ruleset::setSniffProperty() in the old (mixed) format.

This BC-layer will never be hit when PHPCS is used from the CLI/with a ruleset. This BC-layer is only in place for integrations with PHPCS which may call the Ruleset::setSniffProperty() method directly.

The Ruleset::setSniffProperty() will still handle properties passed in the old format correctly, but will also throw a deprecation notice to allow the maintainers of the integration to update their code.

Includes dedicated tests to ensure this BC-layer works as intended.

Updated suggested changelog entry

PHP 8.2: prevent deprecation notices for properties set in a (custom) ruleset for complete standards/complete sniff categories

Invalid properties set for individual sniffs will now result in an error and halt the execution of PHPCS with a descriptive error message to allow users to fix their ruleset.

Properties set for complete standards/complete sniff categories will now only be set on sniffs which explicitly support the property.
The property will be silently ignored for those sniffs which do not support the property.

For sniff developers: it is strongly recommended for sniffs to explicitly declare any user-adjustable public properties.
If dynamic properties need to be supported for a sniff, either declare the magic __set()/__get()/__isset()/__unset() methods on the sniff or let the sniff extend stdClass.
Note: The #[\AllowDynamicProperties] attribute will have no effect for properties which are being set in rulesets.

Sniff developers/integrators of PHPCS may need to make some small adjustments to allow for changes to the PHPCS internals.
A deprecation notice will be thrown for those situations. See the PR description of PR PHP 8.2 | Ruleset: prevent notices about dynamic properties being set #3629 for full details.

This commit adds a BC layer to handle property values passed to `Ruleset::setSniffProperty()` in the old (mixed) format. This BC-layer will never be hit when PHPCS is used from the CLI/with a ruleset. This BC-layer is only in place for integrations with PHPCS which may call the `Ruleset::setSniffProperty()` method directly. The `Ruleset::setSniffProperty()` will still handle properties passed in the old format correctly, but will also throw a deprecation notice to allow the maintainers of the integration to update their code. Includes dedicated tests to ensure this BC-layer works as intended. Note: this commit should **NOT** be ported to PHPCS 4.x.

jrfnl · 2023-12-08T23:36:20Z

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).

Refs: * squizlabs/PHP_CodeSniffer#3629 * squizlabs/PHP_CodeSniffer#3489

jrfnl mentioned this pull request Jul 9, 2022

PHP 8.2: dynamic properties are deprecated #3489

Closed

jrfnl force-pushed the feature/php-8.2-dynamic-properties-are-deprecated branch from 713b88a to b0c435b Compare July 9, 2022 03:22

jrfnl force-pushed the feature/php-8.2-dynamic-properties-are-deprecated branch 2 times, most recently from 339d26d to 5d99541 Compare July 26, 2022 13:06

gsherwood added this to the 3.8.0 milestone Sep 15, 2022

jrfnl mentioned this pull request Nov 14, 2022

PHPCSUtils Breaks With Deprecation Notice ~~Because of Itself~~ PHPCSStandards/PHPCSUtils#380

Closed

jrfnl force-pushed the feature/php-8.2-dynamic-properties-are-deprecated branch from 5d99541 to 17a8e76 Compare March 2, 2023 04:14

This was referenced Apr 8, 2023

can support for slevomat/coding-standard v6 be removed? cakephp/cakephp-codesniffer#382

Closed

Update ruleset.xml cakephp/cakephp-codesniffer#381

Merged

jrfnl force-pushed the feature/php-8.2-dynamic-properties-are-deprecated branch from 17a8e76 to 8fb785b Compare May 4, 2023 20:06

jrfnl force-pushed the feature/php-8.2-dynamic-properties-are-deprecated branch from 8fb785b to 350a9b3 Compare May 4, 2023 20:14

jrfnl force-pushed the feature/php-8.2-dynamic-properties-are-deprecated branch from 350a9b3 to 48c2ae3 Compare May 4, 2023 20:21

jrfnl added Type: bug Core Component: Config & Ruleset & CLI options labels May 7, 2023

gsherwood added a commit that referenced this pull request May 18, 2023

Changelog for #3629

87bdec6

gsherwood merged commit 059989d into squizlabs:master May 18, 2023

jrfnl deleted the feature/php-8.2-dynamic-properties-are-deprecated branch May 18, 2023 07:45

jrfnl mentioned this pull request Jul 16, 2023

Make test suite compatible with PHPCS 3.8.0 sirbrillig/phpcs-variable-analysis#304

Merged

jrfnl mentioned this pull request Oct 31, 2023

Don't stop scan on invalid inline property annotation #3916

Closed

5 tasks

This was referenced Nov 8, 2023

Feature suggestion: --space-indent=# command line option #2229

Closed

Don't stop scan on invalid inline property annotation PHPCSStandards/PHP_CodeSniffer#99

Merged

jrfnl added a commit to jrfnl/slevomat-coding-standard that referenced this pull request Dec 9, 2023

TestCase: update for new Ruleset::$properties format

2df5218

Refs: * squizlabs/PHP_CodeSniffer#3629 * squizlabs/PHP_CodeSniffer#3489

jrfnl added a commit to jrfnl/slevomat-coding-standard that referenced this pull request Dec 9, 2023

TestCase: update for new Ruleset::$properties format

eb92adb

Refs: * squizlabs/PHP_CodeSniffer#3629 * squizlabs/PHP_CodeSniffer#3489

jrfnl mentioned this pull request Dec 9, 2023

Revert "replace squizlabs/php_codesniffer with phpcsstandards/php_codesniffer" slevomat/coding-standard#1646

Merged

kukulich pushed a commit to slevomat/coding-standard that referenced this pull request Dec 11, 2023

TestCase: update for new Ruleset::$properties format

ad4eab6

Refs: * squizlabs/PHP_CodeSniffer#3629 * squizlabs/PHP_CodeSniffer#3489

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHP 8.2 | Ruleset: prevent notices about dynamic properties being set #3629

PHP 8.2 | Ruleset: prevent notices about dynamic properties being set #3629

jrfnl commented Jul 9, 2022

jrfnl commented Jul 9, 2022

gmazzap commented Jul 9, 2022 •

edited

Loading

jrfnl commented Jul 26, 2022

benjaminprojas commented Apr 7, 2023

jrfnl commented Apr 7, 2023

benjaminprojas commented Apr 7, 2023

jrfnl commented May 4, 2023

jrfnl commented May 4, 2023 •

edited

Loading

jrfnl commented Dec 8, 2023

PHP 8.2 | Ruleset: prevent notices about dynamic properties being set #3629

PHP 8.2 | Ruleset: prevent notices about dynamic properties being set #3629

Conversation

jrfnl commented Jul 9, 2022

⚠️ This PR contains two - albeit small - BC-breaks! ⚠️

For end-users

For developers of PHPCS standards/integrations

Technical choices made:

Suggested changelog entry

Other notes

jrfnl commented Jul 9, 2022

gmazzap commented Jul 9, 2022 • edited Loading

jrfnl commented Jul 26, 2022

benjaminprojas commented Apr 7, 2023

jrfnl commented Apr 7, 2023

benjaminprojas commented Apr 7, 2023

jrfnl commented May 4, 2023

jrfnl commented May 4, 2023 • edited Loading

Ruleset::setSniffProperty(): add BC-layer for old format property values

Updated suggested changelog entry

jrfnl commented Dec 8, 2023

gmazzap commented Jul 9, 2022 •

edited

Loading

jrfnl commented May 4, 2023 •

edited

Loading