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

Fix posts_per_page sniff #1297

Closed
wants to merge 2 commits into from
Closed

Conversation

pyronaur
Copy link

@pyronaur pyronaur commented Feb 7, 2018

This fixes Automattic/VIP-Coding-Standards#29

phpcs is looking for T_COMMA, T_SEMICOLON for $valEnd - things like comments, spaces and closing parentheses are also included when there is no T_COMMA.

For example, this is not reported:
$args = array( 'posts_per_page' => -1 ); // Bad

At first I tried to just append closing parentheses and closing square brackets to the tokens to look for, but that didn't work out because in some situations, with a missing comma - it would still include comments and spaces.

This approach is going to find a $valEnd (either semicolon or a comma) then try to step back until it finds a valid value (but not further back than $valStart). If a string/number/bool/constant is found it'll overwrite existing $valEnd with the backtracked position.

Norris added 2 commits February 7, 2018 15:24
Necessary to avoid adding `)` and comments to the string when there is a comma
@jrfnl
Copy link
Member

jrfnl commented Mar 13, 2018

@justnorris Thanks for the PR.

In my opinion, your PR as it is, unfortunately should not be accepted as it is making incorrect assumptions about the type of content which can be in an array value which is not appropriate for an abstract class which is intended to be used by more than just the sniff being addressed here.

I have to admit that I think a complete rewrite of that abstract class for more accurate token walking and code style independence is probably in order.

The issue you linked to should probably have been reported here as an issue instead of in the VIP repo.

@jrfnl
Copy link
Member

jrfnl commented Jul 6, 2018

@justnorris @tomjn Just checking - as the VIP sniffs are going to be deprecated in WPCS, can this PR be closed ?
See #1309, #1409.

@tomjn
Copy link
Contributor

tomjn commented Jul 7, 2018

Yes it can be closed, @justnorris we can build this into a sniff in our own repo

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

Successfully merging this pull request may close these issues.

PostsPerPageSniff.php does not catch the -1 if the line is not followed by a comma
3 participants