Add support for Whoops 2.0 #384
Conversation
|
@michaelmoussa Could you review this code please. If this is a valid solution I'll fix the tests too. I don't want to spend time on the tests if the solution isn't good. |
There was a problem hiding this comment.
Looks pretty good to me! As a possible enhancement, what do you think of automatically using whichever handler is most appropriate for the Accept header? json for json, xml for xml, plaintext if we're in CLI, or prettypage otherwise?
I don't think that's necessary for this PR, but just a suggestion if you're interested. I think what you have already can be merged just fine once the tests are added.
Thanks
| @@ -76,9 +76,18 @@ public function __construct( | |||
| */ | |||
| protected function handleException($exception, Request $request, Response $response) | |||
| { | |||
| $this->prepareWhoopsHandler($request); | |||
| // Push the whoops handler if any | |||
| if (null !== $this->whoopsHandler) { | |||
There was a problem hiding this comment.
Can you reverse the order here for the comparison for consistency? I haven't seen the Yoda condition style being used elsewhere in the project.
|
The tests are now compatible for Whoops 1.x and 2.x. For Expressive version 2 I suggest to drop whoops 1.x support. All Whoops related tests can then be rewritten and properly use the Whoops\RunInterface (this isn't available in Whoops 1.x). One more thing I discovered: The Zend\Expressive\WhoopsErrorHandler expects as the second parameter the PrettyPageHandler. However there is also a Whoops\Handler\HandlerInterface. This would allow to inject any error handler like the JsonResponseHandler without breaking BC. |
|
Great! I'll see about merging and doing a patch release early next week, as I won't have access to a computer for a few days... unless @weierophinney or someone else wants to wrap things up. |
|
@weierophinney this looks good to me, but I noticed that |
|
This is working well for me :) Would be good to see it merged. |
Add support for Whoops 2.0 Conflicts: src/Container/WhoopsErrorHandlerFactory.php
Forward port #384 Conflicts: composer.json
|
Thanks, @xtreamwayz |
This PR adds support for Whoops 2.0 as discussed in zendframework/zend-expressive-skeleton#115 and #383.
Falling back to 1.1 is no option since it doesn't support PHP 7.
It's not optimal but it adds support fro 2.0 while maintaining BC.