WordPress / WordPress-Coding-Standards

PHP_CodeSniffer rules (sniffs) to enforce WordPress coding conventions
MIT License
2.56k stars 485 forks source link

AbstractArrayAssignmentRestrictionsSniff doesn't correctly capture array values with no trailing comma #2211

Closed GaryJones closed 1 year ago

GaryJones commented 1 year ago

Bug Description

Sniff classes that extend the AbstractArrayAssignmentRestrictionsSniff class don't capture the correct value when the array item has no trailing comma.

Minimal Code Snippet

Example 1

Testing the PostPerPage sniff which extends AbstractArrayAssignmentRestrictionsSniff. With this test file:

<?php

$args = array(
    'posts_per_page' => 999,
);

$args = array(
    'posts_per_page' => 999
);

...and this command (run inside a Git clone of WPCS):

$ ./vendor/bin/phpcs --standard=WordPress ../phpcs-test.php

The output includes:

 1 | ERROR   | [ ] Missing file doc comment (Squiz.Commenting.FileComment.Missing)
 4 | WARNING | [ ] Detected high pagination limit, `posts_per_page` is set to `999`
   |         |     (WordPress.WP.PostsPerPage.posts_per_page_posts_per_page)
 8 | WARNING | [ ] Detected high pagination limit, `posts_per_page` is set to `999
   |         |     )` (WordPress.WP.PostsPerPage.posts_per_page_posts_per_page)
 8 | ERROR   | [x] Each array item in a multi-line array declaration must end in a comma (WordPress.Arrays.CommaAfterArrayItem.NoComma)

Line 4 and line 8 (the 999 lines) are both captured as a violation, though interestingly, the "value" captured for line 8 seems to be 999 ) (with a line break as well), as though it hasn't recognised the line break as being the intended end of the line/value. I suspect there's some casting to int going on where 999\n( is seen as 999, which is sufficient to make it a violation.


Example 2

From the VIPCS NoPaging sniff, which also extends WPCS's AbstractArrayAssignmentRestrictionsSniff, a test file of:

<?php

$args = array(
    'nopaging' => true,
);

$args = array(
    'nopaging' => true
);

...and a command of:

$ ./vendor/bin/phpcs --standard=WordPress-VIP-Go -s ../phpcs-test.php

...only captures the first instance:

 4 | ERROR | Disabling pagination is prohibited in VIP context, do not set `nopaging` to `true` ever.
   |       | (WordPressVIPMinimum.Performance.NoPaging.nopaging_nopaging)

...because I suspect the second value is seen as true ) (with a line break), which doesn't resolve to true (unlike the int casting from Example 1).

Testing with $args = array('nopaging=true'); and the violation is correctly captured.

Environment

Question Answer
PHP version 7.4 and 8.1
PHP_CodeSniffer version 3.7.1
WPCS version develop
WPCS install type git clone for development

Additional Context (optional)

Originally reported in https://github.com/Automattic/VIP-Coding-Standards/issues/713.

Tested Against develop branch?

jrfnl commented 1 year ago

I have a fix lined up for this, but this sniff will need quite some more fixes. Will pull it as part of a larger PR which also implements PHPCSUtils.

jrfnl commented 1 year ago

Looks like this issue is a duplicate of #1505. As #1505 was closed already, I won't mark this issue as a duplicate, but leaving this comment to make sure the issues are linked.