Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

Allow connections that do not contain accept header #58

Closed
wants to merge 1 commit into from
Closed

Allow connections that do not contain accept header #58

wants to merge 1 commit into from

Conversation

ddubau
Copy link
Contributor

@ddubau ddubau commented Dec 2, 2015

Updated accept filter to allow connections that do not contain the accept header field. This causes issues with integrating with some services such as GitLab and also does not follow the W3C recommendation.

http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html
Under 14.1: “If no Accept header field is present, then it is assumed that the client accepts all media types. If an Accept header field is present, and if the server cannot send a response which is acceptable according to the combined Accept field value, then the server SHOULD send a 406 (not acceptable) response."

@TomHAnderson
Copy link
Contributor

👎 This is against the long standing opinion(ated) that the request contain an accept header.

@ddubau
Copy link
Contributor Author

ddubau commented Dec 3, 2015

I would suggest that we put opinions aside and follow best development practice which is to adhere to protocol standards. Since there are services that are being used that do not follow this opinion there will be issues with this approach. The fact of the matter is not even all modern software clients include the accept header. I suggest we attempt to make the framework as open as possible to be able to interface with other software.

@TomHAnderson
Copy link
Contributor

From the first announcement of Apigility (not linked below) it has been called opinionated.
http://framework.zend.com/blog/apigility-1-0-0-released.html

@erik-maas
Copy link

I would also love to see this change.
It would render this lib usefull when third party services like gitlab or adyen would be able to call your application.

Instead, it's annoying and hinders application development by not accepting the missing accept header.

Related issue:
#62

@weierophinney weierophinney added this to the 1.1.2 milestone May 26, 2016
@weierophinney weierophinney self-assigned this May 26, 2016
weierophinney added a commit that referenced this pull request May 26, 2016
weierophinney added a commit that referenced this pull request May 26, 2016
weierophinney added a commit that referenced this pull request May 26, 2016
@stefanotorresi
Copy link

stefanotorresi commented Jun 1, 2016

While I understand the reasons for introducing this change, and I support them, the final result is an inconsistent behaviour in the module as a whole.

Prior to this change, resources consistently returned a 406 response to any request with no Accept header, now they do so only for some requests. This is due to the other listener handling the Accept header, ZF\ContentNegotiation\AcceptListener, which is attached to post-dispatch and it only comes into place if no other listener short-circuits the cycle.

This results with all sorts if unpredictable behaviours in a full Apigility application, because for example a POST without the Accept header will recieve a 422 if data doesn't pass content validation, but a 406 if it does.

There is a mechanism to return a default view model already, but I'm not 100% clear under which conditions it is selected.

@weierophinney can this be solved via configuration or should we update AcceptListener too and loosen its strictness?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants