[PhpUnitBridge] Url encoded deprecations helper config #29211
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
308d9b0 to
586053a
Compare
OskarStark
approved these changes
Nov 17, 2018
d010f02 to
4c7adb1
Compare
Why when you are targetting master? |
|
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 in f9414a8 |
greg0ire
added a commit
to greg0ire/symfony-docs
that referenced
this pull request
Nov 24, 2018
|
Wait @nicolas-grekas , is it really 5.3, not 5.5? Did you maybe tell me we could bump to 5.5? |
|
5.5 will be good soon because 2.8 will be out of maintenance in 1 week. |
eea83c2 to
9c62a77
Compare
|
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 to
14fd0e1
Compare
greg0ire
commented
Apr 10, 2019
src/Symfony/Bridge/PhpUnit/Tests/DeprecationErrorHandler/default.phpt
Outdated
Show resolved
Hide resolved
9251b1a to
1006b12
Compare
1006b12 to
1c73f9c
Compare
nicolas-grekas
approved these changes
Apr 12, 2019
|
Thank you @greg0ire. |
fabpot
added a commit
that referenced
this pull request
Apr 12, 2019
… (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  # Default configuration  Commits ------- 1c73f9c [PhpUnitBridge] Url encoded deprecations helper config
stof
reviewed
Apr 12, 2019
| return false !== 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.
this is broken if there is no such threshold
There was a problem hiding this comment.
It is always initialized to 999999, see the ctor
| return false; | ||
| } | ||
| foreach (['self', 'direct', 'indirect'] as $deprecationType) { | ||
| if ($deprecationCounts['remaining '.$deprecationType.'Count'] > $this->thresholds[$deprecationType]) { |
There was a problem hiding this comment.
this is broken if there is no such threshold
Merged
nicolas-grekas
added a commit
that referenced
this pull request
Apr 12, 2019
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 @greg0ire Commits ------- b11585e [PhpUnitBridge] fixes
l-vo
reviewed
May 4, 2019
| $ErrorHandler = self::$utilPrefix.'ErrorHandler'; | ||
|
|
||
| return $ErrorHandler::handleError($type, $msg, $file, $line, $context); | ||
| } | ||
|
|
||
| $trace = debug_backtrace(); | ||
| $deprecation = new Deprecation($msg, debug_backtrace(), $file); |
There was a problem hiding this comment.
$msg is unserialized in Deprecation constructor but unserialized message is not used by DeprecationErrorHandler. Tried to fix it in #31382
nicolas-grekas
added a commit
that referenced
this pull request
May 5, 2019
…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
Merged
2 tasks
l-vo
reviewed
May 11, 2019
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
@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 using runInSeparateProcess, it's not the correct stack trace that is used to determine isIndirect 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.
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.
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.
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.
See implementation of this kind of solution here: #31730
javiereguiluz
pushed a commit
to symfony/symfony-docs
that referenced
this pull request
May 31, 2019
javiereguiluz
added a commit
to symfony/symfony-docs
that referenced
this pull request
May 31, 2019
…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 handler See symfony/symfony#29211 See symfony/symfony#28048 Closes #10701 Commits ------- 64141fc fixup! Document changes in the deprecation error handler dd7eb0b fixup! Document changes in the deprecation error handler b2a7744 fixup! Document changes in the deprecation error handler 988badf fixup! Document changes in the deprecation error handler 2a750ea Document changes in the deprecation error handler
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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