-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Autoloading of sniff fails when multiple classes declared in same file #3145
Comments
Sounds related to #3130. |
@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). |
@aik099 Could you give me some insight into the directory structure your standard uses ? and where this file lives within that structure ? |
@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. |
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 |
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:
Still sounds like a BC break to me, because it worked and stopped. |
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 |
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: Line 198 in 7c01187
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. |
@gsherwood , the fix proposed in #3145 (comment) worked for PHP < 7.4, but with that fix on PHP 7.4 it became broken. |
@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. |
It's possible to detect PHP version and it's below PHP 7.4 add |
@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);
} |
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. |
Have committed this change. It will be in 3.6.0. Thanks for testing. |
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
To reproduce
Steps to reproduce the behavior:
Expected behavior
A clear and concise description of what you expected to happen.
Versions (please complete the following information):
Additional context
No issue at PHP_CodeSniffer 3.5.6 and below. The issue started happening after PHP_CodeSniffer 3.5.7 release.
The text was updated successfully, but these errors were encountered: