Skip to content

[PhpUnitBridge] SYMFONY_DEPRECATIONS_HELPER and DeprecationErrorHandler #38239

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
shehi opened this issue Sep 18, 2020 · 5 comments
Closed

[PhpUnitBridge] SYMFONY_DEPRECATIONS_HELPER and DeprecationErrorHandler #38239

shehi opened this issue Sep 18, 2020 · 5 comments
Labels
Bug Help wanted Issues and PRs which are looking for volunteers to complete them. PhpUnitBridge Status: Needs Review

Comments

@shehi
Copy link
Contributor

shehi commented Sep 18, 2020 •
edited
Loading

Symfony version(s) affected: 5.1.5

Description
This line, where DeprecationErrorHandler is being registered, relies on the fact that a SYMFONY_DEPRECATIONS_HELPER env value exists in $_ENV. Problem is, this bootstrapping happens while inside Composer autoloader:

image

At this point, phpunit.xml file hasn't even been loaded, as a result, the aforementioned env value which is provided there isn't put into $_ENV yet. This technically invalidates the point of this line in the code if you don't provide the env value via command shell.

Once autoloader is loaded (at this point we are past the point of loading DeprecationErrorHandler) env values are loaded from phpunit.xml:

image

How to reproduce

  1. phpunit.xml file has line: <env name="SYMFONY_DEPRECATIONS_HELPER" value="disabled" />
  2. Create some test with deprecated usage triggerable.
  3. Run tests and you still will get THE ERROR HANDLER HAS CHANGED! error at the end of the test, which is emitted by DeprecationErrorHandler.
    image
  4. Run the same tests with env variable passed in command shell: SYMFONY_DEPRECATIONS_HELPER="disabled" phpunit and no errors will be emitted.
    image

And guys... All caps?! Seriously?!
@shehi shehi added the Bug label Sep 18, 2020
@shehi
Copy link
Contributor Author

shehi commented Sep 18, 2020

My phpunit.xml:

<?xml version="1.0" encoding="UTF-8"?>

<!-- https://phpunit.de/manual/current/en/appendixes.configuration.html -->
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:noNamespaceSchemaLocation="http://schema.phpunit.de/9.3/phpunit.xsd"
         colors="true"
         bootstrap="config/bootstrap.php"
         stopOnFailure="true"
>
    <php>
        <env name="SYMFONY_DEPRECATIONS_HELPER" value="disabled"/>
        <env name="KERNEL_CLASS" value="App\Kernel" />
        <ini name="error_reporting" value="32767" />
        <ini name="memory_limit" value="256M" />
        <server name="APP_ENV" value="test" force="true" />
        <server name="SHELL_VERBOSITY" value="-1" />
        <server name="SYMFONY_PHPUNIT_REMOVE" value="" />
        <server name="SYMFONY_PHPUNIT_VERSION" value="9.3" />
    </php>
    <testsuites>
        <testsuite name="Backend Model Tests">
            <directory>tests/Model/</directory>
        </testsuite>
        <testsuite name="Backend Functional Tests">
            <directory>tests/Functional/</directory>
        </testsuite>
        <testsuite name="Backend Legacy Functional Tests">
            <directory>tests/FunctionalLegacy/</directory>
        </testsuite>
        <testsuite name="Backend Unit Tests">
            <directory>tests/Unit/</directory>
        </testsuite>
    </testsuites>
    <listeners>
        <listener class="Symfony\Bridge\PhpUnit\SymfonyTestsListener" />
    </listeners>
    <logging>
        <junit outputFile="./.coverage/logfile.xml"/>
    </logging>
    <coverage processUncoveredFiles="true">
        <include>
            <directory suffix=".php">src/</directory>

        </include>
        <exclude>
            <directory suffix="Exception.php">src/</directory>
            <directory>src/Application/Dto/Constraints/</directory>
            <directory>src/Resources/DataFixtures/</directory>
        </exclude>
        <report>
            <clover outputFile="./.coverage/coverage.xml" />
            <html outputDirectory="./.coverage/report" lowUpperBound="35" highLowerBound="70" />
        </report>
    </coverage>
</phpunit>

@nicolas-grekas
Copy link
Member

Could you please make a repo with the reproducer? That would help us get quickly to running into the issue locally.

@shehi
Copy link
Contributor Author

shehi commented Oct 1, 2020

Hey @nicolas-grekas ,

At the moment I am quite filled with various projects, work and open-source of my own. That might be a bit difficult to spend a time on this one, otherwise I'd have rather worked on PR.

@nicolas-grekas nicolas-grekas added Help wanted Issues and PRs which are looking for volunteers to complete them. and removed Status: Waiting feedback labels Oct 1, 2020
@michaelKaefer
Copy link
Contributor

michaelKaefer commented Oct 25, 2020 •
edited
Loading

Related issues

Reproducer

I created a reproducer:

git clone git@github.com:michaelKaefer/reproducer-38239.git
cd reproducer-38239
composer install

No message with:

$ SYMFONY_DEPRECATIONS_HELPER="disabled" ./bin/phpunit
[...]
OK (1 test, 1 assertion)

With the following you get the message "THE ERROR HANDLER HAS CHANGED!", although in phpunit.xml file has line: <env name="SYMFONY_DEPRECATIONS_HELPER" value="disabled" />:

$ ./bin/phpunit
[...]
OK (1 test, 1 assertion)
THE ERROR HANDLER HAS CHANGED!

Fix

I can try to fix it, this solutions come to my mind:

  • Somewhere in vendor/symfony/phpunit-bridge/bin/simple-phpunit.php read all env variables from phpunit.xml and load them with something like:
putenv("$envName=$envValue");
$_SERVER[$envName] = $_ENV[$envName] = $envValue;
  • In src/Symfony/Bridge/PhpUnit/bootstrap.php read the value of SYMFONY_DEPRECATIONS_HELPER from phpunit.xml before using it. Which of the two following solutions is better for this?
    • From src/Symfony/Bridge/PhpUnit/bin/simple-phpunit.php copy paste the variable $getEnvVar = function ($name, $default = false) use ($argv) { ... }; to src/Symfony/Bridge/PhpUnit/bootstrap.php and use it there.
    • In src/Symfony/Bridge/PhpUnit/bin/simple-phpunit.php make the variable $getEnvVar = function ($name, $default = false) use ($argv) { ... }; globally available, and then use it in src/Symfony/Bridge/PhpUnit/bootstrap.php.
  • If SYMFONY_DEPRECATIONS_HELPER should not be defined in phpunit.xml anymore then it must be changed in the docs at https://symfony.com/doc/current/components/phpunit_bridge.html#configuration

Or is there a better solution?

@shehi
Copy link
Contributor Author

shehi commented Oct 26, 2020 •
edited
Loading

If SYMFONY_DEPRECATIONS_HELPER should not be defined in phpunit.xml anymore then it must be changed in the docs at https://symfony.com/doc/current/components/phpunit_bridge.html#configuration

I vote for this solution. The minimalist in me says there is no need to invent tons of stuff for a simple thing such as this.

@fancyweb fancyweb mentioned this issue Nov 30, 2020
Merged
nicolas-grekas added a commit that referenced this issue Dec 8, 2020
@nicolas-grekas
bug #39226 [PhpUnitBridge] Fix disabling DeprecationErrorHandler from…
cba70a6 
… PHPUnit configuration file (fancyweb)

This PR was merged into the 4.4 branch.

Discussion
----------

[PhpUnitBridge] Fix disabling DeprecationErrorHandler from PHPUnit configuration file

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | #38239
| License       | MIT
| Doc PR        | -

The linked issue description is right, the bridge's bootstrap is hit before the env variables from the phpunit.xml file are read and set. So the easiest solution is to read it ourselves in the root script (like we already do for some of them).

Commits
-------

41158b8 [PhpUnitBridge] Fix disabling DeprecationErrorHandler from phpunit configuration file
symfony-splitter pushed a commit to symfony/phpunit-bridge that referenced this issue Dec 8, 2020
@nicolas-grekas
bug #39226 [PhpUnitBridge] Fix disabling DeprecationErrorHandler from…
7232aa6 
… PHPUnit configuration file (fancyweb)

This PR was merged into the 4.4 branch.

Discussion
----------

[PhpUnitBridge] Fix disabling DeprecationErrorHandler from PHPUnit configuration file

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | symfony/symfony#38239
| License       | MIT
| Doc PR        | -

The linked issue description is right, the bridge's bootstrap is hit before the env variables from the phpunit.xml file are read and set. So the easiest solution is to read it ourselves in the root script (like we already do for some of them).

Commits
-------

41158b8878 [PhpUnitBridge] Fix disabling DeprecationErrorHandler from phpunit configuration file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Help wanted Issues and PRs which are looking for volunteers to complete them. PhpUnitBridge Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@nicolas-grekas @shehi @xabbuh @carsonbot @michaelKaefer