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

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

Closed
1 task done
GaryJones opened this issue Mar 7, 2023 · 2 comments · Fixed by #2266
Closed
1 task done
Assignees
Milestone

Comments

@GaryJones
Copy link
Member

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 Automattic/VIP-Coding-Standards#713.

Tested Against develop branch?

  • I have verified the issue still exists in the develop branch of WPCS.
@jrfnl
Copy link
Member

jrfnl commented Mar 7, 2023

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
Copy link
Member

jrfnl commented Jul 4, 2023

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.

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