Make it easy for Drupal to use the PHPUnit bridge #24685

alexpott · 2017-10-24T19:55:13Z

Q	A
Branch?	3.3
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#...
License	MIT
Doc PR	symfony/symfony-docs#...

Drupal doesn't use src/Symfony/Bridge/PhpUnit/bin/simple-phpunit but would like to use the deprecation collection features of src/Symfony/Bridge/PhpUnit. The checks in src/Symfony/Bridge/PhpUnit/bootstrap.php mean that this is difficult because they rely on simple-phpunit - but I'm not sure that that is necessary.

nicolas-grekas · 2017-10-25T07:55:15Z

src/Symfony/Component/BrowserKit/Client.php

@@ -342,8 +342,9 @@ public function request($method, $uri, array $parameters = array(), array $files
    protected function doRequestInProcess($request)
    {
        $deprecationsFile = tempnam(sys_get_temp_dir(), 'deprec');
-        putenv('SYMFONY_DEPRECATIONS_SERIALIZE='.$deprecationsFile);
-        $process = new PhpProcess($this->getScript($request), null, null);
+        $environment = getenv();


this doesn't work before PHP 7.1
but I don't get what it changes in fact, so maybe it's not required at all?

What this was trying to do was only set SYMFONY_DEPRECATIONS_SERIALIZE in the sub process so we didn't have to unset it after making the request.

why not just relying on the feature of the Process component inheriting the env variables ?

nicolas-grekas · 2017-10-25T07:55:58Z

I'm not sure I understand the technical purpose of this PR :)

alexpott · 2017-10-25T08:42:59Z

The technical purpose of this PR is to make PHPUnit Bridge's bootstrap.php not rely on assumptions about the environment. The problem is that we're doing

// We define the COMPOSER_INSTALL constant, so that PHPUnit knows where to
// autoload from. This is needed for tests run in isolation mode, because
// phpunit.xml.dist is located in a non-default directory relative to the
// PHPUnit executable.
if (!defined('PHPUNIT_COMPOSER_INSTALL')) {
  define('PHPUNIT_COMPOSER_INSTALL', __DIR__ . '/../../autoload.php');
}

in our test bootstrap - so the assumption made by
if (!defined('PHPUNIT_COMPOSER_INSTALL') && !class_exists('PHPUnit_TextUI_Command', false) && !class_exists('PHPUnit\TextUI\Command', false)) {
is not correct for us.

stof · 2017-10-25T08:56:27Z

src/Symfony/Bridge/PhpUnit/bootstrap.php

@@ -12,11 +12,9 @@
 use Doctrine\Common\Annotations\AnnotationRegistry;
 use Symfony\Bridge\PhpUnit\DeprecationErrorHandler;

-// Detect if we're loaded by an actual run of phpunit
-if (!defined('PHPUNIT_COMPOSER_INSTALL') && !class_exists('PHPUnit_TextUI_Command', false) && !class_exists('PHPUnit\TextUI\Command', false)) {


removing this is broken. The logic will now register the bridge error handler even when the bootstrap.php file was not loaded during a composer run but during a normal usage of the app using the composer autoloader.

stof · 2017-10-25T08:58:08Z

@alexpott are you actually defining PHPUNIT_COMPOSER_INSTALL even when not running PHPUnit ?

alexpott · 2017-10-25T09:32:43Z

@stof nope we're not defining when not running PHPUnit - we define it when running PHPUnit and inside an isolated test. This is because in PHPUnit's src/Util/PHP/Template/TestCaseMethod.tpl.dist it does

$configurationFilePath = '{configurationFilePath}';

if ('' !== $configurationFilePath) {
    $configuration = PHPUnit\Util\Configuration::getInstance($configurationFilePath);
    $configuration->handlePHPConfiguration();
    unset($configuration);
}

which needs to be able to autoload PHPUnit\Util\Configuration for drupal because we use a non-standard configurationFilePath. See https://www.drupal.org/node/2597814 for a bit more info.

…cations

alexpott · 2017-10-25T10:56:24Z

I've updated the PR to only remove the check against PHPUNIT_COMPOSER_INSTALL since this constant is necessary for PHPUnit isolated tests where you are using a custom configuration file. I've tested this approach with Drupal and it works great. I've also tested Symfony locally with 5.5, 5.6 and 7.1 and it passes.

alexpott · 2017-10-25T14:38:32Z

I'm just testing this with Drupal and I've found something more odd going on. Still investigating. :(

nicolas-grekas · 2017-10-25T14:41:38Z

Thank you @alexpott.

…e (alexpott)" This reverts commit 41509a2, reversing changes made to 3e3e74c.

* 3.3: Revert "minor #24685 Make it easy for Drupal to use the PHPUnit bridge (alexpott)"

* 3.4: Revert "minor #24685 Make it easy for Drupal to use the PHPUnit bridge (alexpott)"

nicolas-grekas · 2017-10-25T16:57:18Z

Reverted as it breaks something on our CI.

1) Symfony\Component\Process\Tests\ProcessTest::testThatProcessDoesNotThrowWarningDuringRun
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Test Error'
+'The Symfony\Component\Process\Process::setEnhanceSigchildCompatibility() method is deprecated since version 3.3 and will be removed in 4.0. Sigchild compatibility will always be enabled.'

/home/nicolas/Code/symfony/src/Symfony/Component/Process/Tests/ProcessTest.php:100

This can be reproduced locally by applying this patch:

--- a/src/Symfony/Component/Process/Tests/ProcessTest.php
+++ b/src/Symfony/Component/Process/Tests/ProcessTest.php
@@ -1583,7 +1583,7 @@ EOTXT;
             try {
                 $process->setEnhanceSigchildCompatibility(false);
                 $process->getExitCode();
-                $this->fail('ENHANCE_SIGCHLD must be used together with a sigchild-enabled PHP.');
+ //               $this->fail('ENHANCE_SIGCHLD must be used together with a sigchild-enabled PHP.');
             } catch (RuntimeException $e) {
                 $this->assertSame('This PHP has been compiled with --enable-sigchild. You must use setEnhanceSigchildCompatibility() to use this method.', $e->getMessage());
                 if ($enhance) {

then running
SYMFONY_DEPRECATIONS_HELPER=weak ENHANCE_SIGCHLD=0 ./phpunit src/Symfony/Component/Process/

carsonbot added the Status: Needs Review label Oct 24, 2017

nicolas-grekas reviewed Oct 25, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 25, 2017

carsonbot added Status: Needs Work and removed Status: Reviewed labels Oct 25, 2017

stof requested changes Oct 25, 2017

View reviewed changes

nicolas-grekas added this to the 3.3 milestone Oct 25, 2017

alexpott added 3 commits October 25, 2017 11:34

Only use environment variable to determine if we should collect depre…

0ea8b12

…cations

Coding standards

6523039

Remove only PHPUNIT_COMPOSER_INSTALL check

be56b35

alexpott force-pushed the allow-drupal-to-use-bridge branch from 145bcd1 to be56b35 Compare October 25, 2017 10:48

alexpott changed the base branch from master to 3.3 October 25, 2017 10:48

Revert other changes since they are no longer necessary

b0bcc20

Fix tests

553edff

nicolas-grekas approved these changes Oct 25, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Oct 25, 2017

nicolas-grekas closed this in 41509a2 Oct 25, 2017

nicolas-grekas added a commit that referenced this pull request Oct 25, 2017

Revert "minor #24685 Make it easy for Drupal to use the PHPUnit bridg…

ea3a9a9

…e (alexpott)" This reverts commit 41509a2, reversing changes made to 3e3e74c.

nicolas-grekas added a commit that referenced this pull request Oct 25, 2017

Merge branch '3.3' into 3.4

78426b7

* 3.3: Revert "minor #24685 Make it easy for Drupal to use the PHPUnit bridge (alexpott)"

nicolas-grekas added a commit that referenced this pull request Oct 25, 2017

Merge branch '3.4'

64bed1b

* 3.4: Revert "minor #24685 Make it easy for Drupal to use the PHPUnit bridge (alexpott)"

alexpott mentioned this pull request Oct 26, 2017

Ensure DeprecationErrorHandler::collectDeprecations() is triggered #24696

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make it easy for Drupal to use the PHPUnit bridge #24685

Make it easy for Drupal to use the PHPUnit bridge #24685

alexpott commented Oct 24, 2017 •

edited by nicolas-grekas

Loading

nicolas-grekas Oct 25, 2017

alexpott Oct 25, 2017

stof Oct 25, 2017

nicolas-grekas commented Oct 25, 2017

alexpott commented Oct 25, 2017

stof Oct 25, 2017

stof commented Oct 25, 2017

alexpott commented Oct 25, 2017

alexpott commented Oct 25, 2017

alexpott commented Oct 25, 2017

nicolas-grekas commented Oct 25, 2017

nicolas-grekas commented Oct 25, 2017

Make it easy for Drupal to use the PHPUnit bridge #24685

Make it easy for Drupal to use the PHPUnit bridge #24685

Conversation

alexpott commented Oct 24, 2017 • edited by nicolas-grekas Loading

nicolas-grekas Oct 25, 2017

Choose a reason for hiding this comment

alexpott Oct 25, 2017

Choose a reason for hiding this comment

stof Oct 25, 2017

Choose a reason for hiding this comment

nicolas-grekas commented Oct 25, 2017

alexpott commented Oct 25, 2017

stof Oct 25, 2017

Choose a reason for hiding this comment

stof commented Oct 25, 2017

alexpott commented Oct 25, 2017

alexpott commented Oct 25, 2017

alexpott commented Oct 25, 2017

nicolas-grekas commented Oct 25, 2017

nicolas-grekas commented Oct 25, 2017

alexpott commented Oct 24, 2017 •

edited by nicolas-grekas

Loading