WordPress / WordPress-Coding-Standards

PHP_CodeSniffer rules (sniffs) to enforce WordPress coding conventions
MIT License
2.56k stars 487 forks source link

WordPressCS 3.0: Fatal error: Cannot declare class WordPressCS\WordPress\Sniffs\Files\FileNameSniff #2375

Closed dawidurbanski closed 1 year ago

dawidurbanski commented 1 year ago

Bug Description

Minimal Code Snippet

I created a repository with minimal reproduction.

You can test this using develop branch using use-develop branch on the repro repository.

Error Code

Fatal error: Cannot declare class WordPressCS\WordPress\Sniffs\Files\FileNameSniff, because the name is already in use in /Users/xxx/Local Sites/xxx/app/public/wp-content/plugins/phpcs3.0-custom-rule-repro/vendor/wp-coding-standards/wpcs/WordPress/Sniffs/Files/FilenameSniff.php on line 36

Custom Ruleset

<?xml version="1.0"?>
<ruleset name="Test Config">
    <file>.</file>
    <arg name="extensions" value="php"/>
    <rule ref="WordPress"/>
    <rule ref="WordPress-Extra"/>
    <rule ref="WordPress-Docs"/>

    <rule ref="WordPress.Files.Filename">
        <exclude-pattern>/src/*\.php</exclude-pattern>
    </rule>
</ruleset>

Environment

Question Answer
PHP version PHP 8.1.9 (cli) (built: Aug 30 2022 10:24:25) (NTS)
PHP_CodeSniffer version PHP_CodeSniffer version 3.7.2 (stable) by Squiz (http://www.squiz.net)
WordPressCS version 3.0.0
PHPCSUtils version 1.0.8
PHPCSExtra version 1.1.1
WordPressCS install type Composer project local
IDE (if relevant) Not relevant

Tested Against develop Branch?

jrfnl commented 1 year ago

@dawidurbanski I have cloned that repo and tried with both the main branch as well as the use-develop branch and cannot reproduce the error.

I do see some other things which aren't as they should be in your repo, but that still shouldn't cause this issue.

As a side-note, now WordPressCS 3.0 has been released, why would you still want to use dev-develop ?

Advise for improving your setup (also largely mentioned in the upgrade guide):

dawidurbanski commented 1 year ago

Thanks for the feedback @jrfnl!

As a side-note, now WordPressCS 3.0 has been released, why would you still want to use dev-develop ?

I'm not using develop branch. I only added this to satisfy:

Tested Against develop Branch? [x] I have verified the issue still exists in the develop branch of WordPressCS.


Remove everything that is not your dependency

and

Either use the WordPress standard or use WordPress-Extra + WordPress-Docs

I cleaned the repo a bit, now using only WordPress standard and just one single dev dependency ("wp-coding-standards/wpcs": "^3.0").

Unfortunately I still see the issue.


Make sure the Composer PHPCS installer plugin has permission to run: composer config allow-plugins.dealerdirect/phpcodesniffer-composer-installer true

The environment I test this on is Local by Flywheel, by using the "Open site shell" option.

The thing is, it uses composer v 2.1.5:

➜  phpcs3.0-custom-rule-repro git:(main) ✗ composer --version                                                                    
Composer version 2.1.5 2021-07-23 10:35:47

This version does not have allow-plugins option at all yet (this was added in composer 2.2).

In theory before composer 2.2, it should allow all plugins by default, so I should not be required to run this allow-plugins config thing.

Is it maybe the case that WPCS3.0 does not support Composer < 2.2?

If that's the case I'm fine with that :) We may just update the docs to have that info.

EDIT: and then I should move this to Local by Flywheel forums to push them to update the bundled composer version

dawidurbanski commented 1 year ago

Also, I created this super short (1:45), full steps reproduction screencast to shed some more light:

https://www.youtube.com/watch?v=XMzgDL9gS3U

Summary:

jrfnl commented 1 year ago

Aside from the plugin needing permission in Composer >= 2.2, there is nothing Composer specific and the plugin itself supports both Composer 1.x as well as 2.x, so Composer 2.1.5 is supported.

Having said that, I tested with Composer 2.1.5 just to be sure and again couldn't reproduce the issue.

I don't have Local by Flywheel installed, so can't test with that, but other than that, I have replicated your complete setup with Composer 2.1.5, PHP 8.1.9 and cannot reproduce the issue.

I'd like to suggest you try on your native OS and if things work correctly there, then I suggest you report this to Local by Flywheel as that would then be the "variable" which is causing the problem.

dawidurbanski commented 1 year ago

Thanks! I appreciate your help.

I consider this to be solved then.

GaryJones commented 1 year ago

It's a case-sensitivity issue.

I was able to reproduce locally (using LocalWP), but couldn't reproduce when excluding WordPress.PHP.IniSet.

I then noticed that the sniff file name is called FileName (capital N), but your test config file was excluding Filename (lowercase n).

Once I fixed it, I got no error. I was then able to try excluding WordPress.PHP.Iniset (incorrect case) and was able to trigger a similar error.

Fatal error: Cannot declare class WordPressCS\WordPress\Sniffs\PHP\IniSetSniff, because the name is already in use in /Users/gary/Sites/wpcs-2375/app/public/phpcs3.0-custom-rule-repro/vendor/wp-coding-standards/wpcs/WordPress/Sniffs/PHP/InisetSniff.php on line 0

With -vv flag on the wrong case, I see:

Processing rule "WordPress.PHP.IniSet"
                        => /Users/gary/Sites/wpcs-2375/app/public/phpcs3.0-custom-rule-repro/vendor/wp-coding-standards/wpcs/WordPress/Sniffs/PHP/IniSetSniff.php
...
Processing rule "WordPress.PHP.Iniset"
        => /Users/gary/Sites/wpcs-2375/app/public/phpcs3.0-custom-rule-repro/vendor/wp-coding-standards/wpcs/WordPress/Sniffs/PHP/InisetSniff.php
        => added rule-specific absolute ignore pattern: /src/*\.php

So, by using the wrong case for the exclusion, you're actually trying to (first) include the sniff, before later excluding it for the src/*.php files. That initial inclusion seemingly gets normalized/resolved to the same sniff file eventually, but it doesn't match the rule name that maybe the file is stored under, so it tries twice, hence the error. Possibly something for PHPCS to fix.

dawidurbanski commented 1 year ago

@GaryJones WooHoo! That was it!

In case someone lands here from Google, I you took this from the phpcs.xml.dist.sample from the "using custom ruleset" part of the docs.

@GaryJones @jrfnl I created PR to fix this:

https://github.com/WordPress/WordPress-Coding-Standards/pull/2376

Thanks again for all your help. I appreciate it a lot.

jrfnl commented 1 year ago

Oh cricky... that explains it! Thanks for figuring this out @GaryJones and thanks @dawidurbanski for the PR to fix the sample ruleset! It also explains why I wasn't seeing the issue as I'm on Windows, where the file system is case-insensitive.

As a rule of thumb, arguments on the CLI are treated case-insensitively in PHPCS (well, mostly, not completely), while rulesets are treated case-sensitively. I've had a discussion about this before and at the time, Greg indicated that he was thinking of making the ruleset case-insensitive as well for PHPCS 4.x, but as far as I know, there is no active work being done on that at this time.

However, this is not a straight-forward problem to solve as the autoloader still needs to find the file if it's the first mention in a ruleset and for case-sensitive file systems (*nix, Mac), that means it needs the correct case. For subsequent mentions (like the problem described here), it should be solvable though.

@GaryJones @dingo-d Should we maybe add a task to the CI to do a minimal test run with the sample ruleset (which is allowed to fail ?) to prevent this kind of typo (re-)entering the example ruleset in the future ?

jrfnl commented 1 year ago

@GaryJones I've been trying to set up a test case for PHPCS itself for this (with the intention to at least make a second include for the same sniff using a different case less error prone), but as I'm on Windows, I'm struggling to create a working test case.

Could you share the -vvv output for the situation with the fatal error with me ? Might help me better identify where in the flow the real problem is.