Cannot redeclare WordPress\Sniffs\Files\FilenameSniff

GaryJones commented 6 years ago

I'm trying to implement https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#custom-unit-test-classes.

With a custom test class, I've got the following in my .phpcs.xml.dist:

<rule ref="WordPress"/>
<rule ref="WordPress.Files.Filename">
    <properties>
        <property name="custom_test_class_whitelist" type="array" value="WP_Import_UnitTestCase"/>
    </properties>
</rule>

I get the following fatal error:

PHP Fatal error:  Cannot declare class WordPress\Sniffs\Files\FileNameSniff, because the name is already in use in /Users/gary/Local Sites/wordpress-core/app/public/src/wp-content/plugins/wordpress-importer/vendor/wp-coding-standards/wpcs/WordPress/Sniffs/Files/FilenameSniff.php on line 221
PHP Stack trace:
PHP   1. {main}() /Users/gary/Local Sites/wordpress-core/app/public/src/wp-content/plugins/wordpress-importer/vendor/squizlabs/php_codesniffer/bin/phpcs:0
PHP   2. PHP_CodeSniffer\Runner->runPHPCS() /Users/gary/Local Sites/wordpress-core/app/public/src/wp-content/plugins/wordpress-importer/vendor/squizlabs/php_codesniffer/bin/phpcs:18
PHP   3. PHP_CodeSniffer\Runner->init() /Users/gary/Local Sites/wordpress-core/app/public/src/wp-content/plugins/wordpress-importer/vendor/squizlabs/php_codesniffer/src/Runner.php:70
PHP   4. PHP_CodeSniffer\Ruleset->__construct() /Users/gary/Local Sites/wordpress-core/app/public/src/wp-content/plugins/wordpress-importer/vendor/squizlabs/php_codesniffer/src/Runner.php:294
PHP   5. PHP_CodeSniffer\Ruleset->registerSniffs() /Users/gary/Local Sites/wordpress-core/app/public/src/wp-content/plugins/wordpress-importer/vendor/squizlabs/php_codesniffer/src/Ruleset.php:217
PHP   6. PHP_CodeSniffer\Autoload::loadFile() /Users/gary/Local Sites/wordpress-core/app/public/src/wp-content/plugins/wordpress-importer/vendor/squizlabs/php_codesniffer/src/Ruleset.php:1103

With the second rule commented out, I get no fatal error (but do get the Error regarding the name of test file for the class that extends WP_Import_UnitTestCase that I'm trying to silence).

If I comment out the first rule, I don't get a fatal error, but still do get the Error violation.

Direct require-dev dependencies:

dealerdirect/phpcodesniffer-composer-installer v0.4.4
phpunit/phpunit                                5.7.27
squizlabs/php_codesniffer                      3.2.3
wimg/php-compatibility                         8.1.0
wp-coding-standards/wpcs                       0.14.1

WPCS dev-develop also gives the same situation.

jrfnl commented 6 years ago

@GaryJones I've just tried to reproduce the issue and am not having any luck.

Based on the Composer info you provided, I've done a test run against the master of WP Importer using PHPCS 3.2.3, WPCS 0.14.1 and PHPCompatibility 8.1.0 using the following ruleset and am not getting any PHP errors (but lots of CS violations are being reported):

<?xml version="1.0"?>
<ruleset name="Test1322">

    <file>.</file>

    <arg name="extensions" value="php"/>
    <arg value="sp"/>

    <rule ref="WordPress"/>

    <rule ref="WordPress.Files.Filename">
        <properties>
            <property name="custom_test_class_whitelist" type="array" value="WP_Import_UnitTestCase"/>
        </properties>
    </rule>

    <config name="testVersion" value="5.2-"/>
    <rule ref="PHPCompatibility"/>
</ruleset>

I've also searched all three standards for additional class_exists() calls which might suffer the same issue as we saw in the PrefixAllGlobals sniff, but haven't found any problems there either.

What PHP version are you using ? Not that it should matter, but I might as well try and reproduce with the same version.
Can you push your WIP branch ? I've cloned the WP Importer GH repo + your fork.

GaryJones commented 6 years ago

PHP 7.1.7.

Once I'd done the last file (see https://github.com/WordPress/wordpress-importer/pull/31), then the fatal error seems to sort itself out. With the second rule commented out, the Error violation wasn't reported (WP_Import_UnitTestCase extends WP_UnitTestCase, so I think my custom_test_class_whitelist wasn't needed anyway). If I un-commented the second rule, then the violation appears.

Bizarre, but I like these self-resolving bugs :-)

jrfnl commented 6 years ago

Hmm, still can't reproduce it when I run it against the unclean master branch, even when just and only using the snippet you gave above as the ruleset (wrapped in ruleset tags).

(WP_Import_UnitTestCase extends WP_UnitTestCase, so I think my custom_test_class_whitelist wasn't needed anyway).

It is. The sniff does not keep track of which sniffs extend WP_UnitTestCase, it uses the custom property to examine the extends part of the class declaration and if it it not one of the whitelisted classes, it will (should) report it.

As an alternative to the property, you could just exclude the tests directory from the InvalidClassFileName errorcode - but I think that should trigger the same error:

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

Bizarre, but I like these self-resolving bugs :-)

I don't... still feels like there must be something wrong, just waiting for a way to be able reproduce it.

jrfnl commented 6 years ago

FYI: I've opened PR squizlabs/PHP_CodeSniffer#1923 upstream which will hopefully solve this and similar issues.

WordPress / WordPress-Coding-Standards

Cannot redeclare WordPress\Sniffs\Files\FilenameSniff #1322