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] Url encoded deprecations helper config#29211
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
[PhpUnitBridge] Url encoded deprecations helper config#29211
Uh oh!
There was an error while loading.Please reload this page.
Conversation
308d9b0 to586053aCompared010f02 to4c7adb1Compareostrolucky commentedNov 24, 2018
Why when you are targetting master? |
greg0ire commentedNov 24, 2018
IIRC that's because recent versions of the bridge will be used to test old versions of other Symfony components: symfony/src/Symfony/Bridge/PhpUnit/composer.json Lines 19 to 21 inf9414a8
|
greg0ire commentedNov 24, 2018
Wait@nicolas-grekas , is it really 5.3, not 5.5? Did you maybe tell me we could bump to 5.5? |
nicolas-grekas commentedNov 24, 2018
5.5 will be good soon because 2.8 will be out of maintenance in 1 week. |
eea83c2 to9c62a77Comparegreg0ire commentedNov 25, 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.
Here is how the "Improve stack trace display" commit changes things: BeforeAfter:The package name is now included, which helps me. Tell me if you do not like it, and I will remove it just before this is merged. |
9c62a77 to14fd0e1CompareUh oh!
There was an error while loading.Please reload this page.
9251b1a to1006b12Compare1006b12 to1c73f9cCompare
nicolas-grekas left a comment
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.
Nice, thanks! This will improve a lot the semantics and the flexibility!
fabpot commentedApr 12, 2019
Thank you@greg0ire. |
… (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
| returnfalse !==strpos($key,'Count') &&false ===strpos($key,'legacy'); | ||
| },ARRAY_FILTER_USE_KEY); | ||
| if (array_sum($deprecationCounts) >$this->thresholds['total']) { |
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.
this is broken if there is no such threshold
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.
It is always initialized to 999999, see the ctor
| returnfalse; | ||
| } | ||
| foreach (['self','direct','indirect']as$deprecationType) { | ||
| if ($deprecationCounts['remaining'.$deprecationType.'Count'] >$this->thresholds[$deprecationType]) { |
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.
this is broken if there is no such threshold
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.
same as above
Uh oh!
There was an error while loading.Please reload this page.
This PR was merged into the 4.3-dev branch.Discussion----------[PhpUnitBridge] fixes| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Fixes@stof's review in#29211 + fixes PHP5.5 support (array consts are PHP5.6+)/cc@greg0ireCommits-------b11585e [PhpUnitBridge] fixes
| } | ||
| $trace =debug_backtrace(); | ||
| $deprecation =newDeprecation($msg,debug_backtrace(),$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.
$msg is unserialized in Deprecation constructor but unserialized message is not used by DeprecationErrorHandler. Tried to fix it in#31382
…nErrorHandler refacto (l-vo)This PR was merged into the 4.3-dev branch.Discussion----------[PhpunitBridge] Fix not unserialized logs after DeprecationErrorHandler refacto| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |During the refactoring for#29211 , it seems a little bug was introduced. When using runInSeparateProcess, deprecation message isn't unserialized anymore.Commits-------f522729 [PhpunitBridge] Fix not unserialized logs after DeprecationErrorHandler refactoring
| } | ||
| returnfalse; | ||
| } |
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.
@greg0ire what is the goal of this method please ? Is it only used for the color of the deprecation text or I missed another usage ? I saw some minors problems (see#31478 (WIP)). When usingrunInSeparateProcess, it's not the correct stack trace that is used to determineisIndirect result. I'm not sure how fix it. In some cases, I will not be able to serialize the deprecation original stack trace (when there is a closure). What would be (from your point of view) the consequences if isIndirect can't be determined ?
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.
The goal is to help making a difference between direct and indirect. If the result is inaccurate, a test suite might fail when it should not and vice versa. I'm on hols, and answering from my phone while walking in the street so my answer might be inaccurate too :P
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.
IIRC there is a group of "others" deprecations, maybe we should throw an exception when unable to answer, and catch it and then put the deprecation in that group
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 thought too about this kind of solution. To accept to not be able to determine direct/indirect in some edge cases (runInSeparateProcess and unserializable trace in this case). Thank you for your help 👍
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.
See implementation of this kind of solution here:#31730
…ire, llaakkkk)This PR was submitted for the master branch but it was merged into the 4.3 branch instead (closes#11323).Discussion----------Document changes in the deprecation error handlerSeesymfony/symfony#29211Seesymfony/symfony#28048Closes#10701Commits-------64141fc fixup! Document changes in the deprecation error handlerdd7eb0b fixup! Document changes in the deprecation error handlerb2a7744 fixup! Document changes in the deprecation error handler988badf fixup! Document changes in the deprecation error handler2a750ea Document changes in the deprecation error handler
Uh oh!
There was an error while loading.Please reload this page.
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:
strictQuiet configuration
Default configuration