Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[PhpUnitBridge]allow outdated vendors mode#24867
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
b263050 tof382030Comparegreg0ire commentedNov 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 commentedNov 8, 2017
Well, if it's a new feature, it will go into 4.1. |
greg0ire commentedNov 8, 2017
Because of the feature freeze? I originally made the PR on master, then read this in the PR template:
|
f382030 to9ef2619Comparegreg0ire commentedNov 8, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Anyway, fixed, and this is good because I used a ton of return type hints 😅 |
stof commentedNov 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 commentedNov 8, 2017
Should the PR template be updated then? |
| if (!self::inVendors($file)) { | ||
| returnfalse; | ||
| } | ||
| if (isset($erroringFile,$erroringPackage)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
would be better to initialize variables tonull 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)
| privatestaticfunctionisLaggingVendor(array$trace):bool | ||
| { | ||
| foreach ($traceas$line) { | ||
| if (isset($line['file'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I suggest using an earlycontinue when it is not set instead.
| return$vendor.'/'.strstr(substr($relativePath,strlen($vendor) +1),DIRECTORY_SEPARATOR,true); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 tonull not respecting thestring return type (which would not be clear at all).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wait, it's anerror handler, so an exception should probably be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I tried putting an exception unconditionally on the first line, and it worked as expected, so it's fine indeed 👍
| static$vendors; | ||
| if (null ===$vendors) { | ||
| foreach (get_declared_classes()as$class) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you need to initialize$vendors to an empty array before the loop. Otherwise, it is possible thatnull is returned instead of an empty array if no composer autoloader can be detected.
| } | ||
| } | ||
| privatestaticfunctiongetVendors(): ?array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
c49960c to7925208Comparegreg0ire commentedNov 8, 2017
I don't understand why the build is failing. It's working locally, and I use php 7.1.10 |
stof commentedNov 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 runnernot installing its own copy of the bridge, to use the one under test. |
nicolas-grekas commentedNov 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"). |
greg0ire commentedNov 9, 2017
Might that be why that other bugfix PR I made fails only in 7.2 :#24864 (comment)? |
| * 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Both statements are true ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yes 😄
greg0ire commentedNov 15, 2017
How should you proceed? Will the CI be fixed or will you try my changes locally? |
greg0ire commentedJan 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. |
7925208 to0a10c00Comparegreg0ire commentedJan 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 commentedJan 5, 2018
Oh wait it is already on master |
0a10c00 tob95ce02Comparegreg0ire commentedJan 22, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 commentedJan 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 commentedJan 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 adeprecated way are not taken into account when deciding to make thebuild fail. They also appear in a separate group.
This is slightly less weird than a static local variable.
b95ce02 tof613ebfComparegreg0ire commentedJan 28, 2018
fabbot is unhappy but proposes to do this: No thanks 😅 @alexpott, I serialized the trace as per your recommendation |
greg0ire commentedJan 31, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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?):
The exit code will change depending on which section is empty or full, and will express that in binary, meaning:
This will allow people to make their build fail or pass with a simple comparison. Thus, |
… (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#10701First stab at implementing a new way of configuring the deprecation error handler. Includes a refactoring to keep things manageable.Rework of#24867, blocked by#29718TODO:- [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# Default configurationCommits-------1c73f9c [PhpUnitBridge] Url encoded deprecations helper config
Uh oh!
There was an error while loading.Please reload this page.
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.