8000 [PhpUnitBridge] not compatible with latest phpunit · Issue #21125 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content
8000

[PhpUnitBridge] not compatible with latest phpunit #21125

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.

Sign up for GitHub

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

Jump to bottom
Closed
garak opened this issue Jan 2, 2017 · 30 comments
Closed

[PhpUnitBridge] not compatible with latest phpunit #21125

garak opened this issue Jan 2, 2017 · 30 comments
Labels
Feature PhpUnitBridge
Milestone
3.3

Comments

@garak
Copy link
Contributor
garak commented Jan 2, 2017
Q A
Bug report? no
Feature request? yes
BC Break report? yes
RFC? yes
Symfony version 3.3.*-dev

I tried to use phpunit-bridge with latest phpunit (^6.0@dev, will be stable next month), it does not work due to changes in phpunit itself.

Error: Class 'PHPUnit_Util_ErrorHandler' not found.

Since phpunit (at last) switched to namespaces, the current class is PHPUnit\Util\ErrorHandler, not PHPUnit_Util_ErrorHandler any more.

I see that supporting this is a BC break, so maybe we could provide a compatibility layer to old phpunit?
@xabbuh xabbuh added this to the 3.3 milestone Jan 2, 2017
@Jean85
Copy link
Contributor
Jean85 commented Jan 2, 2017 •
edited
Loading

A class_alias declaration should be enough to avoid a BC

[EDIT] Also this is needed for every PHPUnit's class that used by the bridge.

@stof
Copy link
Member
stof commented Jan 2, 2017

It may be worth reporting this to PHPUnit to see whether they could include this class in their BC layer (they have BC classes for some classes)

@Jean85 Jean85 mentioned this issue Jan 2, 2017
Closed
@Jean85
Copy link
Contributor
Jean85 commented Jan 2, 2017 •
edited
Loading

I've opened a PR on PHPUnit: sebastianbergmann/phpunit#2414

@Jean85
Copy link
Contributor
Jean85 commented Jan 2, 2017

The BC layer PR has been refused. We need to try an other approach, like a way to declare a lot of class_alias.. I would like to work on that, any input to where should I put that code?

@nicolas-grekas
Copy link
Member

I don't think it's our job to alias classes in the phpunit namespace/prefix... We should try harder to find another solution that could be purely on our side.

@garak
Copy link
Contributor Author
garak commented Jan 2, 2017

My temporary solution, used with success in my project, was adding a classmap to my composer.json:

    "autoload-dev": {
        "psr-4": { "Tests\\": "tests/" },
        "classmap": [ "tests/phpunit-bc" ]
    },

and then putting classes like the ones proposed by @Jean85 in his PR.

@Jean85
Copy link
Contributor
Jean85 commented Jan 2, 2017

@nicolas-grekas what approach would you suggest? I skimmed through the bridge and it seems to me that the class_alias is the only option, due to the fact that various static methods are used, like

symfony/src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

Line 94 in 87ef723

|| in_array('legacy', \PHPUnit_Util_Test::getGroups($class, $method), true)

@nicolas-grekas
Copy link
Member

@Jean85 add some if in the code to make it able to deal with both situations?
8000

@Jean85
Copy link
Contributor
Jean85 commented Jan 2, 2017

I would find that way a bit harder to read in Symfony's codebase, and it would force us to do that everywhere, without being sure of having everything tested IMHO.

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Jan 2, 2017 •
edited
Loading

I agree, but the PhpUnit namespace is not our vendor, so me must not get there. An example of a nasty side effect of using class_alias: phpunit 6 would behave differently when doing class_exists('PHPUnit_Util_ErrorHandler') with or without the bridge.

@garak
Copy link
Contributor Author
garak commented Jan 2, 2017

The odd thing is that phpunit 5.7 has a forward compatibility layer, but it's not implementing every class. See https://github.com/sebastianbergmann/phpunit/tree/5.7/src/ForwardCompatibility

@Jean85
Copy link
Contributor
Jean85 commented Jan 2, 2017

I'm not sure what Sebastian is up to on that, the 6.0 has the same classes in reverse as a BC layer, but he shot down my PR...

@xabbuh
Copy link
Member
xabbuh commented Jan 2, 2017 •
edited
Loading

@Jean85 Maybe he would accept a similar PR für PHPUnit 5.7 to ensure forward compatibility. That would at least make more sense than providing backwards compatibility in a major version where you are actually able to do BC breaking changes. But note that this wouldn't still help us as we also need to support versions of PHPUnit prior to 5.7.

@garak
Copy link
Contributor Author
garak commented Jan 2, 2017 •
edited
Loading

@xabbuh do we really need it? Phpunit 4.8 will be out of support in one month.

@xabbuh
Copy link
Member
xabbuh commented Jan 2, 2017

@garak We need to be compatible with PHPUnit 4.8 as long as we have Symfony versions with active support that are compatible with PHP 5.5 or lower.

@Jean85
Copy link
Contributor
Jean85 commented Jan 3, 2017

The opposite solution would be lock the bridge at <6 and then release the v4 with ^6.0

@xabbuh
Copy link
Member
xabbuh commented Jan 3, 2017

I guess it shouldn't be too hard to make the bridge compatible with PHPUnit 6. I try to look into it tonight.

@stof
Copy link
Member
stof commented Jan 3, 2017

The bridge itself should be possible.

For the Symfony testsuite itself, it is not, until we can require PHPUnit 5.7+ to use the new namespaced TestCase class

@xabbuh
Copy link
Member
xabbuh commented Jan 3, 2017

Yes, but that's okay for the Symfony development as we can control which PHPUnit versions we want to use. PHPUnit 6 support in the bridge is nice though as people want to use PHPUnit 6 for their own projects/packages.

@Jean85
Copy link
Contributor
Jean85 commented Jan 3, 2017

The Bridge major version is NOT is sync with Symfony's, right?

So, IMHO the best solution is to migrate it completely to PHPUnit 6.0 in a 4.0 release, and leave the 3.x for PHPUnit 4.8 and 5.7.

@garak
Copy link
Contributor Author
garak commented Jan 3, 2017

I agree with @Jean85

@stof
Copy link
Member
stof commented Jan 3, 2017

@Jean85 Releases of the bridge are still synchronized with Symfony. The only difference is the PHP requirement, where we kept support for PHP 5.3 in the bridge even in 3.2, to allow us to benefit from the bridge new features even when running the testsuite for LTS branches.

Releasing a new major version of the bridge that Symfony itself cannot use is a no-go, as it would mean that Symfony would not receive new features anymore (unless we continue to add new features in 3.x after releasing 4.0, which does not fit our process).

Anyway, adding support for PHPUnit 6 in the bridge should be possible without a new major version IMO with some tricks.

@Jean85
Copy link
Contributor
Jean85 commented Jan 3, 2017

Thanks @stof for the clarification about the release process; I agree for the tricks, but:

  • class_alias, as pointed out by @nicolas-grekas could change the behavior of other pieces of code
  • putting ifs everywhere is feasible but dangerous, may lead to breakages
    Any other suggestions?

@Jean85
Copy link
Contributor
Jean85 commented Jan 3, 2017

While at lunch 😄 I got an other idea about a different approach to solve this issue:

  • Create classes inside the Symfony\Bridge\PhpUnit\Compat namespace with same name of PHPUnit 6 classes
    • Example: \PHPUnit_Util_ErrorHandler AKA PHPUnit\Util\ErrorHandler becomes Symfony\Bridge\PhpUnit\Compat\Util\ErrorHandler
  • The compat classes will just have the necessary dark magic to extend the right class (I was thinking about use statements wrapped inside an if (class_exist), feel free to suggest other approaches)
  • The compat classes will be already deprecated and dropped in 4.0
  • All usages of PHPUnit classes are replaced by those compat classes
    • Example: \PHPUnit_Util_ErrorHandler::handleError(...); is replaced by ErrorHandler::handleError(...); plus use Symfony\Bridge\PhpUnit\Compat\Util\ErrorHandler;

Advantages:

  • the changes to the code are simple and they are the same as to moving to PHPUnit 6, apart from the use statement
  • The git diff now will be minimal and understandable
  • The git diff later (4.0) will be even smaller, with the deletion of all the compat classes and just a lot of changes to the use statements: use Symfony\Bridge\PhpUnit\Compat\* to use PhpUnit\*

@Jean85 Jean85 mentioned this issue Jan 4, 2017
Closed
@Jean85
Copy link
Contributor
Jean85 commented Jan 4, 2017 •
edited
Loading

I've started a WIP PR (#21158) to try my approach, and the only doubt that I have is how to solve argument typehints; example:

public function addSkippedTest(\PHPUnit_Framework_Test $test, \Exception $e, $time)

The only approach that I found that would preserve BC is a bit verbose:

use PHPUnit\Framework\TestCase as TestCase6;

// ...

public function addSkippedTest($test, \Exception $e, $time)
{
    if ($test instanceof \PHPUnit_Framework_Test) {
        return $this->addSkippedTestCompat5($test, $e, $time);
    }
    
    return $this->addSkippedTestCompat6($test, $e, $time);
}

private function addSkippedTestCompat5(\PHPUnit_Framework_Test $test, \Exception $e, $time)
{
    return $this->doAddSkippedTest($test, $e, $time);
}

private function addSkippedTestCompat6(Test6 $test, \Exception $e, $time)
{
    return $this->doAddSkippedTest($test, $e, $time);
}

/**
 * @param \PHPUnit_Framework_Test | Test6 $test
 * @param \Exception $e
 * @param $time
 */
private function doAddSkippedTest($test, \Exception $e, $time)
{
   // previous code...
}

@Jean85 Jean85 mentioned this issue Jan 9, 2017
Closed
@piotrplenik
Copy link
piotrplenik commented Jan 10, 2017 •
edited
Loading

Hi guys,

small tip - I got same problem with PHP 5.6.29 + latest stable phpunit (v5.7.5):

Fatal error: Class 'PHPUnit_Util_ErrorHandler' not found in ./vendor/symfony/phpunit-bridge/DeprecationErrorHandler.php on line 73
THE ERROR HANDLER HAS CHANGED!
PHP Fatal error: Class 'PHPUnit_Util_ErrorHandler' not found in ./vendor/symfony/phpunit-bridge/DeprecationErrorHandler.php on line 73
Script php vendor/bin/phpunit handling the tests event returned with error code 255

Problem does not appear in PHP 7.0.9+

Resolved adding manually "files" in "autoload-dev" with entry:

    "autoload-dev": {
        "psr-4": { "Tests\\": "tests/" },
        "files": [ "vendor/phpunit/phpunit/src/Util/ErrorHandler.php" ]
    },

This change works for php5 and 7.

@garak
Copy link
Contributor Author
garak commented Feb 3, 2017

Just a quick update to notice that phpunit 6 is now stable.

@Jean85
Copy link
Contributor
Jean85 commented Feb 3, 2017

Unfortunately I haven't found a working solution for my PR #21221

@julienfalque julienfalque mentioned this issue Feb 7, 2017
Closed
@Tobion
Copy link
Contributor
Tobion commented Feb 7, 2017

\Symfony\Bundle\FrameworkBundle\Test\KernelTestCase would also need an alternative/fix for phpunit 6

@Jean85 Jean85 mentioned this issue Feb 20, 2017
Closed
@nicolas-grekas nicolas-grekas mentioned this issue Feb 21, 2017
Merged
@nicolas-grekas
Copy link
Member

See #21694

nicolas-grekas added a commit that referenced this issue Feb 21, 2017
@nicolas-grekas
feature #21694 [Bridge/PhpUnit] Add PHPUnit 6 support (nicolas-grekas)
6c0d5c9 
This PR was merged into the 3.3-dev branch.

Discussion
----------

[Bridge/PhpUnit] Add PHPUnit 6 support

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #21125
| License       | MIT
| Doc PR        | -

This PR makes our phpunit bridge compatible with all namespaced versions of phpunit, from 4.8.35 to 6.
It takes another approach than #21668 and #21221, thus replaces them.
Tested locally : tests pass when using phpunit 5.7, and fails with v6.0 because our own test suite is not yet compatible with it - but at least it runs nice.
If this were handled as usual Symfony component, we would consider some changes to be BC breaks. But in this specific case - a phpunit bridge - it makes no sense to me to apply the bc policy here. I added `@final` and `@internal` annotations to make this clearer.

Commits
-------

9e0745c [Bridge/PhpUnit] Add PHPUnit 6 support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Feature PhpUnitBridge
Projects
None yet
Milestone
3.3
Development

No branches or pull requests
7 participants
@garak @nicolas-grekas @stof @Tobion @piotrplenik @xabbuh @Jean85
0