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

phpcs:set annotations do not cause the line they are on to be ignored #1939

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Mar 19, 2018

Tokenizer: minor bug fix

The ignore setting for PHPCS annotations on their own line uses ['all' => true] everywhere else, except for the phpcs:set annotation.

This has now been made consistent.


SuperfluousWhitespace: fix fixer bug

The change in the tokenizer exposed a bug in the Squiz.WhiteSpace.SuperfluousWhitespace sniff, where if the last non-whitespace token in a file is a PHPCS annotation, the fixer would not kick in.

Moving the fixer start token determination for JS/CSS files into the fixer code block, fixes this.

The ignore setting for PHPCS annotations on their own line uses `['all' => true]` everywhere else, except for the `phpcs:set` annotation.

This has now been made consistent.
The change in the tokenizer exposes a bug in this sniff, where if the last non-whitespace token in a file is a PHPCS annotation, the fixer would not kick in.

Moving the start token determination for JS/CSS files into the fixer code block, fixes this.
@gsherwood
Copy link
Member

gsherwood commented Mar 21, 2018

Looks like there are more problems here. Muting that whole line means that this code wont report an error with that sniff:

<?php
/* phpcs:set foo bar */ ?>

Although it still fixes it for some reason, I think (still need to look into that).

But also, no error for start of file whitespace will be reported for this:

<?php /* phpcs:set foo bar */
?>

So I thought I'd fix the check to make sure the comment is actually on a line by itself and found that check only looks towards the front of the line and doesn't check if there is any content after the comment. But it also ignores open tags before the comment, so it sees <?php /* phpcs:set foo bar */ as being on a comment on a line by itself and so would never report errors on the open tag, which is pretty bad given a lot of sniffs do that.

More to look into.

@jrfnl
Copy link
Contributor Author

jrfnl commented Mar 21, 2018

@gsherwood Just checking: Are you looking into this or would you like me to dig deeper ?

@gsherwood
Copy link
Member

Are you looking into this or would you like me to dig deeper ?

I'm looking into it. Have an idea I'm testing out.

@gsherwood gsherwood changed the title Tokenizer: minor bug fix + fixer bug in Squiz/SuperfluousWhitespace sniff phpcs:set annotations do not cause the line they are on to be ignored Mar 21, 2018
@gsherwood gsherwood added this to the 3.3.0 milestone Mar 21, 2018
@gsherwood gsherwood merged commit 7e01514 into squizlabs:master Mar 21, 2018
gsherwood added a commit that referenced this pull request Mar 21, 2018
@gsherwood
Copy link
Member

I had to differentiate between lines that had no other tokens on them, and lines that had no other important tokens on them. The phpcs:set annotation needed to only ignore the line if it was really the only thing on there, which the other ones (especially phpcs:ignore) could ignore the line they are on even if there are PHP tags.

Tests are all passing, so I think it's working.

@jrfnl jrfnl deleted the feature/tokenizer-annotation-ignore-consistency branch March 21, 2018 03:45
@jrfnl
Copy link
Contributor Author

jrfnl commented Mar 21, 2018

@gsherwood Nice, though things are getting more and more complicated. Happy to hear this is fixed now though.

gsherwood added a commit that referenced this pull request Mar 21, 2018
@gsherwood gsherwood removed this from Ready for Release in PHPCS v3 Development Jun 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants