[PhpUnitBridge]allow outdated vendors mode #24867

greg0ire · Nov 7, 2017

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	could not find any
License	MIT
Doc PR	symfony/symfony-docs#8686

This PR introduces a new mode that will allow projects to ignore deprecation they cannot do anything about, in the case when a vendor calls another vendor in a deprecated way.

greg0ire · Nov 7, 2017

Not to be confused with weak vendors mode, which is useful for libraries, this one will be more useful for projects IMO.

To sum up, "weak" is weaker than "weak vendors", which is weaker than "weak lagging vendors", which is weaker than strict mode.

xabbuh · Nov 8, 2017

Well, if it's a new feature, it will go into 4.1.

greg0ire · Nov 8, 2017

Because of the feature freeze? I originally made the PR on master, then read this in the PR template:

Features and deprecations must be submitted against the 3.4, legacy code removals go to the master branch.

greg0ire · Nov 8, 2017

Anyway, fixed, and this is good because I used a ton of return type hints 😅

stof · Nov 8, 2017

@greg0ire you are right than during the development phase of 3.4 and 4.0, submissions like that should have been submitted to 3.4. But the feature freeze means that this feature cannot be accepted for 3.4/4.0 anymore.

greg0ire · Nov 8, 2017

Should the PR template be updated then?

stof · Nov 8, 2017

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

+                if (!self::inVendors($file)) {
+                    return false;
+                }
+                if (isset($erroringFile, $erroringPackage)) {


would be better to initialize variables to null before, and compare here instead. Having code dealing with undefined variables without failing makes it much easier to introduce mistakes (making a typo in a variable name does not trigger an error anymore)

stof · Nov 8, 2017

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

+    private static function isLaggingVendor(array $trace): bool
+    {
+        foreach ($trace as $line) {
+            if (isset($line['file'])) {


I suggest using an early continue when it is not set instead.

stof · Nov 8, 2017

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

+
+                return $vendor.'/'.strstr(substr($relativePath, strlen($vendor) + 1), DIRECTORY_SEPARATOR, true);
+            }
+        }


you should throw a meaningful exception when reaching this place explaining that the provided path is not a vendor path, instead of letting PHP throw a fatal error due to null not respecting the string return type (which would not be clear at all).

@stof we're inside the error handler, that's why I refrained from doing that, I'm not sure how it will behave, but I will try.

Wait, it's an error handler, so an exception should probably be fine.

Reaching this place would indicate a bad usage of the private method inside the class anyway (as you are not supposed to pass a non-vendor path). So I think it would be fine (and would make it easier for contributors to see their mistakes)

I tried putting an exception unconditionally on the first line, and it worked as expected, so it's fine indeed 👍

stof · Nov 8, 2017

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

+        static $vendors;
+
+        if (null === $vendors) {
+            foreach (get_declared_classes() as $class) {


you need to initialize $vendors to an empty array before the loop. Otherwise, it is possible that null is returned instead of an empty array if no composer autoloader can be detected.

stof · Nov 8, 2017

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

+        }
+    }
+
+    private static function getVendors(): ?array


returning a nullable value is not good, as the code wants to iterate over it all the time. Make it return an array all the time.

greg0ire · Nov 8, 2017

I don't understand why the build is failing. It's working locally, and I use php 7.1.10

stof · Nov 9, 2017

@greg0ire that's because the script used to run tests is installing the latest version of the PHPUnit Bridge, which does not yet have this feature.

Our CI does not play well for testing the bridge itself. We would have to make it execute tests with a special runner not installing its own copy of the bridge, to use the one under test.
@nicolas-grekas do you have an idea about how to achieve this ?

nicolas-grekas · Nov 9, 2017

Our CI does not play well for testing the bridge itself

correct, it's difficult because there can be 3 variants of the code (vendor/, .phupnit/ and src/) and deciding which one should be loaded is hard ("it depends").
Usually, I symlink all of them to src/ when working locally. No better for now.

greg0ire · Nov 9, 2017

correct, it's difficult because there can be 3 variants of the code (vendor/, .phupnit/ and src/) and deciding which one should be loaded is hard ("it depends").

Might that be why that other bugfix PR I made fails only in 7.2 : #24864 (comment)?

OskarStark · Nov 9, 2017

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

@@ -30,6 +31,8 @@ class DeprecationErrorHandler
     * The following reporting modes are supported:
     * - use "weak" to hide the deprecation report but keep a global count;
     * - use "weak_vendors" to act as "weak" but only for vendors;
+     * - use "weak_lagging_vendors" to act as "weak" but only for vendors that


maybe I am wrong, but shouldn't it be?

- * - use "weak_lagging_vendors" to act as "weak" but only for vendors that + * - use "weak_lagging_vendors" to act as "weak_wendors" but only for vendors that

Both statements are true ;)

greg0ire · Nov 15, 2017

How should you proceed? Will the CI be fixed or will you try my changes locally?

Refs symfony/symfony#24867

greg0ire · Jan 5, 2018

@alexpott I don't think this is the right place to discuss that though. Deciding whether to merge this or not should be independent from this bug IMO.

greg0ire · Jan 5, 2018

Oh I think I get what you mean, I'll wait for your changes to land on master to apply the same fix on this PR.

greg0ire · Jan 5, 2018

Oh wait it is already on master

greg0ire · Jan 22, 2018

@alexpott unless I'm mistaken, the serialized deprecation does not contain the whole trace, which makes it impossible for me to make the right call here. I chose to be lenient in that case.

alexpott · Jan 22, 2018

@greg0ire yep it does not. We could write that to the file but feels complex. Maybe if we get the trace using DEBUG_BACKTRACE_IGNORE_ARGS it might be okay.

greg0ire · Jan 23, 2018

You mean serialize the whole trace? Maybe yeah 😅

Now that we have php 7, we can afford to do this.

In this mode, failures coming from vendors that call other vendors in a deprecated way are not taken into account when deciding to make the build fail. They also appear in a separate group.

This is slightly less weird than a static local variable.

greg0ire · Jan 28, 2018

fabbot is unhappy but proposes to do this:

diff --git a/src/Symfony/Bridge/PhpUnit/Legacy/SymfonyTestsListenerTrait.php b/src/Symfony/Bridge/PhpUnit/Legacy/SymfonyTestsListenerTrait.php
index 6cd5391488..7350e06e45 100644
--- a/src/Symfony/Bridge/PhpUnit/Legacy/SymfonyTestsListenerTrait.php
+++ b/src/Symfony/Bridge/PhpUnit/Legacy/SymfonyTestsListenerTrait.php
@@ -271,7 +271,7 @@ class SymfonyTestsListenerTrait
                     'trace' => isset($deprecation[3]) ? $deprecation[3] : null,
                 ));
                 if ($deprecation[0]) {
-                    trigger_error($error, E_USER_DEPRECATED);
+                    @trigger_error($error, E_USER_DEPRECATED);
                 } else {
                     @trigger_error($error, E_USER_DEPRECATED);
                 }

No thanks 😅

@alexpott, I serialized the trace as per your recommendation
@dunglas , I got rid of the static local variable
@nicolas-grekas this is ready for review

Refs symfony/symfony#24867

greg0ire · Jan 31, 2018

Instead of introducing a new mode, it has been decided that a new display will be introduced, segregating deprecations into 3 sections (regardless of the mode?):

yours
your vendors
your vendors' vendors

The exit code will change depending on which section is empty or full, and will express that in binary, meaning:

0 = no deprecations
1 = deprecations triggered by your vendors, called by your vendors
2 = deprecations triggered by your vendors when called by your code
3 = deprecations triggered by your vendors when called by your code AND deprecations triggered by your vendors, called by your vendors
4 = deprecations triggered by your code when called by your code
…

This will allow people to make their build fail or pass with a simple comparison.

Thus, weak_vendors would be deprecated and translate to [[ $? < 4 ]] , and the mode described in this PR to [[ $? < 2 ]]

… (greg0ire) This PR was merged into the 4.3-dev branch. Discussion ---------- [PhpUnitBridge] Url encoded deprecations helper config | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #28048 | License | MIT | Doc PR | symfony/symfony-docs#10701 First stab at implementing a new way of configuring the deprecation error handler. Includes a refactoring to keep things manageable. Rework of #24867, blocked by #29718 TODO: - [x] make the code 5.5 compatible 😢 - [x] add more tests - [x] deprecate modes (using echo :P) - [x] test this on real life projects and add some screenshots - [x] docs PR - [x] handle `strict` - [x] adapt existing CI config # Quiet configuration ![quiet](https://user-images.githubusercontent.com/657779/49341318-fa78c900-f64b-11e8-9504-a8a9eac4baf8.png) # Default configuration ![verbose](https://user-images.githubusercontent.com/657779/49341322-10868980-f64c-11e8-9d90-dc3f6a18c335.png) Commits ------- 1c73f9c [PhpUnitBridge] Url encoded deprecations helper config

carsonbot added Status: Needs Review Feature labels Nov 7, 2017

greg0ire force-pushed the ignore_lagging_vendors_mode branch from b263050 to f382030 Compare November 7, 2017 23:36

greg0ire changed the title ~~Ignore lagging vendors mode~~ weak lagging vendors mode Nov 7, 2017

xabbuh added the PhpUnitBridge label Nov 8, 2017

xabbuh added this to the 4.1 milestone Nov 8, 2017

greg0ire changed the base branch from 3.4 to master November 8, 2017 09:20

greg0ire force-pushed the ignore_lagging_vendors_mode branch from f382030 to 9ef2619 Compare November 8, 2017 09:20

stof requested changes Nov 8, 2017

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Nov 8, 2017

greg0ire force-pushed the ignore_lagging_vendors_mode branch 4 times, most recently from c49960c to 7925208 Compare November 8, 2017 19:44

OskarStark reviewed Nov 9, 2017

View reviewed changes

greg0ire added a commit to greg0ire/symfony-docs that referenced this pull request Nov 16, 2017

Document weak lagging vendors mode

6301a75

Refs symfony/symfony#24867

This was referenced Nov 16, 2017

Document allow outdated vendors mode symfony/symfony-docs#8686

Closed

Have weak_vendors ignore deprecations from outside #24864

Merged

greg0ire force-pushed the ignore_lagging_vendors_mode branch from 7925208 to 0a10c00 Compare January 5, 2018 10:41

greg0ire force-pushed the ignore_lagging_vendors_mode branch from 0a10c00 to b95ce02 Compare January 22, 2018 20:03

greg0ire added 7 commits January 27, 2018 17:59

Refactor inVendors to a private static method

4a02b81

Now that we have php 7, we can afford to do this.

Introduce weak_lagging_vendors mode

9fdbe2b

In this mode, failures coming from vendors that call other vendors in a deprecated way are not taken into account when deciding to make the build fail. They also appear in a separate group.

Skip if php 7.2 and php input or phar

b17db80

Give out more details

de52e93

Include the trace in the deprecation metadata

8566bd6

Adapt to new message format

cc6e2bb

Refactor static variable to static property

f613ebf

This is slightly less weird than a static local variable.

greg0ire force-pushed the ignore_lagging_vendors_mode branch from b95ce02 to f613ebf Compare January 28, 2018 20:02

Refactor lagging into outdated

4d40407

greg0ire changed the title ~~weak lagging vendors mode~~ allow outdated vendors mode Jan 29, 2018

greg0ire added a commit to greg0ire/symfony-docs that referenced this pull request Jan 29, 2018

Document allow outdated vendors mode

9e6d8b0

Refs symfony/symfony#24867

greg0ire added a commit to greg0ire/symfony-docs that referenced this pull request Jan 30, 2018

Document allow outdated vendors mode

ed845bd

Refs symfony/symfony#24867

greg0ire closed this Jan 31, 2018

greg0ire deleted the ignore_lagging_vendors_mode branch January 31, 2018 16:00

This was referenced Apr 22, 2018

[PhpUnitBridge] Always segregate vendor and non vendor deprecations #27006

Closed

[WebProfilerBundle] Improve the deprecation logs panel #26972

Closed

greg0ire changed the title ~~allow outdated vendors mode~~ [PhpUnitBridge]allow outdated vendors mode Apr 22, 2018

greg0ire mentioned this pull request Aug 10, 2018

[PhpUnitBridge] make "numbered" deprecation mode count only vendor deprecations #28048

Closed

greg0ire mentioned this pull request Nov 13, 2018

[PhpUnitBridge] Url encoded deprecations helper config #29211

Merged

7 tasks

Search code, repositories, users, issues, pull requests...

Uh oh!

[PhpUnitBridge]allow outdated vendors mode #24867

[PhpUnitBridge]allow outdated vendors mode #24867

Uh oh!

Conversation

greg0ire commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greg0ire commented Nov 7, 2017

Uh oh!

xabbuh commented Nov 8, 2017

Uh oh!

greg0ire commented Nov 8, 2017

Uh oh!

greg0ire commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stof commented Nov 8, 2017

Uh oh!

greg0ire commented Nov 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire commented Nov 8, 2017

Uh oh!

stof commented Nov 9, 2017

Uh oh!

nicolas-grekas commented Nov 9, 2017

Uh oh!

greg0ire commented Nov 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire commented Nov 15, 2017

Uh oh!

greg0ire commented Jan 5, 2018

Uh oh!

greg0ire commented Jan 5, 2018

Uh oh!

greg0ire commented Jan 5, 2018

Uh oh!

greg0ire commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexpott commented Jan 22, 2018

Uh oh!

greg0ire commented Jan 23, 2018

Uh oh!

greg0ire commented Jan 28, 2018

Uh oh!

greg0ire commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

greg0ire commented Nov 7, 2017 •

edited

Loading

greg0ire commented Nov 8, 2017 •

edited

Loading

greg0ire commented Jan 22, 2018 •

edited

Loading

greg0ire commented Jan 31, 2018 •

edited

Loading