PHPCS annotations: selective re-enabling not working as expected

[jrfnl]

Example code based on: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#ignoring-parts-of-a-file with some small adjustments to test this properly:

// phpcs:disable PEAR,Squiz.Arrays
$foo = [1,2,3];
bar( $foo, true );
// phpcs:enable PEAR.Functions.FunctionCallSignature
bar( $foo, true );
// phpcs:enable Squiz
$foo = [1,2,3];
// phpcs:enable

When running PHPCS over this snippet using phpcs -p -s ./test.php --standard=PEAR,Squiz --ignore-annotations, I see the following errors (ignoring the filecomment related errors):

 4 | ERROR | [x] Array with multiple values cannot be declared on a single line
   |       |     (Squiz.Arrays.ArrayDeclaration.SingleLineNotAllowed)
 5 | ERROR | [x] Space after opening parenthesis of function call prohibited
   |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
 5 | ERROR | [x] Expected 0 spaces before closing bracket; 1 found
   |       |     (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)
 7 | ERROR | [x] Space after opening parenthesis of function call prohibited
   |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
 7 | ERROR | [x] Expected 0 spaces before closing bracket; 1 found
   |       |     (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)
 9 | ERROR | [x] Array with multiple values cannot be declared on a single line
   |       |     (Squiz.Arrays.ArrayDeclaration.SingleLineNotAllowed)

If I repeat this without the --ignore-annotations, I only see the file comment errors, while I would expect to see:

• The PEAR.Functions.FunctionCallSignature errors on line 7
• The Squiz.Arrays.ArrayDeclaration error on line 9

A cursory analysis gives me the impression that selective re-enabling only works when using the exact same "selection" as for the disabling, so:

// phpcs:disable PEAR.Functions.FunctionCallSignature,Squiz.Arrays
$foo = [1,2,3];
bar( $foo, true );
// phpcs:enable PEAR.Functions.FunctionCallSignature
bar( $foo, true );
// phpcs:enable Squiz.Arrays
$foo = [1,2,3];
// phpcs:enable

... works as expected.

Using a higher/lower level Standard/Category/SniffName in the enable vs the disable command, seems to break the functionality which is in stark contrast to what the documentation suggests.

I've checked the above against various points in time between the 3.2.0 version and the current master, but it looks like this was never implemented as intended.

[gsherwood]

A cursory analysis gives me the impression that selective re-enabling only works when using the exact same "selection" as for the disabling

That looks right to me too, which wasn't the intention.

[gsherwood]

Getting this working:

// phpcs:disable PEAR,Squiz.Arrays
$foo = [1,2,3];
bar( $foo, true );
// phpcs:enable PEAR.Functions.FunctionCallSignature
bar( $foo, true );
// phpcs:enable Squiz
$foo = [1,2,3];
// phpcs:enable

Is going to be pretty hard using the current system. It only maintains a list of sniffs that are currently disabled, which allows you to re-enable a superset of sniffs (just remove entires from the list) but not a subset or a specific sniff, like in this case.

I think this will require a second whitelist to be created and used during the adding of messages, which is't ideal but I can't think of another way right now. Will think more though, and try and few things.

[jrfnl]

Is going to be pretty hard using the current system.

@gsherwood Yes, the only thing I could come up with while I was investigating this, was actually breaking the codes used in the "selection" down to the individual parts and setting it off against the sniffs included via the ruleset.
That would, however, require both the disable logic in the Tokenizer as well as the enable logic to loop through all the "active" sniffs and disable/enable them individually.

[gsherwood]

I've committed a change to get this working. I ended up having to use an exception list, but I worked that into the current ignoredLines member var and did my best not to use it unless needed.

I also renamed the existing all entry in there to .all because it is possible to have a standard called All so that could have got confusing.

I'll leave this open in case you have some time to test.

[jrfnl]

@gsherwood Thanks for the quick turn-around on this one.

Tests are mostly looking good, though I found one weird bug.

Given this example code:

// phpcs:disable PEAR
bar( $foo, true );
// phpcs:enable PEAR.Functions.FunctionCallSignature
bar( $foo, true );

// phpcs:disable PEAR.Functions.FunctionCallSignature
bar( $foo, true );
// phpcs:enable
bar( $foo, true );

When running PHPCS over this snippet using phpcs -p -s ./test.php --standard=PEAR --ignore-annotations, I see the following errors (ignoring the comment related errors):

  4 | ERROR | [x] Space after opening parenthesis of function call prohibited
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
  4 | ERROR | [x] Expected 0 spaces before closing bracket; 1 found
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)
  6 | ERROR | [x] Space after opening parenthesis of function call prohibited
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
  6 | ERROR | [x] Expected 0 spaces before closing bracket; 1 found
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)
  9 | ERROR | [x] Space after opening parenthesis of function call prohibited
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
  9 | ERROR | [x] Expected 0 spaces before closing bracket; 1 found
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)
 11 | ERROR | [x] Space after opening parenthesis of function call prohibited
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
 11 | ERROR | [x] Expected 0 spaces before closing bracket; 1 found
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)

If I repeat this without the --ignore-annotations, I:

• Correctly see the errors for line 6 and 11.
• INcorrectly still see errors on line 9, while those should not be there.

[gsherwood]

Tests are mostly looking good, though I found one weird bug.

I totally missed that case (selectively re-enable a sniff, then disable it again) but the latest commit fixes that. Thanks so much for testing.

[jrfnl]

@gsherwood Thank you. I'll try and run some more tests later.

[jrfnl]

@gsherwood Ok, so I think I've found another one. 😕

// phpcs:disable PEAR.Functions
bar( $foo, true );
// phpcs:enable PEAR.Functions.FunctionCallSignature
bar( $foo, true );
// phpcs:enable PEAR.Functions.ValidDefaultValue
bar( $foo, true );
// phpcs:enable PEAR
bar( $foo, true );

When running PHPCS over this snippet using phpcs -p -s ./test.php --standard=PEAR --ignore-annotations, I see the following errors (ignoring the comment related errors):

  4 | ERROR | [x] Space after opening parenthesis of function call prohibited
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
  4 | ERROR | [x] Expected 0 spaces before closing bracket; 1 found
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)
  6 | ERROR | [x] Space after opening parenthesis of function call prohibited
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
  6 | ERROR | [x] Expected 0 spaces before closing bracket; 1 found
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)
  8 | ERROR | [x] Space after opening parenthesis of function call prohibited
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
  8 | ERROR | [x] Expected 0 spaces before closing bracket; 1 found
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)
 10 | ERROR | [x] Space after opening parenthesis of function call prohibited
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
 10 | ERROR | [x] Expected 0 spaces before closing bracket; 1 found
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)

If I repeat this without the --ignore-annotations, I:

• Correctly see the errors for line 6 and 10.
• INcorrectly see no errors on line 8, while those should be there.

[gsherwood]

Ok, so I think I've found another one. 😕

Yep, I figured out what I stuffed up and have fixed that one up too. Thanks for testing (again)! 😄

[jrfnl]

Thanks Greg. I'm going to leave it for a bit now as my imagination has run out of possible tests I can run.
Fingers crossed I won't run into any other exceptions and no further bug reports come in about this ;-)

[gsherwood]

I'm going to leave it for a bit now as my imagination has run out of possible tests I can run.

Thanks again. I'll close as I think all the major cases are covered.