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

Squiz.Arrays.ArrayDeclaration.NoComma adds invalid comma to certain match statements since 3.10.1 #747

Open
4 tasks done
janedbal opened this issue Nov 28, 2024 · 3 comments

Comments

@janedbal
Copy link

Describe the bug

Mentioned fixable sniff breaks the PHP code as follows:

Code sample

match ($foo) {
    'a' => [
        'x' => 0,
        'y' => 0,
    ],
    default => 0
};

Custom ruleset

<?xml version="1.0"?>
<ruleset>
    <rule ref="Squiz.Arrays.ArrayDeclaration.NoComma"/>
</ruleset>

To reproduce

Steps to reproduce the behavior:

  1. Create a file called test.php with the code sample above
  2. Create phpcs.xml with the custom ruleset above
  3. Run phpcbf test.php
  4. See fix applied
match ($foo) {
    'a' => [
        'x' => 0,
        'y' => 0,
    ,],
    default => 0
};

Expected behavior

No file changes.

Versions

Operating System ubuntu
PHP version PHP 8.3.13
PHP_CodeSniffer version 3.10.1
Standard Squiz
Install type composer

Additional context

Version 3.10.0 does not contains this issue.

Please confirm

  • I have searched the issue list and am not opening a duplicate issue.
  • I have read the Contribution Guidelines and this is not a support question.
  • 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
Copy link
Member

jrfnl commented Nov 28, 2024

@janedbal Thanks for reporting this.

The Squiz.Arrays.ArrayDeclaration sniff is known to be problematic and has been for years.

The problem is largely that as soon as something is excluded from that sniff, the sniff is broken. This is a design flaw in the sniff and applies to your situation.

I've been building up a (highly configurable) NormalizedArrays standard in PHPCSExtra to replace it, but that's not complete yet. Would be lovely if I could find some time to finish that at some point.

With that in mind, I will not be working on this issue.
If someone would submit a PR with a small/simple fix for this issue, I will accept it, but other than that, this sniff is out of bounds for large fixes/rewrites as it is just not worth the time.

@jrfnl
Copy link
Member

jrfnl commented Nov 30, 2024

@janedbal I've just checked, but if all you are using of the sniff is the Squiz.Arrays.ArrayDeclaration.NoComma error code, I suggest switching to the PHPCSExtra NormalizedArrays.Arrays.CommaAfterLast sniff, which already handles this correctly.

@janedbal
Copy link
Author

janedbal commented Dec 1, 2024

We were actually using more from that, this was just the most simple example to show the bug. But since it is buggy and wont be maintained, we will disable it completely. Thanks

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

No branches or pull requests

2 participants