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:enable can sometimes override phpcs:ignore #111

Open
3 tasks done
anomiex opened this issue Dec 1, 2023 · 2 comments · May be fixed by #123
Open
3 tasks done

phpcs:enable can sometimes override phpcs:ignore #111

anomiex opened this issue Dec 1, 2023 · 2 comments · May be fixed by #123

Comments

@anomiex
Copy link
Contributor

anomiex commented Dec 1, 2023

Duplicated from squizlabs/PHP_CodeSniffer#3889 (and updated from later comments)


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
<?php

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

Custom ruleset

N/A

To reproduce

Steps to reproduce the behavior:

  1. Create a files called test1.php and test2.php with the code samples above.
  2. Run phpcs -s --standard=PSR12 test1.php test2.php
  3. See error message displayed
FILE: /tmp/test/test1.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
---------------------------------------------------------------------------------------------------------------


FILE: /tmp/test/test2.php
--------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------------------------------------
 6 | ERROR | [x] TRUE, FALSE and NULL must be lowercase; expected "true" but found "TRUE"
   |       |     (Generic.PHP.LowerCaseConstant.Found)
--------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------------------------------------------------------

Expected behavior

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

The error on test2.php line 6 should have been ignored due to the specific rule having been disabled.

Versions (please complete the following information)

Operating System Debian sid
PHP version 8.2.12
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.
@anomiex
Copy link
Contributor Author

anomiex commented Dec 1, 2023

Hmm, I can't create the PR corresponding to squizlabs/PHP_CodeSniffer#3891 yet, either I'll have to wait for GitHub to respond to my support request for "rerouting" my fork or I'll have to create a new fork.

@jrfnl
Copy link
Member

jrfnl commented Dec 1, 2023

@anomiex True, though you should be able to then just add the new fork + the new repo as extra remotes to your local git copy, which will allow you to push the existing branch up to the new fork.
I had GH "de-fork" the new repo to prevent new PRs still going to this repo, which gets pretty annoying very quickly.

@jrfnl jrfnl added this to the 4.0.0 milestone Dec 1, 2023
anomiex added a commit to anomiex/PHP_CodeSniffer that referenced this issue Dec 4, 2023
The current method, listing codes to disable and a list of exceptions to
that list, still has trouble with some cases. For example, disabling a
standard, re-enabling a category within that standard, then ignoring or
disabling a sniff within that category cannot be handled. We'd need a
list of exceptions to the exceptions, and possibly a list of exceptions
to that list too, and figuring out how to keep those lists up to date as
new directives are encountered could prove to be confusing.

Since the standard→category→sniff→code hierarchy is supposed to be
thought of as a tree, let's store the ignore list that way instead.
Manipulating the branches of the tree is straightforward no matter what
directives are encountered.

In this implementation I've favored speed over space: there are cases
where we could prune a subtree that would evaluate to "ignore" or "don't
ignore" for any possible input, but detecting that doesn't seem worth
the time when it's not likely there will be so many enable or disable
directives that the wasted space will be a problem.

Fixes PHPCSStandards#111
@anomiex anomiex linked a pull request Dec 4, 2023 that will close this issue
11 tasks
anomiex added a commit to anomiex/PHP_CodeSniffer that referenced this issue Dec 10, 2023
The current method, listing codes to disable and a list of exceptions to
that list, still has trouble with some cases. For example, disabling a
standard, re-enabling a category within that standard, then ignoring or
disabling a sniff within that category cannot be handled. We'd need a
list of exceptions to the exceptions, and possibly a list of exceptions
to that list too, and figuring out how to keep those lists up to date as
new directives are encountered could prove to be confusing.

Since the standard→category→sniff→code hierarchy is supposed to be
thought of as a tree, let's store the ignore list that way instead.
Manipulating the branches of the tree is straightforward no matter what
directives are encountered.

In this implementation I've favored speed over space: there are cases
where we could prune a subtree that would evaluate to "ignore" or "don't
ignore" for any possible input, but detecting that doesn't seem worth
the time when it's not likely there will be so many enable or disable
directives that the wasted space will be a problem.

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

Successfully merging a pull request may close this issue.

2 participants