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

Don't override content-type if it's already set #166

Merged
merged 1 commit into from Jul 23, 2018

Conversation

davispuh
Copy link
Contributor

JsonStrategy might return different content-type which we don't want to be overridden.

Also there can only be one content-type header so use withHeader

Otherwise currently using withAddedHeader we would add 2 'content-type' headers with undefined priority.

JsonStrategy might return different content-type which we don't want to be overridden.

Also there can only be one "content-type" header so use withHeader
@hannesvdvreken
Copy link
Contributor

I'm afraid this change is not something everyone will want. If the existing JsonStrategy doesn't fit your needs, you're free to implement your own and pass that on to the RouteCollection.

@davispuh
Copy link
Contributor Author

I don't see any downsides. If your route doesn't set any content-type then nothing changes they'll get same application/json but if they set content-type to something else then I would expect it to be used instead of being overridden which may seem surprising.

For example if you've JSON API for file downloads then in error case you may want response with content-type: application/json but in success case actual file contents such as content-type: application/pdf

@hannesvdvreken
Copy link
Contributor

I see. But these basic strategies are just examples. And they only work in very simple use cases, which your case is not: yours is very elaborate and sounds well thought out.

Your contribution is very welcome though, don't get me wrong :-) it's just that - I believe (and that's just me, I am not the maintainer or anything like that) - that the purpose of this simple class shouldn't be to fit everyone's use case.

I'd like to encourage you to use the flexibility that this package provides and to create your own strategies.

@davispuh
Copy link
Contributor Author

I already did that and subclassed it this way but that doesn't seem as nice as implemented here as had to copy whole getCallable method just to change 2 lines.

@frankdejonge
Copy link
Member

@hannesvdvreken sorry to barge in 🤪I think this PR is pretty legit. Custom json mimetypes are allowed and I've seen it a lot more often these days. This PR ensures people can use what the spec allows in a backwards compatible way. It's got my 👍 (but I'm not a regular contributor to this project so my opinion is worth as much as anyone else's)

@philipobenito philipobenito merged commit 216b022 into thephpleague:master Jul 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants