Php_codesniffer: Autoloading of sniff fails when multiple classes declared in same file

[aik099]

Describe the bug
When a sniff PHP file contains multiple PHP classes in it (e.g. sniff class itself and a helper class), then \PHP_CodeSniffer\Autoload::loadFile method called in \PHP_CodeSniffer\Ruleset::registerSniffs method returns the last declared class in that file, which might not be a sniff class at all. As a result that class is treated like a sniff class and nothing good happens.

In my case, that class was with mandatory constructor arguments and PHP_CodeSniffer tried to initiate it like a sniff (without constructor arguments) and I've got a Fatal Error.

Code sample

class TypeCommentStructure
{
    public function __construct(File $phpcsFile, $stackPtr)
    {
    }
}

To reproduce
Steps to reproduce the behavior:

1. add an above-added class at the bottom of any sniff class containing file
2. run the ruleset, that contains a sniff that was just added
3. See error message displayed

PHP Fatal error:  Uncaught ArgumentCountError: Too few arguments to function CodingStandard\Sniffs\Commenting\TypeCommentStructure::__construct(), 0 passed in /mnt/UserData/home/user/.config/composer/vendor/squizlabs/php_codesniffer/src/Ruleset.php on line 1209 and exactly 2 expected in /mnt/UserData/home/user/project/vendor/aik099/coding-standard/CodingStandard/Sniffs/Commenting/TypeCommentSniff.php:608
      Stack trace:
      #0 /mnt/UserData/home/user/.config/composer/vendor/squizlabs/php_codesniffer/src/Ruleset.php(1209): CodingStandard\Sniffs\Commenting\TypeCommentStructure->__construct()
      #1 /mnt/UserData/home/user/.config/composer/vendor/squizlabs/php_codesniffer/src/Ruleset.php(218): PHP_CodeSniffer\Ruleset->populateTokenListeners()
      #2 /mnt/UserData/home/user/.config/composer/vendor/squizlabs/php_codesniffer/src/Runner.php(332): PHP_CodeSniffer\Ruleset->__construct(Object(PHP_CodeSniffer\Config))
      #3 /mnt/UserData/home/user/.config/composer/vendor/squizlabs/php_codesniffer/src/Runne... (182 more bytes) ...
(Run with `--trace` for a full exception trace.)

Expected behavior
A clear and concise description of what you expected to happen.

Versions (please complete the following information):

• OS: Linux
• PHP: 7.2
• PHPCS: 3.5.8
• Standard: any

Additional context
No issue at PHP_CodeSniffer 3.5.6 and below. The issue started happening after PHP_CodeSniffer 3.5.7 release.

[jrfnl]

Sounds related to #3130.

[aik099]

@jrfnl , I've tried reverting all changes from 2020 year, including this one, and still had the same issue (at least on PHP 7.2).

[jrfnl]

@aik099 Could you give me some insight into the directory structure your standard uses ? and where this file lives within that structure ?

[aik099]

@jrfnl , even better, here it's on the GitHub: https://github.com/aik099/CodingStandard . The sniff file in question is https://github.com/aik099/CodingStandard/blob/master/CodingStandard/Sniffs/Commenting/TypeCommentSniff.php, where I have several classes defined.

If you attempt to run tests (included in that repo) for that specific sniff (or all test suite) you'll see that test for it fail, because 2nd class (instead of a sniff class) is registered as a sniff and of course isn't able to find any issues in code to allow the test to pass.

[jrfnl]

Ah yes, so basically it comes down to the autoloader having an assumption that each file only contains one class. I'm surprised to hear you say that it worked in PHPCS 3.5.6 though, as the change I referred to above is the only change of any significance made to the autoloader since PHPCS 3.0.2.

I'd say that it should probably be made more clear that "one class per file" is a requirement in the tutorial.

As for your use-case, what about moving the second class to its own file in a CodingStandard/Helpers directory ? The PHPCS autoloader will handle loading classes anywhere within the CodingStandard directory without problems, but expects classes in the Sniffs directory to be, well, sniffs.

[aik099]

Sure, I can extract a class into a separate file. You can see yourself that it works on PHP_CodeSniffer 3.5.6 if you:

1. change composer.json file in a coding standard to use 3.5.6 version specifically (instead of latest available)
2. run a composer update command
3. run tests

Still sounds like a BC break to me, because it worked and stopped.

[theilig]

I'm seeing the same issue. Having multiple classes in a file worked in 3.5.6, broke in 3.5.8

Working on splitting them out

[gsherwood]

I think I might know what this is. The array_reverse call was removed as part of the PHP 7.4 fix because the order of loaded classes can no longer be relied upon. So while the issue with multiple classes in a single file was likely to be encountered by developers sometime in the future, the removal of this call has forced it to occur now.

If anyone is able to test this, could you change this line in autoload.php:
https://github.com/squizlabs/PHP_CodeSniffer/blob/7c01187e14b8e56f2bc358915a466c35b8f84eb8/autoload.php#L198
To be:

$newClasses = array_reverse(array_diff($classesAfterLoad['classes'], $classesBeforeLoad['classes']));

It works for me locally, but would be good to test with a failing standard as well.

[aik099]

@gsherwood , the fix proposed in https://github.com/squizlabs/PHP_CodeSniffer/issues/3145#issuecomment-718269703 worked for PHP < 7.4, but with that fix on PHP 7.4 it became broken.

[gsherwood]

@aik099 Thanks for testing. I think this is one of those PHP changes where there isn't a lot of ways around it - moving to a single class per file appears to be the only thing that's going to work 100% of the time.

If anyone else has any ideas, please let me know.

[aik099]

It's possible to detect PHP version and it's below PHP 7.4 add array_reverse call.

[gsherwood]

@aik099 Sounds like a good idea. Any chance you could try this code:

$newClasses = array_diff($classesAfterLoad['classes'], $classesBeforeLoad['classes']);
if (PHP_VERSION_ID < 70400) {
    $newClasses = array_reverse($newClasses);
}

[aik099]

Yes, this solution works correctly.

Maybe you can try adding a test case for this and Travis CI would happily ensure, that it keeps working as expected.

[gsherwood]

Have committed this change. It will be in 3.6.0. Thanks for testing.