Skip to content
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.

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

[Bridge\PhpUnit] Add "disabled" mode to SYMFONY_DEPRECATIONS_HELPER #18232

Merged
merged 1 commit into from Mar 23, 2016

Conversation

@nicolas-grekas
Copy link
Member

commented Mar 19, 2016

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

See https://github.com/symfony/symfony/pull/18232/files?w=1

Because we keep adding features to the bridge, so that one could want some features but not the deprecations helper.
In weak mode, this PR also disables colors.
Fetching the SYMFONY_DEPRECATIONS_HELPER env var is also done lazily to workaround old phpunit versions affected by sebastianbergmann/phpunit#1216

@nicolas-grekas nicolas-grekas changed the title [Bridge\PhpUnit] Add "disabled" mode [Bridge\PhpUnit] Add "disabled" mode to SYMFONY_DEPRECATIONS_HELPER Mar 19, 2016

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:phpunit-wa branch from 74963db to 5ce8438 Mar 19, 2016

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2016

Status: needs work, for 5.3 support

@theofidry

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2016

In weak mode, this PR also disables colors.

That's the only change I'm not really fond of to be honest: if you don't want colors I would expect to set the PHPUnit colors configuration at false.

Otherwise 👍

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2016

@theofidry we don't access to phpunit configuration. PR welcomed if anyone thinks this is of importance.

@theofidry

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2016

we don't access to phpunit configuration

Is it because it's something that has been needed until now or there is a reason behind that?

PR welcomed if anyone thinks this is of importance.

In the example of the color, it doesn't really make sense (at least to me) to set this at the bridge level. But I have no idea of how you access to PHPUnit config so if it's too troublesome then it's understandable. It's just a color setting after all...

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:phpunit-wa branch from 5ce8438 to 89cd063 Mar 19, 2016

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2016

Status: needs review
Code updated to work on 5.3

@theofidry that's it: nobody spent time looking for a way to have tighter phpunit integration, because as you said:

It's just a color setting after all...

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:phpunit-wa branch from 89cd063 to 13ed5cf Mar 19, 2016

@theofidry

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2016

Fair.

This PR will require to update the doc though.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2016

@theofidry would you like opening it please?

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:phpunit-wa branch from 13ed5cf to 49931f4 Mar 19, 2016

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2016

Tests added

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:phpunit-wa branch 3 times, most recently from 2e0fa55 to 85852f7 Mar 19, 2016

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:phpunit-wa branch from 85852f7 to 8e00abf Mar 19, 2016

$mode = preg_match('/^[1-9][0-9]*$/', $mode) ? (int) $mode : 0;
}
$mode = function () use ($mode) {

This comment has been minimized.

Copy link
@stof

stof Mar 21, 2016

Member

reusing the same variable name to store the function makes the code much harder to read

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:phpunit-wa branch from 8e00abf to 3a65159 Mar 21, 2016

putenv('SYMFONY_DEPRECATIONS_HELPER');
define('PHPUNIT_COMPOSER_INSTALL', __DIR__.'/../../vendor/autoload.php');

This comment has been minimized.

Copy link
@stof

stof Mar 21, 2016

Member

would this work when running tests from the root of the project ?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 21, 2016

Author Member

no, that's why they are skipped (see line 3-4)

This comment has been minimized.

Copy link
@stof

stof Mar 21, 2016

Member

wouldn't it be better to check for __DIR__.'/../../vendor/autoload.php' instead then ? It would at least be easier to understand

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 21, 2016

Author Member

sure it would, but __DIR__ is not correct/does not work in the SKIPIF section context...

This comment has been minimized.

Copy link
@stof

stof Mar 21, 2016

Member

actually, we cannot use __DIR__ in skipif as it is not replaced before evaluating the code (which is not done from a file anymore).

This comment has been minimized.

Copy link
@stof

stof Mar 21, 2016

Member

btw, would be it possible to support the case of running these tests from the root too ? It would make it easier for contributors to run them.

@stof

This comment has been minimized.

Copy link
Member

commented Mar 21, 2016

@nicolas-grekas shouldn't most of these tests actually be added in older branches ?

--TEST--
Test DeprecationErrorHandler in default mode
--SKIPIF--
<?php if (!file_exists('DeprecationErrorHandler.php')) print "skip"; ?>

This comment has been minimized.

Copy link
@stof

stof Mar 21, 2016

Member

I suggest giving more details about the reason for the test being skipped (for instance skip can only run from a standalone install of the component).

.travis.yml Outdated
- if [[ $deps = low ]]; then echo "$COMPONENTS" | parallel --gnu -j10% 'cd {}; composer update --prefer-dist --prefer-lowest --prefer-stable; $PHPUNIT --exclude-group tty,benchmark,intl-data'; fi;
# Test the PhpUnit bridge using the original phpunit script
- if [[ $deps = low ]]; then (cd src/Symfony/Bridge/PhpUnit && phpenv global 5.3 && php --version && composer update --prefer-dist && phpunit && find -name '*.php' -not -path './vendor/*' | xargs -n1 php -l); fi;

This comment has been minimized.

Copy link
@stof

stof Mar 21, 2016

Member

wouldn't it be more logical to run php -l before running tests rather than after ?

This comment has been minimized.

Copy link
@stof

stof Mar 21, 2016

Member

btw, should we run php -l on the whole codebase rather than just this component ? How many time would it take ?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 21, 2016

Author Member

We could, but I'm sure we have expected parse errors in fixtures so this would need an exclusion rule. Not for this PR though, so if anyone wants to submit it (against 2.3), please do.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 21, 2016

Author Member

I fact, I removed the php -l check since we can test everything with phpt

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:phpunit-wa branch from b91ee53 to d79d731 Mar 21, 2016

@stof

This comment has been minimized.

Copy link
Member

commented Mar 21, 2016

right, but it's much simpler for me to make them work on master first, and backport afterwards

Will you backport them before merging this PR though ? It would make the git history cleaner by avoiding to add them twice.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:phpunit-wa branch 11 times, most recently from 3cd458d to 5747dc1 Mar 21, 2016

@stof

This comment has been minimized.

Copy link
Member

commented Mar 23, 2016

Looks good to me. 👍

@nicolas-grekas can you answer my previous question ?

@stof

This comment has been minimized.

Copy link
Member

commented Mar 23, 2016

Status: reviewed

nicolas-grekas added a commit that referenced this pull request Mar 23, 2016
minor #18270 [travis] Upgrade phpunit wrapper & hirak/prestissimo (ni…
…colas-grekas)

This PR was merged into the 2.3 branch.

Discussion
----------

[travis] Upgrade phpunit wrapper & hirak/prestissimo

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

Must be merged after #18232

Commits
-------

bf465eb [travis] Upgrade phpunit wrapper & hirak/prestissimo

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:phpunit-wa branch from 5747dc1 to 64fe539 Mar 23, 2016

@nicolas-grekas nicolas-grekas merged commit 64fe539 into symfony:master Mar 23, 2016

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details
nicolas-grekas added a commit that referenced this pull request Mar 23, 2016
feature #18232 [Bridge\PhpUnit] Add "disabled" mode to SYMFONY_DEPREC…
…ATIONS_HELPER (nicolas-grekas)

This PR was merged into the 3.1-dev branch.

Discussion
----------

[Bridge\PhpUnit] Add "disabled" mode to SYMFONY_DEPRECATIONS_HELPER

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

See https://github.com/symfony/symfony/pull/18232/files?w=1

Because we keep adding features to the bridge, so that one could want some features but not the deprecations helper.
In weak mode, this PR also disables colors.
Fetching the SYMFONY_DEPRECATIONS_HELPER env var is also done lazily to workaround old phpunit versions affected by sebastianbergmann/phpunit#1216

Commits
-------

64fe539 [Bridge\PhpUnit] Add "disabled" mode to SYMFONY_DEPRECATIONS_HELPER

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:phpunit-wa branch Mar 23, 2016

@fabpot fabpot referenced this pull request May 13, 2016
@joeyhub

This comment has been minimized.

Copy link

commented Mar 19, 2019

No matter what I do I can't disable the depreciations, can you explain clearly how to do that?

@@ -67,11 +68,10 @@ public static function register($mode = 0)
'other' => array(),
);
$deprecationHandler = function ($type, $msg, $file, $line, $context) use (&$deprecations, $getMode) {
if (E_USER_DEPRECATED !== $type) {
if (E_USER_DEPRECATED !== $type || DeprecationErrorHandler::MODE_DISABLED === $mode = $getMode()) {

This comment has been minimized.

Copy link
@joeyhub

joeyhub Mar 19, 2019

Where is $mode used?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.