Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.x] Add content-type for the default error renderer #2751

Merged
merged 7 commits into from Jul 14, 2019

Conversation

@adriansuter
Copy link
Contributor

commented Jul 1, 2019

This PR addresses #2749.

There is one commit 419f9bd that is actually a fix (independent of the issue).

@coveralls

This comment has been minimized.

Copy link

commented Jul 1, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling ab1abe0 on adriansuter:patch-errorHandler into be94ac0 on slimphp:4.x.

@l0gicgate l0gicgate added the Slim 4 label Jul 1, 2019

@l0gicgate l0gicgate added this to the 4.0.0 milestone Jul 1, 2019

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

So one thing I noticed is that right now we're letting the request Accept header dictate which renderer is being used. Let's just take a real world example where I'm building an API and I want to force the renderer to ALWAYS be rendering in application/json, don't you agree that we should make the defaultErrorRenderer override the determined content type?

@adriansuter

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

That is absolutely true. In fact, it is the route that could define, what error content type to be expected. Or the user might even set the response content type based on a route param. For example, say we have a route that would usually return html, but if we add ?json to the url, then the content type should be json. How do we know in the error handler, what content type the user would like to return?

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

For example, say we have a route that would usually return html, but if we add ?json to the url, then the content type should be json. How do we know in the error handler, what content type the user would like to return?

This is why we used the Accept header in the first place.

So what I'm thinking is that we need to introduce two modes into the handler. First mode is detect which detects the content type (it would also be the default mode) and then a force mode which would force the content type to be what the user defines.

$errorHandler->forceContentType('application/json')

There's a few more things we need to do with this handler. Namely add default headers for the response since it creates an entirely new response and if you need CORS headers then you'd need to extend the ResponseEmitter in order to add headers to that response which kind of sucks. I'm not sure if we need to add setDefaultHeaders or perhaps include a way to add middleware so we can do something like:

$errorHandler->addResponseMiddleware(function (Response $response) {
    return $response->withHeader(...);
});
@adriansuter

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

Would it be possible to set the forced content type in a route handler? I'm asking because if you mix different content types in the same route handler (or between different route handlers), then it should be possible to do that. Or do you recommend that the client does have to set the accept header correctly in that case?

Concerning the response headers: one callback should be enough. Or do you think that there could be more than one "middleware"? Probably some sort of ErrorResponseHeaderRenderer or ErrorResponseHeaderHandler.

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

I don't think that's necessary. Also, I'm thinking that maybe we should modify the ResponseEmitter instead to give the opportunity to append default headers via there since all content that gets emitted gets funneled through there. Lets' just add the detect and the force mode to the error handler and I think we'll be good to go. We can dive into appending default headers in a separate PR.

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

Is this PR good to go @adriansuter? or are you going to add the detect/force mode?

@adriansuter

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

I will add the force mode.

@adriansuter

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2019

@l0gicgate The forceContentType() is added to the error handler. I think that this should be added to the docs somewhere.

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2019

Yes we should probably put it in the advanced error handling section

@l0gicgate l0gicgate merged commit c4bdfa0 into slimphp:4.x Jul 14, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

@adriansuter adriansuter deleted the adriansuter:patch-errorHandler branch Jul 16, 2019

@l0gicgate l0gicgate referenced this pull request Aug 1, 2019

Merged

Slim 4 Release #2769

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.