Fix PHP7 error handler compatibility. #11518
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11518 +/- ##
============================================
- Coverage 93.38% 93.36% -0.02%
- Complexity 13028 13034 +6
============================================
Files 436 436
Lines 32775 32848 +73
============================================
+ Hits 30606 30669 +63
- Misses 2169 2179 +10
Continue to review full report at Codecov.
|
|
Any chance we could add a test for the scenario this is handling? |
|
I added a test. |
|
Thank you for helping with the test case :) |
|
|
||
| if ($this->getConfig('skipLog')) { | ||
| foreach ((array)$this->getConfig('skipLog') as $class) { | ||
| if ($unwrapped instanceof $class) { |
There was a problem hiding this comment.
Do tests need to be updated for logging fixes?
There was a problem hiding this comment.
This is basically just a copy and paste of the old error handler. But could sure be added.
Would probably be better to follow @saeideng s advice and re-use more common code instead of duplicating in the long run. But for now out of scope of the bugfix.
There was a problem hiding this comment.
Actually this change isn't necessary at all. logException() never get's Error instance wrapped inside PHP7ErrorException. The wrapping is only done inside getRenderer() when passing the exception instance to ExceptionRender's constructor.
There was a problem hiding this comment.
Are you 100% sure? I am also referring to any outside or custom handler extending the middleware?
With the above change I just wanted to make double sure nothing goes wrong if it ever would get such an exception passed.
There was a problem hiding this comment.
The unwrapping is surely not needed inside logException() with current state of middleware code. If some extending middleware needs it (I don't see why) then it's up to them to update their code as required.
There was a problem hiding this comment.
I think you are mistaken. The incoming one can very well be PHP7+ or a wrapped one afaik. The wrapping currently only happens inside getRenderer() and thus does not affect the variable inside handleException(), that is correct. But it could already be passed wrapped, correct? And that means that the logging should be aware of both versions.
As such you should revert as that code piece is very well required and should stay both for the class itself as for any extension to function properly.
Short version: It doesnt hurt to keep IMO.
But I am fine either way.
Possible solution of #11514
We convert it back to the PHP5 exception where needed as the constructor for ExceptionRenderer exceptions Exception which is not compatible with PHP7.
Removing the typehint here can only be done in a non-patch version as the eco system could implement them still as such.