Ruleset::processRule(): fix edge case bug - inconsistent handling of empty string array key #1193
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…y value Add tests for a scenario that I found on the WPCS codebase that I believe was not documented in a test in PHPCS, and I'm not sure if it is intentional or not. Also, not sure if it should be documented explicitly in the upgrade guide. https://github.com/WordPress/WordPress-Coding-Standards/blob/4d0160f32f8537f3cdf2e301433cb15641083963/WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.1.inc#L513 Co-authored-by: jrfnl <[email protected]>
…empty string array key Unlikely scenario for real-life, but still a bug. When an array property is set via the old comma-separated string ruleset format or via an inline `phpcs:set` annotation, and the key for an array element would be explicitly set to an empty string, the resulting array item in the property on the sniff class would be added without a key, resulting in a numeric key. However, if the (not so) "new" ruleset format using `<element>` nodes is used and an array element key is an empty string, this would result in the array item being added with an empty string for the array key. This is inconsistent behaviour and is now fixed. As 2 out of 3 scenarios would result in a numeric key for PHPCS 3.x, the behaviour has now been standardized as such. Note: for PHPCS 4.x, an empty string array key will remain an empty string and will not become a numeric key. This is in-line with the type handling proposal in 708, which explicitly states: > I do NOT intend to change the handling of array keys. These will remain strings at all times.
|
A documentation update for the 4.0 upgrade guide, documenting how this will be handled in 4.0 has been pulled here: PHPCSStandards/PHP_CodeSniffer-documentation#48 |
rodrigoprimo
added a commit
to rodrigoprimo/WordPress-Coding-Standards
that referenced
this pull request
Aug 18, 2025
Add a new error when validating prefixes if a prefix is not a string or is an empty string. This is necessary in preparation for PHPCS 4.0. Starting with this version, array properties can have types other than string, and the sniff needs to be updated to handle non-string prefixes. Using the same error when the string is empty made sense as before the sniff would generate an error complaining about the length of the prefix. TODO: the value of the prefix is type casted to a string when displaying the error message. For example, `true` becomes `1`. I need to think if this is a problem or not. Also, if `true` is passed as the first and only element of an array property there seems to be an error in PHPCS. Check how PHPCSStandards/PHP_CodeSniffer#1172 and PHPCSStandards/PHP_CodeSniffer#1193 impact this commit.
rodrigoprimo
approved these changes
Aug 18, 2025
jrfnl
deleted the
feature/ruleset-document-handling-empty-string-in-array
branch
|
Commit 038abb6 shows the different implementation for PHPCS 4.0. |
|
@jrfnl, the commit with the PHPCS 4.0 implementation looks good to me as well. |
|
@rodrigoprimo Thanks for reporting the issue and checking the implementations. |
rodrigoprimo
added a commit
to rodrigoprimo/WordPress-Coding-Standards
that referenced
this pull request
Aug 21, 2025
Add a new error when validating prefixes if a prefix is not a string or is an empty string. This is necessary in preparation for PHPCS 4.0. Starting with this version, array properties can have types other than string, and the sniff needs to be updated to handle non-string prefixes. Using the same error when the string is empty made sense as before the sniff would generate an error complaining about the length of the prefix. TODO: the value of the prefix is type casted to a string when displaying the error message. For example, `true` becomes `1`. I need to think if this is a problem or not. Also, if `true` is passed as the first and only element of an array property there seems to be an error in PHPCS. Check how PHPCSStandards/PHP_CodeSniffer#1172 and PHPCSStandards/PHP_CodeSniffer#1193 impact this commit.
rodrigoprimo
added a commit
to rodrigoprimo/WordPress-Coding-Standards
that referenced
this pull request
Aug 21, 2025
Add a new error when validating prefixes if a prefix is not a string or is an empty string. This is necessary in preparation for PHPCS 4.0. Starting with this version, array properties can have types other than string, and the sniff needs to be updated to handle non-string prefixes. Using the same error when the string is empty made sense as before the sniff would generate an error complaining about the length of the prefix. TODO: the value of the prefix is type casted to a string when displaying the error message. For example, `true` becomes `1`. I need to think if this is a problem or not. Also, if `true` is passed as the first and only element of an array property there seems to be an error in PHPCS. Check how PHPCSStandards/PHP_CodeSniffer#1172 and PHPCSStandards/PHP_CodeSniffer#1193 impact this commit.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Ruleset::setSniffProperty(): add extra test with empty string as array value
Add tests for a scenario that I found on the WPCS codebase that I
believe was not documented in a test in PHPCS, and I'm not sure if it is
intentional or not. Also, not sure if it should be documented explicitly
in the upgrade guide.
https://github.com/WordPress/WordPress-Coding-Standards/blob/4d0160f32f8537f3cdf2e301433cb15641083963/WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.1.inc#L513
Ruleset::processRule(): fix edge case bug - inconsistent handling of empty string array key
Unlikely scenario for real-life, but still a bug.
When an array property is set via the old comma-separated string ruleset format or via an inline
phpcs:setannotation, and the key for an array element would be explicitly set to an empty string, the resulting array item in the property on the sniff class would be added without a key, resulting in a numeric key.However, if the (not so) "new" ruleset format using
<element>nodes is used and an array element key is an empty string, this would result in the array item being added with an empty string for the array key.This is inconsistent behaviour and is now fixed.
As 2 out of 3 scenarios would result in a numeric key for PHPCS 3.x, the behaviour has now been standardized as such.
Suggested changelog entry
Fixed: edge case inconsistency in how empty string array keys for sniff properties are handled.
Additional notes
Turns out this was also inconsistent for PHPCS 4.x.
I propose for PHPCS 4.x that an empty string array key will remain an empty string and will not become a numeric key.
This does constitute a behavioural change, but only for a very unlikely edge case, so I do not think it needs to block the release.
This change for 4.0 is also in-line with the type handling proposal in #708, which was published ten months ago and which explicitly states:
Related: #1006, #1172