Php_codesniffer: Request : Issue with the 'TestCase' class alias in bootstrap file

[El-Sam]

Hi, first of all, thanks for this library, it's of a great help.

I'm using PHPUnit 6 for my unit tests.

In the bootstrap.php file in PHP_CodeSniffer/tests , a class alias is created, this alias causes Multiple definitions exist for class TestCase issue in my IDE (PHPStorm).

This breaks a lot of features that speed up my testing like code completion. So, I think you should remove this alias or handle it in another way that you find convenient.

[gsherwood]

I might need a bit more information. Is this just an issue with your IDE thinking that multiple class definitions exist when they actually don't at runtime? If so, isn't this an issue with your IDE and not an issue with PHPCS?

The reason the class alias is there is so that the unit tests will run with both old and new versions of PHPUnit, so it can't just be removed. Do you have a suggestion for another way to achieve PHPUnit backwards compatibility?

[orlangur]

Caused by a hacky https://github.com/squizlabs/PHP_CodeSniffer/pull/1384/files.

+if (class_exists('PHPUnit_Framework_TestCase') === true && class_exists('PHPUnit\Framework\TestCase') === false) {
 +    class_alias('PHPUnit_Framework_TestCase', 'PHPUnit\Framework\TestCase');
 +}

So, PhpStorm ignores this condition?

https://stackoverflow.com/a/23079082/8640867 doesn't help? Even if PHPUnit_Framework_TestCase does not exist which is the case for PHPUnit 6 PhpStorm is complaining?

[orlangur]

The reason the class alias is there is so that the unit tests will run with both old and new versions of PHPUnit, so it can't just be removed. Do you have a suggestion for another way to achieve PHPUnit backwards compatibility?

@gsherwood sorry if I missed a previous explanation somewhere, but why PHP_CodeSniffer needs to explicitly support unsupported PHPUnit versions?

I remember you explained reasoning behind supporting unsupported PHP versions but PHPUnit is just a tool to test PHP_CodeSniffer itself, it was tested just fine without such a hack https://travis-ci.org/squizlabs/PHP_CodeSniffer/jobs/254679990. Why not just let Composer to install whatever PHPUnit version is suitable for the particular platform?

[gsherwood]

PHPUnit is just a tool to test PHP_CodeSniffer itself

No, it's not. Developers of custom coding standards want to be able to write unit tests for their sniffs, which I (of course) encourage. If they are already using newer versions of PHPUnit for their projects because they don't need to support older PHP versions, the ability for the PHPUnit tests to be run on newer versions of PHPUnit would be a great benefit to them.

[jrfnl]

I missed a previous explanation somewhere, but why PHP_CodeSniffer needs to explicitly support unsupported PHPUnit versions?

The explanation is quite simple:

• PHP Codesniffer 3.x has a minimum PHP requirement of PHP 5.4.
• The PHPUnit versions which are still actively supported are:
  
  
  
  • PHPUnit 6.x which has a minimum PHP requirement of PHP 7.0
  
  
  • PHPUnit 5.7 which has a minimum PHP requirement of PHP 5.6. Support for PHPUnit 5.7 will end February 2018.
  
  

To be able to run the PHPCS unit tests on PHP 5.4 up to nightly, it is therefore necessary that the PHPCS unit tests are compatible with PHPUnit 4.x, 5.x and 6.x.

Using the Composer based interim solution would lock PHPCS into a PHPUnit version which will lose support by February 2018 (and an unsupported version for PHP 5.4/5.5) and is therefore not a sustainable solution.

[orlangur]

@gsherwood thanks for the explanation, I was scratching my head trying to understand the use case, as with necessity to support PHPUnit 4 modern PHPUnit 6 features cannot be used anyway. The fact some projects may drop some PHP versions didn't came to my mind.

@jrfnl yeah, this is quite clear knowing the fact PHP 5.4 requirement is unavoidable, just that I don't think all three PHPUnit versions have to be supported for internal testsuite, it's enough to have it runnable for _some_ PHPUnit version on _every_ platform.

[gmponos]

Hello.. this case happens with other libraries as well. I think it happened for me with twig.

What I do is having this the tests folder marked as excludes. It is also mentioned at stackoverflow at the link posted above.

Btw @gsherwood is there any reason why the tests are included when the lib is installed with composer? I didn't see the tests folder in .gitattributes file.

[El-Sam]

as @orlangur mentionned, it's this lines of code that are missing with the PHPStorm :

// Compatibility for PHPUnit < 6 and PHPUnit 6+.
if (class_exists('PHPUnit_Framework_TestSuite') === true && class_exists('PHPUnit\Framework\TestSuite') === false) {
    class_alias('PHPUnit_Framework_TestSuite', 'PHPUnit\Framework\TestSuite');
}

if (class_exists('PHPUnit_Framework_TestCase') === true && class_exists('PHPUnit\Framework\TestCase') === false) {
    class_alias('PHPUnit_Framework_TestCase', 'PHPUnit\Framework\TestCase');
}

I support @gmponos suggestion to add your tests folder to the .gitattributes so it would not be fetched with the lib.
I don't need your tests in my production.

[kubawerlos]

@El-Sam it was removed from .gitattributes not long ago after discussion.

What about bumping minimum PHPUnit version to 4.8? This one has forward compatibility classes, so we wouldn't need the lines in bootstrap.php that are messing with PHPStorm.

Or am I missing something?

[michalbundyra]

@El-Sam
In PHPUnit 4.8 we have only couple forward compatibility classes:
https://github.com/sebastianbergmann/phpunit/tree/4.8/src/ForwardCompatibility
and it's not enough for PHP_CodeSniffer:
https://github.com/squizlabs/PHP_CodeSniffer/blob/master/tests/bootstrap.php#L31-L46

So there is no PHPUnit\TextUI\TestRunner and PHPUnit\Framework\TestResult...

[orlangur]

At least it would allow to slightly reduce the compatibility layer, unless somebody is sitting on some PHPUnit 4.1 and needs this constraint unchanged. If such improvement useful or not is more a matter of long-term strategy.

[El-Sam]

Do you mean that I should use PHPUnit v4.8 instead of v6? if that's the case, I can't, my whole team uses v6 and I can't just switch to v4.8.

I'm sorry if I'm missing something.

[orlangur]

No-no, I mean some projects with custom standards may still need PHPUnit 4.lo (an opposite case to https://github.com/squizlabs/PHP_CodeSniffer/issues/1671#issuecomment-331000656 when they want newest PHPUnit).

[El-Sam]

Ah ok, thanks for explaining.

So, I guess adding the tests folder to .getattributes will not happen.

Any other solutions? all the PHPStorm tricks mentioned above and on StackOverflow end up disabling features in the IDE that are essentials for coding.

[orlangur]

Sorry, I missed the point why you cannot exclude PHP_CodeSniffer/tests in your PhpStorm?

[El-Sam]

Because:

1. PHPStorm by default excludes the vendor (recursively all the files and folders) of my project, so the PHP_CodeSniffer is also excluded and everything inside of it too, but I still get the Multiple definitions exist for class TestCase message.

2. A workaround to solve the issue above is to prevent squizlabs/php_codesniffer from being excluded, and then exclude the PHP_CodeSniffer/tests. But the issue here is that I frequently delete my vendor and run the compose install to clean my vendor and repeating this process every time is kinda a waste of my time.

[orlangur]

Clearing vendor folder is unusual scenario but anyway... Looks like https://intellij-support.jetbrains.com/hc/en-us/community/posts/207069905/comments/207466459 should do the trick, please check.

[El-Sam]

I did play with excluding and some other stuff with the help that you link you provided to see how the TestCase behave.
Results:

• Excluding the squizlabs/php_codesniffer doesn't do anything.
• The only thing that worked is to remove the squizlabs/php_codesniffer entry from Settings | Languages & Frameworks | PHP --> Include paths, BUT when I restarted PHPStorm, the squizlabs/php_codesniffer entry was back.

[eclipxe13]

I would suggest to reconsider removing the test from the library (include it in .gitattributes). I don't need your tests in my production.

They were added by issue #548 but I think that is an edge case (not the common way we use this library as a development dependence) and I think that can be easily solved in the edge case by adding this to its composer.json project:

{
    "config": {
        "preferred-install": {
            "squizlabs/php_codesniffer": "source",
            "*": "dist"
        }        
    }
}

For PHPStorm users that require squizlabs/php_codesniffer as a dependence: mark as plain text vendor/squizlabs/php_codesniffer/tests/bootstrap.php https://blog.jetbrains.com/phpstorm/2011/04/exclude-single-file/

PS: We may need some tool or composer extension to install / update / remove development utils like squizlabs/php_codesniffer, jakub-onderka/php-parallel-lint, friendsofphp/php-cs-fixer to use them but don't mess with the library dependences (build dependences, not code dependences)

[PatrickRose]

I would suggest to reconsider removing the test from the library (include it in .gitattributes). I don't need your tests in my production.

You don't need PHPCS in your production, so that's irrelevant...

[svycka]

Maybe as a workaround we could change

class_alias('PHPUnit_Framework_TestCase', 'PHPUnit\Framework\TestCase');

to something like

class_alias('PHPUnit_Framework_TestCase', 'PHPUnit'.'\Framework\TestCase');

this should fix phpStorm problem and alias would still work?

[neomerx]

Please remove/change this hacky and annoying implementation.

Why it is still a question if polluting global namespace is acceptable.

[gsherwood]

Awaiting feedback from PR #1686

[gsherwood]

A PR has been merged that looks to work around this issue. Is anyone able to check out the latest master and confirm if this specific PHPStorm issue has been resolved?

[El-Sam]

I tried the latest master, I confirm I no longer get the Multiple definitions exist for class TestCase issue in PHPStorm.

Thank you @svycka for the fix.

[gsherwood]

@El-Sam Thanks for testing that. That's two confirmations now, so I'll close this issue off.

[kamazee]

This creates another issue in PHPStorm, which will likely be fixed soon, and when it is, this workaround will stop working.

[artspb]

The fix is not coming anytime soon so the workaround is fine. Suggestions how the problem with multiple definitions can be addressed on the IDE side are awaited here.