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

Do not assume CONTENT_TYPE is available, fall back to CakeResponse::type() #1661

Closed
wants to merge 5 commits into from

Conversation

sime
Copy link
Contributor

@sime sime commented Sep 20, 2013

Not all servers will provide a CONTENT_TYPE environment variable.

In such a situation, the user is required to decode POST data explicitly with CakeRequest::input().

This patch falls back to CakeResponse::type() to determine the content type when the existing check provides null.

@Phally
Copy link
Contributor

Phally commented Sep 20, 2013

Could you please add a test for this and do you have an example of a server that doesn't include CONTENT_TYPE?

@@ -518,6 +518,9 @@ public function requestedWith($type = null) {
}

list($contentType) = explode(';', env('CONTENT_TYPE'));
if (is_null($contentType)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$contentType === null please.

@markstory
Copy link
Member

When is content type missing?

@markstory markstory closed this Sep 20, 2013
@markstory markstory reopened this Sep 20, 2013
@sime
Copy link
Contributor Author

sime commented Sep 20, 2013

cli-server does not have CONTENT_TYPE on POST, instead it can be found in HTTP_CONTENT_TYPE.

I'll see if I can drum up a test.

@markstory
Copy link
Member

Interesting. The CLI server continues to be a fountain of strange :)

@@ -518,6 +518,9 @@ public function requestedWith($type = null) {
}

list($contentType) = explode(';', env('CONTENT_TYPE'));
if ($contentType === null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explode() always returns an array of strings, so list() will most likely never be able to assign $contentType to null. Tests should proof that if ($contentType === '') {} might be required here.

@sime
Copy link
Contributor Author

sime commented Sep 21, 2013

So the reason that CONTENT_TYPE is not available is because it is a CGI/1.1 requirement and as far as I can tell CLI PHP executables are built without CGI.

Both Apache and FPM/FastCGI servers have a GATEWAY_INTERFACE of CGI/1.1.

@Phally
Copy link
Contributor

Phally commented Sep 27, 2013

Could you squash all those commits into one please? Then we can merge them. Looks good to me. 👍

markstory pushed a commit that referenced this pull request Oct 1, 2013
In some server environments notably the CLI server, _SERVER['CONTENT_TYPE'] is not available.
In these cases, fall back to the HTTP_CONTENT_TYPE header.

Refs #GH-1661
@markstory
Copy link
Member

Squashed and merged in b1ea2d3 Thanks 👍

@markstory markstory closed this Oct 1, 2013
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