phpcs:enable can sometimes override phpcs:ignore

[anomiex]

Describe the bug

phpcs:enable can sometimes wind up overriding a later phpcs:ignore for the rule. This can particularly happen when multiple phpcs:enable comments are present in the file.

Code sample

<?php

// phpcs:disable PSR12.Operators.OperatorSpacing.NoSpaceBefore
// phpcs:disable PSR12.Operators.OperatorSpacing.NoSpaceAfter
$x=1;
// phpcs:enable PSR12.Operators.OperatorSpacing.NoSpaceAfter
// phpcs:enable PSR12.Operators.OperatorSpacing.NoSpaceBefore

$x= 2; // phpcs:ignore PSR12.Operators.OperatorSpacing.NoSpaceBefore

Custom ruleset

N/A

To reproduce

Steps to reproduce the behavior:

1. Create a file called test.php with the code sample above.
2. Run phpcs -s --standard=PSR12 test.php
3. See error message displayed

FILE: /tmp/test/test.php
---------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------------------
 9 | ERROR | [x] Expected at least 1 space before "="; 0 found
   |       |     (PSR12.Operators.OperatorSpacing.NoSpaceBefore)
---------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------------------------------------

Expected behavior

The error on line 9 should have been ignored due to the phpcs:ignore comment on that line.

Versions (please complete the following information)

Operating System	Debian sid
PHP version	8.2.10
PHP_CodeSniffer version	3.7.2, master
Standard	PSR12
Install type	Composer local

Additional context

It seems that in this situation it's setting $ignoring['.except'][...], which overrides phpcs:ignore.

Please confirm:

• I have searched the issue list and am not opening a duplicate issue.
• I confirm that this bug is a bug in PHP_CodeSniffer and not in one of the external standards.
• I have verified the issue still exists in the master branch of PHP_CodeSniffer.

[jrfnl]

Thanks for reporting this @anomiex. Confirmed as reproducible.

I think the problem is in this bit of code, but reading it, gives me the impression this may have been done intentionally. This may need clarification. /cc @gsherwood

Tests can be added here: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/tests/Core/ErrorSuppressionTest.php

[anomiex]

I think the problem is in this bit of code, but reading it, gives me the impression this may have been done intentionally. This may need clarification.

I'm only guessing, but my feeling from looking at the code setting it is that the .except logic is intended to handle a situation like

// phpcs:disable SomeRuleSet
// phpcs:enable SomeRuleSet.ButKeep.ThisOne

Looks like the commit adding it refers back to #1986, which seems to support that.

[jrfnl]

@anomiex Yes, I did a lot of testing when that feature initially came out to straighten out the bugs. Even so, you've clearly found one I didn't think of at the time ;-)

Note: based on what we've identified, I think the patch + test should be reasonably straight forward, so I'm not working on this myself (as a patch from someone else can get merged a lot quicker than my patches).

[anomiex]

Once I started working on a patch, I found some more cases. These one in particular doesn't seem very simple to fix within the current structure:

// phpcs:disable Generic
// phpcs:enable Generic.PHP
// phpcs:disable Generic.PHP.LowerCaseConstant
$var = TRUE;

I wound up changing the format for the ignore list entirely (and then wrapping it in a class to hold the functions for manipulating it); I hope that doesn't turn out to be too much of a breaking change since PHP_CodeSniffer\Tokenizers\Tokenizer->ignoredLines happens to be public.

[jrfnl]

I'll need to have a good look at the patch, but yes, a bigger change like that does sound like something which would be more suitable for PHPCS 4.x instead of 3.x...

[jrfnl]

I wonder if a more targetted patch, which only addresses the immediate issue could be created for the 3.x branch ?