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

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

Closed
1 task done
dawidurbanski opened this issue Sep 3, 2023 · 9 comments

Comments

@dawidurbanski
Copy link
Contributor

dawidurbanski commented Sep 3, 2023

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?

  • I have verified the issue still exists in the develop branch of WordPressCS.
@jrfnl
Copy link
Member

jrfnl commented Sep 3, 2023

@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):

  • Make sure the Composer PHPCS installer plugin has permission to run:
    composer config allow-plugins.dealerdirect/phpcodesniffer-composer-installer true
  • Remove everything that is not your dependency, i.e. squizlabs/php_codesniffer, phpcsstandards/phpcsextra, phpcsstandards/phpcsutils. Unless you are a PHPCS standards developer, I don't understand why you would have those in your composer.json. Those are dependencies of WordPressCS, not of your package. Having those as explicit requires in your package will only set you up for version conflicts.
  • Either use the WordPress standard or use WordPress-Extra + WordPress-Docs.
    As per the README, the WordPress ruleset already contains WordPress-Extra + WordPress-Docs, so you are now just trying to load everything twice. (should still not cause a problem as PHPCS handles that, but there is no need for it and it will slow down the runtime).

@dawidurbanski
Copy link
Contributor Author

dawidurbanski commented Sep 3, 2023

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
Copy link
Contributor Author

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:

  • Create all defaults settings site in Local
  • Open site shell
  • Clone the reproduction repository
  • Run composer install
  • Run vendor/bin/phpcs src

@jrfnl
Copy link
Member

jrfnl commented Sep 3, 2023

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
Copy link
Contributor Author

Thanks! I appreciate your help.

I consider this to be solved then.

@GaryJones
Copy link
Member

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
Copy link
Contributor Author

dawidurbanski commented Sep 3, 2023

@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:

#2376

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

@jrfnl
Copy link
Member

jrfnl commented Sep 3, 2023

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
Copy link
Member

jrfnl commented Sep 3, 2023

@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.

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

3 participants