[PhpUnitBridge] Add type hints #26994

greg0ire · 2018-04-20T22:34:25Z

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no (only changed classes marked as internal)
Deprecations?	no
Tests pass?	yes
Fixed tickets	#26931
License	MIT
Doc PR	n/a

This makes classes inheriting from phpunit classes compatible with phpunit 7.

greg0ire · 2018-04-20T22:43:07Z

When trying to run simple-phpunit, I now get autoloading issues that I also get with phpunit 6:

SYMFONY_PHPUNIT_VERSION=7.0 php vendor/bin/simple-phpunit --bootstrap vendor/autoload.php src/Symfony/Component/Asset/Tests/PackageTest.php
PHPUnit 7.0.3 by Sebastian Bergmann and contributors.

Testing Symfony\Component\Asset\Tests\PackageTest
EEEEEEEEEEEE                                                      12 / 12 (100%)

Time: 19 ms, Memory: 4.00MB

There were 12 errors:

1) Symfony\Component\Asset\Tests\PackageTest::testGetUrl with data set #0 ('v1', '', 'http://example.com/foo', 'http://example.com/foo')
Error: Class 'Symfony\Component\Asset\Package' not found

/home/greg/dev/symfony/src/Symfony/Component/Asset/Tests/PackageTest.php:26

2) Symfony\Component\Asset\Tests\PackageTest::testGetUrl with data set #1 ('v1', '', 'https://example.com/foo', 'https://example.com/foo')
Error: Class 'Symfony\Component\Asset\Package' not found

/home/greg/dev/symfony/src/Symfony/Component/Asset/Tests/PackageTest.php:26

3) Symfony\Component\Asset\Tests\PackageTest::testGetUrl with data set #2 ('v1', '', '//example.com/foo', '//example.com/foo')
Error: Class 'Symfony\Component\Asset\Package' not found

/home/greg/dev/symfony/src/Symfony/Component/Asset/Tests/PackageTest.php:26

4) Symfony\Component\Asset\Tests\PackageTest::testGetUrl with data set #3 ('v1', '', '/foo', '/foo?v1')
Error: Class 'Symfony\Component\Asset\Package' not found

/home/greg/dev/symfony/src/Symfony/Component/Asset/Tests/PackageTest.php:26

5) Symfony\Component\Asset\Tests\PackageTest::testGetUrl with data set #4 ('v1', '', 'foo', 'foo?v1')
Error: Class 'Symfony\Component\Asset\Package' not found

/home/greg/dev/symfony/src/Symfony/Component/Asset/Tests/PackageTest.php:26

6) Symfony\Component\Asset\Tests\PackageTest::testGetUrl with data set #5 (null, '', '/foo', '/foo')
Error: Class 'Symfony\Component\Asset\Package' not found

/home/greg/dev/symfony/src/Symfony/Component/Asset/Tests/PackageTest.php:26

7) Symfony\Component\Asset\Tests\PackageTest::testGetUrl with data set #6 (null, '', 'foo', 'foo')
Error: Class 'Symfony\Component\Asset\Package' not found

/home/greg/dev/symfony/src/Symfony/Component/Asset/Tests/PackageTest.php:26

8) Symfony\Component\Asset\Tests\PackageTest::testGetUrl with data set #7 ('v1', 'version-%2$s/%1$s', '/foo', '/version-v1/foo')
Error: Class 'Symfony\Component\Asset\Package' not found

/home/greg/dev/symfony/src/Symfony/Component/Asset/Tests/PackageTest.php:26

9) Symfony\Component\Asset\Tests\PackageTest::testGetUrl with data set #8 ('v1', 'version-%2$s/%1$s', 'foo', 'version-v1/foo')
Error: Class 'Symfony\Component\Asset\Package' not found

/home/greg/dev/symfony/src/Symfony/Component/Asset/Tests/PackageTest.php:26

10) Symfony\Component\Asset\Tests\PackageTest::testGetUrl with data set #9 ('v1', 'version-%2$s/%1$s', 'foo/', 'version-v1/foo/')
Error: Class 'Symfony\Component\Asset\Package' not found

/home/greg/dev/symfony/src/Symfony/Component/Asset/Tests/PackageTest.php:26

11) Symfony\Component\Asset\Tests\PackageTest::testGetUrl with data set #10 ('v1', 'version-%2$s/%1$s', '/foo/', '/version-v1/foo/')
Error: Class 'Symfony\Component\Asset\Package' not found

/home/greg/dev/symfony/src/Symfony/Component/Asset/Tests/PackageTest.php:26

12) Symfony\Component\Asset\Tests\PackageTest::testGetVersion
Error: Class 'Symfony\Component\Asset\Package' not found

/home/greg/dev/symfony/src/Symfony/Component/Asset/Tests/PackageTest.php:52

Not quite sure why this happens but I think I might have fixed the bug :P

greg0ire · 2018-04-20T22:57:42Z

Other, bigger issue: "The PHPUnit\Framework\BaseTestListener class has been removed (deprecated in PHPUnit 6.4)"

greg0ire · 2018-04-20T23:07:53Z

Ah nevermind that has been handled already it seems.

greg0ire · 2018-04-20T23:10:21Z

@juliendufresne @geoff-maddock can you please test this patch on whatever project you were having this issue, and review this PR?

juliendufresne · 2018-04-20T23:23:50Z

I did exactly what you did before posting the issue and ran with another more complex issue.
Something like class A does not have the method B.

I'll try again to get the right output.

juliendufresne · 2018-04-20T23:32:59Z

PHP Fatal error:  Uncaught Error: Call to undefined method PHPUnit\Util\Configuration::getExtensionConfiguration() in /app/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:908
Stack trace:
#0 /app/vendor/symfony/phpunit-bridge/TextUI/TestRunner.php(34): PHPUnit\TextUI\TestRunner->handleConfiguration(Array)
#1 /app/vendor/phpunit/phpunit/src/TextUI/TestRunner.php(151): Symfony\Bridge\PhpUnit\TextUI\TestRunner->handleConfiguration(Array)
#2 /app/vendor/bin/.phpunit/phpunit-7.0/src/TextUI/Command.php(205): PHPUnit\TextUI\TestRunner->doRun(Object(PHPUnit\Framework\TestSuite), Array, true)
#3 /app/vendor/bin/.phpunit/phpunit-7.0/src/TextUI/Command.php(153): PHPUnit\TextUI\Command->run(Array, true)
#4 /app/vendor/bin/.phpunit/phpunit-7.0/phpunit(17): PHPUnit\TextUI\Command::main()
#5 /app/vendor/symfony/phpunit-bridge/bin/simple-phpunit(251): include('/app/vendor/bin...')
#6 {main}
  thrown in /app/vendor/phpunit/phpunit/src/TextUI/TestRunner.php on line 908

Fatal error: Uncaught Error: Call to undefined method PHPUnit\Util\Configuration::getExtensionConfiguration() in /app/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:908
Stack trace:
#0 /app/vendor/symfony/phpunit-bridge/TextUI/TestRunner.php(34): PHPUnit\TextUI\TestRunner->handleConfiguration(Array)
#1 /app/vendor/phpunit/phpunit/src/TextUI/TestRunner.php(151): Symfony\Bridge\PhpUnit\TextUI\TestRunner->handleConfiguration(Array)
#2 /app/vendor/bin/.phpunit/phpunit-7.0/src/TextUI/Command.php(205): PHPUnit\TextUI\TestRunner->doRun(Object(PHPUnit\Framework\TestSuite), Array, true)
#3 /app/vendor/bin/.phpunit/phpunit-7.0/src/TextUI/Command.php(153): PHPUnit\TextUI\Command->run(Array, true)
#4 /app/vendor/bin/.phpunit/phpunit-7.0/phpunit(17): PHPUnit\TextUI\Command::main()
#5 /app/vendor/symfony/phpunit-bridge/bin/simple-phpunit(251): include('/app/vendor/bin...')
#6 {main}
  thrown in /app/vendor/phpunit/phpunit/src/TextUI/TestRunner.php on line 908

EDIT: this is the output of one call meaning that I have two times the same error.

EDIT2: ok I'm guessing it's not getting the right phpunit project because this one is located in vendor/phpunit/phpunit instead of vendor/bin/.phpunit/phpunit-7.0

greg0ire · 2018-04-20T23:37:36Z

Looks like you have phpunit installed on your project: /app/vendor/phpunit/phpunit/src/TextUI/TestRunner.php. Try removing it, it probably interferes in a weird way.

greg0ire · 2018-04-20T23:38:53Z

Here is the commit that introduces this method: sebastianbergmann/phpunit@41c7584

juliendufresne · 2018-04-20T23:38:56Z

Just saw this at the same time :)
It's because of another dependency (phpstan-phpunit) :/

greg0ire · 2018-04-20T23:39:38Z

Yeah that's why we should stop using phpstan as a vendor like this :P

greg0ire · 2018-04-20T23:42:54Z

Argh, as you said it's phpstan-phpunit not sure you can run that in isolation from the project :/

juliendufresne · 2018-04-20T23:42:55Z

Removing the dependency that introduced phpunit works. Using phpstan standalone (outside of the project) is another debate :)

greg0ire · 2018-04-20T23:43:35Z

Argh, as you said it's phpstan-phpunit not sure you can run that in isolation from the project :/

Ah nevermind, it's totally doable: https://hub.docker.com/r/phpstan/phpstan/

juliendufresne · 2018-04-20T23:49:15Z

It's so sick I've not found that it uses the wrong phpunit earlier because I could have done the exact same PR months ago ^^

juliendufresne · 2018-04-21T01:23:05Z

I still don't understand why it loaded the class PHPUnit\TextUI\TestRunner from the wrong directory (/app/vendor/phpunit instead of /app/vendor/bin/.phpunit/phpunit-7.0/src/TextUI/TestRunner.php)

It should use its own autoloading located in /app/vendor/bin/.phpunit/phpunit-7.0/vendor/autoload.php

What I found so far is that if I force the autoloading from the vendor/bin/.phpunit/phpunit-7.0/phpunit using this code:

$reflector = new ReflectionClass('PHPUnit\TextUI\TestRunner');

It works. (you need to put this code in the simple-phunit file on line 123 so it will regenerate the phpunit file.)

juliendufresne · 2018-04-21T09:15:36Z

I figured it out.
simple-phpunit uses the composer autoload file located in vendor/bin/.phpunit/phpunit-7.0/vendor/autoload.php
But when phpunit tries to load a class in the project (in the tests folder), it uses the file specified in the phpunit.xml.dist: bootstrap="vendor/autoload.php".
The later overrides the former and every unknown files will then uses the <project-root-dir>vendor/autolad.php file.

That's why we can not have both phpunit-bridge and phpunit as a dependency in the project.
Sadly, I don't think there is any trick to remove phpunit from the classmap of <project-root-dir>/vendor/composer/autoload_classmap.php at runtime.

nicolas-grekas · 2018-04-21T09:16:13Z

Actually, the bridge should still be compatible with PHP 5.3, so we might have to use the same approach as we have for SymfonyTestsListener.php...

greg0ire · 2018-04-21T09:18:06Z

Ah right, I didn't check the right composer.json, and assumed the constraints would be the same

symfony/src/Symfony/Bridge/PhpUnit/composer.json

Lines 19 to 21 in 833909b

    
           "php": ">=5.3.3 EVEN ON LATEST SYMFONY VERSIONS TO ALLOW USING", 
        
           "php": "THIS BRIDGE WHEN TESTING LOWEST SYMFONY VERSIONS.", 
        
           "php": ">=5.3.3"

greg0ire · 2018-04-21T09:24:56Z

src/Symfony/Bridge/PhpUnit/TextUI/Command.php

@@ -26,7 +27,7 @@ class Command extends BaseCommand
        /**
         * {@inheritdoc}
         */
-        protected function createRunner()
+        protected function createRunner(): BaseRunner


@nicolas-grekas wait, aren't we already guaranteed to have php 7.0 here? We are inside a conditional block where phpunit is version 6 or above, ergo php itself must be 7 or above, correct? Or does the class_exists('PHPUnit_Runner_Version') condition part have other implications?

PHP 5.3 will still raise a parse error...

Ah but 5.3 needs to be able to parse this I think.

juliendufresne · 2018-04-21T10:13:05Z

src/Symfony/Bridge/PhpUnit/TextUI/TestRunner.php

@@ -11,45 +11,14 @@

 namespace Symfony\Bridge\PhpUnit\TextUI;

-use PHPUnit\TextUI\TestRunner as BaseRunner;
-use Symfony\Bridge\PhpUnit\SymfonyTestsListener;
-
 if (class_exists('PHPUnit_Runner_Version') && version_compare(\PHPUnit_Runner_Version::id(), '6.0.0', '<')) {
    class_alias('Symfony\Bridge\PhpUnit\Legacy\TestRunner', 'Symfony\Bridge\PhpUnit\TextUI\TestRunner');


Shouldn't we rename this one TestRunnerForV5?

Maybe yeah, @nicolas-grekas please advise

let's rename to ForV4 yes

juliendufresne · 2018-04-21T10:14:24Z

src/Symfony/Bridge/PhpUnit/TextUI/TestRunner.php

-            if (!$registeredLocally) {
-                $arguments['listeners'][] = $listener;
-            }
+    class_alias('Symfony\Bridge\PhpUnit\Legacy\TestRunnerForV6', 'Symfony\Bridge\PhpUnit\TextUI\TestRunner');


I would have create a TestRunnerForV7 as well.

The TestRunnerForV6 should be the same as the previous defined class (i.e without return typehint) and the TestRunnerForV7 with the return typehint

I'd rather have fewer files since adding the type hints even if they are not required is possible, but maybe the naming could be improved *ForV6AndAbove, *ForV5AndBelow?

I don't know the Symfony standards regarding naming. I'll let others answer :)

No need for the suffix: let use the lowest number

juliendufresne · 2018-04-21T10:19:26Z

src/Symfony/Bridge/PhpUnit/Legacy/SymfonyTestsListener.php

@@ -0,0 +1,59 @@
+<?php


Why do you need to define this class? It is already defined in the Symfony\Bridge\PhpUnit namespace

Oh sorry, didn't mean to do this, I copied too many files from my sandbox directory.

juliendufresne · 2018-04-21T10:20:02Z

src/Symfony/Bridge/PhpUnit/TextUI/Command.php

-     * @internal
-     */
-    class Command extends BaseCommand
+    class_alias('Symfony\Bridge\PhpUnit\Legacy\CommandForV6', 'Symfony\Bridge\PhpUnit\TextUI\Command');


I would have create CommandForV7 as well

fabpot · 2018-04-22T05:49:34Z

Why is it for 4.0 and not 2.7?

greg0ire · 2018-04-22T07:34:42Z

The bug was reported for 4.0, let's see if it applies to lower versions.

greg0ire · 2018-04-22T07:42:45Z

The 2.7 code looks a lot different: there is no TestRunner class or Command::handleConfiguration, there are no class aliases.

I can make a separate PR just for this method signature:

symfony/src/Symfony/Bridge/PhpUnit/TextUI/Command.php

Line 22 in 9234171

protected function handleBootstrap($filename)

greg0ire · 2018-04-22T07:51:13Z

The Legacy directory appears on the 3.3 branch, but 3.3 is not supported, so I think the right target for this bugfix is 3.4. I can write other PR 2.7 and 2.8 if you deem it necessary, but I guess that if Legacy is missing from those branches, it means that you didn't think supporting phpunit 6 was necessary (might also mean that 2.7 and 2.8 were unaffected by the phpunit 6 bc-breaks, I don't know). Please tell me what you think, but in the meantime, I am going to rebase on 3.4.

greg0ire · 2018-04-22T07:53:37Z

Apparently, support for phpunit 7 is a feature and was therefore added on 3.4

greg0ire · 2018-04-22T08:28:38Z

The Appveyor and Travis build failures seem unrelated.

greg0ire · 2018-04-22T09:23:54Z

The adopted naming convention is to use the lowest supported version for naming. Since we are not 100% sure about phpunit 4 support, we use V5, and if it works for phpunit 4, that's great but we don't really care.

This makes classes inheriting from phpunit classes compatible with phpunit 7. Fixes symfony#26931

nicolas-grekas · 2018-04-22T09:29:57Z

Thank you @greg0ire.

This PR was merged into the 3.4 branch. Discussion ---------- [PhpUnitBridge] Add type hints | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no (only changed classes marked as internal) | Deprecations? | no | Tests pass? | yes | Fixed tickets | #26931 | License | MIT | Doc PR | n/a This makes classes inheriting from phpunit classes compatible with phpunit 7. Commits ------- 6fbcc51 Add type hints

* 3.4: (22 commits) [appveyor] use PHP 7.1 to run composer [HttpKernel] Don't clean legacy containers that are still loaded [VarDumper] Fix HtmlDumper classes match Make the simple auth provider the same as in Symfony 2.7. [PhpUnitBridge] silence wget fix merge [Security] guardAuthenticationProvider::authenticate cannot return null according to interface specification [PhpUnitBridge] Fix #26994 [VarDumper] Remove decoration from actual output in tests [PropertyInfo] Minor cleanup and perf improvement [Bridge/Doctrine] fix count() notice on PHP 7.2 [Security] Skip user checks if not implementing UserInterface [DI] Add check of internal type to ContainerBuilder::getReflectionClass [HttpFoundation] Add HTTP_EARLY_HINTS const [DoctrineBridge] Improve exception message at `IdReader::getIdValue()` Add type hints fixed CS Use new PHP7.2 functions in hasColorSupport [VarDumper] Fix dumping of SplObjectStorage [HttpFoundation] Add functional tests for Response::sendHeaders() ...

* 4.0: (22 commits) [appveyor] use PHP 7.1 to run composer [HttpKernel] Don't clean legacy containers that are still loaded [VarDumper] Fix HtmlDumper classes match Make the simple auth provider the same as in Symfony 2.7. [PhpUnitBridge] silence wget fix merge [Security] guardAuthenticationProvider::authenticate cannot return null according to interface specification [PhpUnitBridge] Fix #26994 [VarDumper] Remove decoration from actual output in tests [PropertyInfo] Minor cleanup and perf improvement [Bridge/Doctrine] fix count() notice on PHP 7.2 [Security] Skip user checks if not implementing UserInterface [DI] Add check of internal type to ContainerBuilder::getReflectionClass [HttpFoundation] Add HTTP_EARLY_HINTS const [DoctrineBridge] Improve exception message at `IdReader::getIdValue()` Add type hints fixed CS Use new PHP7.2 functions in hasColorSupport [VarDumper] Fix dumping of SplObjectStorage [HttpFoundation] Add functional tests for Response::sendHeaders() ...

carsonbot added Status: Needs Review Bug labels Apr 20, 2018

greg0ire force-pushed the add_phpunit_type_hints branch from 2403702 to 74f4773 Compare April 20, 2018 22:35

greg0ire commented Apr 21, 2018

View reviewed changes

greg0ire force-pushed the add_phpunit_type_hints branch 2 times, most recently from 31adcfb to 855c112 Compare April 21, 2018 09:54

juliendufresne reviewed Apr 21, 2018

View reviewed changes

greg0ire force-pushed the add_phpunit_type_hints branch from 855c112 to 3206bdb Compare April 21, 2018 10:25

juliendufresne approved these changes Apr 21, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 21, 2018

nicolas-grekas approved these changes Apr 21, 2018

View reviewed changes

nicolas-grekas added this to the 2.7 milestone Apr 22, 2018

nicolas-grekas changed the title ~~Add type hints~~ [PhpUnitBridge] Add type hints Apr 22, 2018

greg0ire force-pushed the add_phpunit_type_hints branch from 3206bdb to 3db0323 Compare April 22, 2018 07:52

greg0ire changed the base branch from 4.0 to 3.4 April 22, 2018 07:52

nicolas-grekas modified the milestones: 2.7, 3.4 Apr 22, 2018

nicolas-grekas approved these changes Apr 22, 2018

View reviewed changes

juliendufresne approved these changes Apr 22, 2018

View reviewed changes

Add type hints

6fbcc51

This makes classes inheriting from phpunit classes compatible with phpunit 7. Fixes symfony#26931

greg0ire force-pushed the add_phpunit_type_hints branch from 182e4b8 to 6fbcc51 Compare April 22, 2018 09:29

nicolas-grekas merged commit 6fbcc51 into symfony:3.4 Apr 22, 2018

greg0ire deleted the add_phpunit_type_hints branch April 22, 2018 09:31

nicolas-grekas added a commit that referenced this pull request Apr 25, 2018

[PhpUnitBridge] Fix #26994

e9cc947

This was referenced Apr 30, 2018

Release v3.4.9 #27102

Merged

Release v4.0.9 #27103

Merged

[PhpUnitBridge] Add type hints #26994

[PhpUnitBridge] Add type hints #26994

Conversation

greg0ire commented Apr 20, 2018 • edited Loading

greg0ire commented Apr 20, 2018

greg0ire commented Apr 20, 2018 • edited Loading

greg0ire commented Apr 20, 2018

greg0ire commented Apr 20, 2018

juliendufresne commented Apr 20, 2018

juliendufresne commented Apr 20, 2018 • edited Loading

greg0ire commented Apr 20, 2018

greg0ire commented Apr 20, 2018

juliendufresne commented Apr 20, 2018

greg0ire commented Apr 20, 2018

greg0ire commented Apr 20, 2018

juliendufresne commented Apr 20, 2018

greg0ire commented Apr 20, 2018 • edited Loading

juliendufresne commented Apr 20, 2018 • edited Loading

juliendufresne commented Apr 21, 2018

juliendufresne commented Apr 21, 2018

nicolas-grekas commented Apr 21, 2018 • edited Loading

greg0ire commented Apr 21, 2018

greg0ire Apr 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Apr 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Apr 22, 2018

greg0ire commented Apr 22, 2018

greg0ire commented Apr 22, 2018

greg0ire commented Apr 22, 2018

greg0ire commented Apr 22, 2018

greg0ire commented Apr 22, 2018

greg0ire commented Apr 22, 2018 • edited Loading

nicolas-grekas commented Apr 22, 2018

greg0ire commented Apr 20, 2018 •

edited

Loading

greg0ire commented Apr 20, 2018 •

edited

Loading

juliendufresne commented Apr 20, 2018 •

edited

Loading

greg0ire commented Apr 20, 2018 •

edited

Loading

juliendufresne commented Apr 20, 2018 •

edited

Loading

nicolas-grekas commented Apr 21, 2018 •

edited

Loading

greg0ire Apr 21, 2018 •

edited

Loading

greg0ire Apr 21, 2018 •

edited

Loading

greg0ire commented Apr 22, 2018 •

edited

Loading