Introduce weak vendors mode #21539

greg0ire · 2017-02-05T18:18:41Z

Deprecations coming from the vendors are segregated from other
deprecations. A new mode is introduced, in which deprecations coming
from the vendors are not taken into account when deciding to exit with
an error code.

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

Sample output :

Weak vendor mode

With both vendors and non-vendor errors


WARNINGS!
Tests: 1068, Assertions: 2714, Warnings: 6, Skipped: 15.

Remaining deprecation notices (1)

some error: 1x
    1x in SonataAdminBundleTest::testBuild from Sonata\AdminBundle\Tests

Remaining vendor deprecation notices (4)

Legacy deprecation notices (48)

Exit code is 1

After fixing non-vendor errors

WARNINGS!
Tests: 1068, Assertions: 2714, Warnings: 6, Skipped: 15.

Remaining vendor deprecation notices (4)

Legacy deprecation notices (48)

Exit code is 0

TODO

fix colorization issues (vendor deprecation notices are always in red)
make the vendor detection more robust
make the vendor dir configurable
~~figure out how to run tests and~~ add more of them
- test on non-vendor file
- test on vendor file
do not change the output of other modes

nicolas-grekas · 2017-02-05T19:11:53Z

I see two issues here:

the first, minor, is that the vendor dir is not always called "vendors"
the second is more serious: any code in vendors is always called by some code in userland. Code don't get called magically - but because userland asked for it - thus there is not such thing as "vendor deprecations" vs others. Or do you have a way to decide that works reliably?

greg0ire · 2017-02-05T19:28:30Z

Yes that's an issue (see my todo list), maybe I can work around that with another env var?
Not an issue IMO, I'm only looking at the first file of the stack trace, which is the one where trigger_error is written. Quoting the manual : "The third parameter is optional, errfile, which contains the filename that the error was raised in, as a string. " So to sum up, a deprecation is a "vendor deprecations" iff the trigger_error that raises it is written in a file that lives in vendor

greg0ire · 2017-02-05T19:35:44Z

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

@@ -76,6 +83,10 @@ public static function register($mode = 0)
            $trace = debug_backtrace(true);
            $group = 'other';

+            $isVendor = (strpos($file, '/vendor/') !== false);


@nicolas-grekas : we're outside the while on $trace here, so really, we're not looking at every file of the stack trace, but only at the file that has the error triggered in it.

greg0ire · 2017-02-05T19:43:36Z

Third issue, even more minor, some people might have a vendor directory inside their src (why not). Might be solvable with __DIR__ though.

nicolas-grekas · 2017-02-05T20:28:30Z

I'm only looking at the first file of the stack trace

That means you target deprecations triggered by code in src/. Ok.

For vendors, see my PR about ComposerResource.

greg0ire · 2017-02-05T20:32:51Z

I'm only looking at the first file of the stack trace

Actually that was wrong and has nothing to do with the stack trace, I'm looking at the $errfile argument of the error handler function. So if I have a file in src, that calls a function from the vendor dir, that has a trigger_error in it, $errfile will be vendor/callee.php, and not src/caller.php, right?

greg0ire · 2017-02-05T20:34:12Z

For vendors, see my PR about ComposerResource.

link for the lazy

ogizanagi · 2017-02-05T20:42:06Z

link for the laziest 😄

greg0ire · 2017-02-05T20:51:16Z

Maybe I should explain a bit more the goal of this PR, I feel I'm being unclear.

This is going to used way more in libs than in project, that seldom use deprecation notices themselves. @nicolas-grekas I think that's why you talk about userland vs vendors.

Let there be lib X, that has n dependencies. Anytime any of the n dependencies introduces a new deprecation, I have to fix it, and no PR can be merged during that time, unless I use SYMFONY_DEPRECATIONS_HELPER=weak.

And that's what I do most of the time, because I don't want to shift the burden of fixing these to anyone who wants to contribute anything unrelated to the vendor lib, and has the bad luck of doing so at the wrong time.

Now let's suppose a contributor submits a new PR, and let's suppose that that PR introduces a deprecation notice in the codebase of the lib , and by that I don't mean a deprecated call to another lib, I mean a call to trigger_error directly. Then they must be careful to mark all corresponding tests with @group legacy, and fix all calls throughout the codebase of the lib so that they use the new way of doing things.

In my experience, many people forget to do that, and deprecations are introduced, that could easily be avoided. See this PR where I end up doing the work that should have been done by the contributor from the start.

nicolas-grekas · 2017-02-06T16:46:41Z

Doc PR would be really nice before merging since the use case should be explained there :)

stof · 2017-02-06T16:53:58Z

@greg0ire the issue is that your logic looks wrong. Vendor deprecations must not be distinguished based on the location of the deprecation, but based on the caller location.
You will almost never call your own deprecated code, as you are also the one deprecating it. However, you are very likely to be calling a deprecated method of one of your dependency. In this case, trigger_error is inside the vendor folder, but the place needing to be fixed is inside your own code.
And using the backtrace to identify whether it is called from your own code or no is fragile, because your code may not be the direct caller. We have cases in Symfony where we have more classes between your own code and the code triggering the deprecation for instance, while the fix is still needed in your own code, for instance when asking to migrate from a type name to a class name to reference form types in Symfony 2.8

greg0ire · 2017-02-06T16:57:18Z

You will almost never call your own deprecated code, as you are also the one deprecating it

That's precisely the case I'm trying to help fix however. There are contributors that contribute new deprecations, and forget to fix these calls. Then it get merged, but could have easily be prevented. Also, there are tests that are marked with @group Legacy (note the capital), which does not work either. That could also have been prevented easily. Or they just outright forget to mark tests with @group legacy That's what I want to catch here.

And using the backtrace to identify whether it is called from your own code or no is fragile

I'm not doing that (I think). I'm not indentify the caller location, I'm identifying the callee location. When developing a lib, the lib is always calling others or itself, but no other lib is calling my lib. So in that case, I think it's safe to want to know "is there any call to a deprecated method of my lib in the codebase of my lib?"

horizontally: caller / vertically: callee	my lib	my dependency
my lib	This is the case I want to catch. It can only be introduced in new contributions that come with a deprecation notice	Cannot happen in the context of testing a library. If it happens, it means the dependency knows the lib, and that we have a circular dependency.
my dependency	This is the case I don't want to catch, because it would make the build fail randomly	Can't happen either in the context of testing lib.

greg0ire · 2017-02-06T23:21:38Z

Doc PR would be really nice before merging since the use case should be explained there :)

Didn't notice your comment. Will do!

greg0ire · 2017-02-07T22:26:13Z

Here is the docs PR

nicolas-grekas · 2017-02-07T22:33:17Z

Thanks, just posted a few short notes

rybakit · 2017-02-08T07:56:00Z

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

@@ -50,7 +52,10 @@ public static function register($mode = 0)
            if (false === $mode) {
                $mode = getenv('SYMFONY_DEPRECATIONS_HELPER');
            }
-            if (DeprecationErrorHandler::MODE_WEAK !== $mode && (!isset($mode[0]) || '/' !== $mode[0])) {
+            if (!in_array($mode, array(


Missing true as a 3rd parameter for in_array: https://3v4l.org/hsgBm

fixed, thanks!

the in_array + line splits look unreadable to me - what about using two comparisons on a single line instead?

I think it's a matter of taste, because to me the more readable version is of course mine. Plus it's less repetitive. I'll do as you say though, b/c you're probably more aware of what the sf style is.

greg0ire · 2017-02-08T22:42:26Z

I changed the code so that other modes output is no longer split into vendors / non-vendors.

greg0ire · 2017-02-08T22:49:54Z

I fixed the colorization issue, deprecation notices are always in yellow now.

fabpot · 2017-02-12T19:22:38Z

fabbot reports errors that should be fixed.

greg0ire · 2017-02-12T19:54:41Z

@fab{b,p}ot : fixed

greg0ire · 2017-02-21T14:47:53Z

I rebased again, can anyone review and remove the "Needs work" label? This is supposed to be finished.

greg0ire · 2017-02-21T17:15:35Z

Changed the poor man's error handler from var_dump to print_r after discussing with @Nek-

nicolas-grekas · 2017-02-21T17:18:23Z

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

@@ -160,16 +196,26 @@ public static function register($mode = 0)
                    $colorize = function ($str) { return $str; };
                }
                if ($currErrorHandler !== $deprecationHandler) {
+                    print_r(error_get_last());


unrelated, and very fragile to me, should be removed from this PR

Ok let's remove it.

Not sure what exactly qualifies as fragile here though. I think both methods are rock solid.

the link between error_get_last() and the error handler change - maybe I'm wrong - it's still unrelated so please open another PR if you want to investigate the idea

Unrelated indeed. Will do

A new mode is introduced, in which deprecations coming from the vendors are not taken into account when deciding to exit with an error code. In this mode, deprecations coming from the vendors are segregated from other deprecations.

greg0ire · 2017-02-27T17:11:54Z

This is supposed to be ready on my end, please review again.

nicolas-grekas

👍

fabpot · 2017-02-28T01:37:45Z

Thank you @greg0ire.

This PR was merged into the 3.3-dev branch. Discussion ---------- Introduce weak vendors mode Deprecations coming from the vendors are segregated from other deprecations. A new mode is introduced, in which deprecations coming from the vendors are not taken into account when deciding to exit with an error code. | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT | Doc PR | symfony/symfony-docs#7453  Sample output : # Weak vendor mode ## With both vendors and non-vendor errors ``` WARNINGS! Tests: 1068, Assertions: 2714, Warnings: 6, Skipped: 15. Remaining deprecation notices (1) some error: 1x 1x in SonataAdminBundleTest::testBuild from Sonata\AdminBundle\Tests Remaining vendor deprecation notices (4) Legacy deprecation notices (48) ``` Exit code is 1 ## After fixing non-vendor errors ``` WARNINGS! Tests: 1068, Assertions: 2714, Warnings: 6, Skipped: 15. Remaining vendor deprecation notices (4) Legacy deprecation notices (48) ``` Exit code is 0 # TODO - [x] fix colorization issues (vendor deprecation notices are always in red) - [x] make the vendor detection more robust - [x] make the vendor dir configurable - [x] ~figure out how to run tests and~ add more of them - [x] test on non-vendor file - [x] test on vendor file - [x] do not change the output of other modes Commits ------- 61fd043 Introduce weak vendors mode

greg0ire · 2017-02-28T09:58:08Z

Made a separate PR for the poor man's error handling : #21795

This PR was merged into the master branch. Discussion ---------- Document the weak_vendors value See symfony/symfony#21539 Commits ------- abe88c6 Document the weak_vendors value

carsonbot added the Status: Needs Review label Feb 5, 2017

greg0ire commented Feb 5, 2017

View reviewed changes

nicolas-grekas added this to the 3.x milestone Feb 6, 2017

nicolas-grekas added Feature Status: Needs Work and removed Status: Needs Review labels Feb 6, 2017

greg0ire mentioned this pull request Feb 7, 2017

Document the weak_vendors value symfony/symfony-docs#7453

Merged

rybakit suggested changes Feb 8, 2017

View reviewed changes

greg0ire force-pushed the weak_vendors branch from d8442b0 to 2ea482b Compare February 8, 2017 09:01

greg0ire force-pushed the weak_vendors branch 2 times, most recently from 028a816 to bd8f0b3 Compare February 8, 2017 22:49

greg0ire force-pushed the weak_vendors branch from bd8f0b3 to ddf0f9b Compare February 12, 2017 19:53

greg0ire force-pushed the weak_vendors branch from 457d29b to 15cce3b Compare February 21, 2017 14:34

greg0ire force-pushed the weak_vendors branch from 15cce3b to 4df40df Compare February 21, 2017 17:07

nicolas-grekas reviewed Feb 21, 2017

View reviewed changes

greg0ire force-pushed the weak_vendors branch from 4df40df to 5d0dbdf Compare February 21, 2017 17:19

Introduce weak vendors mode

61fd043

A new mode is introduced, in which deprecations coming from the vendors are not taken into account when deciding to exit with an error code. In this mode, deprecations coming from the vendors are segregated from other deprecations.

greg0ire force-pushed the weak_vendors branch from 5d0dbdf to 61fd043 Compare February 21, 2017 22:18

greg0ire mentioned this pull request Feb 27, 2017

avoid triggering our own deprecation notice sonata-project/SonataBlockBundle#367

Merged

nicolas-grekas approved these changes Feb 27, 2017

View reviewed changes

nicolas-grekas added Status: Reviewed and removed Status: Needs Work labels Feb 27, 2017

xabbuh approved these changes Feb 27, 2017

View reviewed changes

fabpot merged commit 61fd043 into symfony:master Feb 28, 2017

greg0ire deleted the weak_vendors branch February 28, 2017 07:08

greg0ire mentioned this pull request Feb 28, 2017

[PHPUnit Bridge]Implement poor man's error handler #21795

Closed

theofidry mentioned this pull request Mar 12, 2017

[PhpUnitBridge] vendor weak mode fails #21980

Closed

xabbuh mentioned this pull request Mar 13, 2017

Stop relying on the $mode argument #21986

Merged

theofidry mentioned this pull request Mar 14, 2017

[PhpUnitBridge] weak vendors mode silence legitimate deprecations #21991

Closed

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

greg0ire mentioned this pull request Jan 15, 2018

Recommend being lenient only with other vendors symfony/symfony-docs#9062

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce weak vendors mode #21539

Introduce weak vendors mode #21539

greg0ire commented Feb 5, 2017 •

edited

Loading

nicolas-grekas commented Feb 5, 2017 •

edited

Loading

greg0ire commented Feb 5, 2017 •

edited

Loading

greg0ire Feb 5, 2017

greg0ire commented Feb 5, 2017

nicolas-grekas commented Feb 5, 2017

greg0ire commented Feb 5, 2017

greg0ire commented Feb 5, 2017

ogizanagi commented Feb 5, 2017

greg0ire commented Feb 5, 2017 •

edited

Loading

nicolas-grekas commented Feb 6, 2017

stof commented Feb 6, 2017

greg0ire commented Feb 6, 2017 •

edited

Loading

greg0ire commented Feb 6, 2017

greg0ire commented Feb 7, 2017

nicolas-grekas commented Feb 7, 2017

rybakit Feb 8, 2017

greg0ire Feb 8, 2017

nicolas-grekas Feb 14, 2017

greg0ire Feb 14, 2017

greg0ire commented Feb 8, 2017

greg0ire commented Feb 8, 2017

fabpot commented Feb 12, 2017

greg0ire commented Feb 12, 2017

greg0ire commented Feb 21, 2017

greg0ire commented Feb 21, 2017

nicolas-grekas Feb 21, 2017

greg0ire Feb 21, 2017

greg0ire Feb 21, 2017 •

edited

Loading

nicolas-grekas Feb 21, 2017

greg0ire Feb 21, 2017

greg0ire commented Feb 27, 2017

nicolas-grekas left a comment

fabpot commented Feb 28, 2017

greg0ire commented Feb 28, 2017

Introduce weak vendors mode #21539

Introduce weak vendors mode #21539

Conversation

greg0ire commented Feb 5, 2017 • edited Loading

Weak vendor mode

With both vendors and non-vendor errors

After fixing non-vendor errors

TODO

nicolas-grekas commented Feb 5, 2017 • edited Loading

greg0ire commented Feb 5, 2017 • edited Loading

Choose a reason for hiding this comment

greg0ire commented Feb 5, 2017

nicolas-grekas commented Feb 5, 2017

greg0ire commented Feb 5, 2017

greg0ire commented Feb 5, 2017

ogizanagi commented Feb 5, 2017

greg0ire commented Feb 5, 2017 • edited Loading

nicolas-grekas commented Feb 6, 2017

stof commented Feb 6, 2017

greg0ire commented Feb 6, 2017 • edited Loading

greg0ire commented Feb 6, 2017

greg0ire commented Feb 7, 2017

nicolas-grekas commented Feb 7, 2017

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 commented Feb 8, 2017

greg0ire commented Feb 8, 2017

fabpot commented Feb 12, 2017

greg0ire commented Feb 12, 2017

greg0ire commented Feb 21, 2017

greg0ire commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Feb 27, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

fabpot commented Feb 28, 2017

greg0ire commented Feb 28, 2017

greg0ire commented Feb 5, 2017 •

edited

Loading

nicolas-grekas commented Feb 5, 2017 •

edited

Loading

greg0ire commented Feb 5, 2017 •

edited

Loading

greg0ire commented Feb 5, 2017 •

edited

Loading

greg0ire commented Feb 6, 2017 •

edited

Loading

greg0ire Feb 21, 2017 •

edited

Loading