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

Fixes #23, thanks to @garoevans for reporting the issue, and fixing soon... #24

Closed
wants to merge 2 commits into from

Conversation

harikt
Copy link
Member

@harikt harikt commented Jan 17, 2014

...er than later

@pmjones
Copy link
Member

pmjones commented Jan 17, 2014

Good catch, and a good start. It needs to also check CONTENT_LENGTH and CONTENT_MD5.

@harikt
Copy link
Member Author

harikt commented Jan 18, 2014

@pmjones done. Have a look.

@pmjones
Copy link
Member

pmjones commented Jan 18, 2014

Argh -- I think this is in the wrong place.

First off, look at https://tools.ietf.org/rfc/rfc3875.txt -- this is what PHP follows. There are no such keys as HTTP_(CONTENT_TYPE|CONTENT_LENGTH) so checking for HTTP_* versions of them makes no sense. I was wrong to mention CONTENT_MD5 as it's not listed there, so that would be HTTP_CONTENT_MD5 (I guess).

The issue appears to be in the "headers" class, not the "content" class. The loop there looks only for HTTP_* headers and does not track the CONTENT_* headers.

Sorry for the back-and-forth, man. :-/

@pmjones
Copy link
Member

pmjones commented Jan 18, 2014

Here's another note about it: http://php.net/manual/en/reserved.variables.server.php#110763

@pmjones
Copy link
Member

pmjones commented Jan 18, 2014

@harikt I think this will do the trick: bba1e67

Let me know if you think something else has to happen.

@harikt
Copy link
Member Author

harikt commented Jan 18, 2014

I am running with php version 5.4, not sure your php version.

The php's built in server when you have a Content-Type: application/json will not give any result for your query $request->content->getType(); for $_SERVER is having HTTP_CONTENT_TYPE and not CONTENT_TYPE . If you check with apache, then you are right.

I do believe that is true for CONTENT_MD5 .

This is my code

<?php
require __DIR__ . '/vendor/autoload.php';
use Aura\Web\WebFactory;
$web_factory = new WebFactory($GLOBALS);
$request = $web_factory->newRequest();
echo "Hello :) " . $request->content->getType();

echo "<br />\n";

echo isset($_SERVER['CONTENT_TYPE']) ? $_SERVER['CONTENT_TYPE'] : ' none ';

echo "<br />\n";

echo isset($_SERVER['HTTP_CONTENT_TYPE']) ? $_SERVER['HTTP_CONTENT_TYPE'] : ' http none';

echo "<br />\n";

echo isset($_SERVER['CONTENT_MD5']) ? $_SERVER['CONTENT_MD5'] : ' none md5';

echo "<br />\n";

When you use curl -H "Content-Type: application/json" http://localhost:8000 what is the result you are getting?

If it is

Hello :) <br />
 none <br />
application/json<br />
 none md5<br />
 http none md5

then we have not fixed.

Thanks

@pmjones
Copy link
Member

pmjones commented Jan 18, 2014

The php's built in server when you have a Content-Type: application/json will not give any result for your query $request->content->getType(); for $_SERVER is having HTTP_CONTENT_TYPE and not CONTENT_TYPE . If you check with apache, then you are right.

I wonder if that's a bug in the PHP 5.4 server; the RFC states it should be CONTENT_TYPE and not HTTP_CONTENT_TYPE. Maybe we do need to make an allowance there, as you noted earlier.

@pmjones
Copy link
Member

pmjones commented Jan 18, 2014

Yes, I'm seeing the behavior you described. I'm on PHP 5.4.22 -- you?

@harikt
Copy link
Member Author

harikt commented Jan 18, 2014

Yes I am in PHP 5.4.22 .

@pmjones
Copy link
Member

pmjones commented Jan 18, 2014

/me nods

I ask because I went to report the bug, but they won't take bugs on anything other than the latest versions (5.4.24 in this case). I don't have the time to upgrade any time soon -- do you?

@harikt
Copy link
Member Author

harikt commented Jan 18, 2014

Sorry I too don't think so :( .

As this is built in server may be a note is good?.

@pmjones
Copy link
Member

pmjones commented Jan 18, 2014

/me nods again

OK, so I'll merge this change too, and that at least will make an allowance for the bug. Thanks man!

@jelofson
Copy link
Member

I can confirm that php 5.5.3's built in web server does use HTTP_CONTENT_TYPE instead of CONTENT_TYPE.

I just did a var_dump of $_SERVER with curl -H "Content-Type: application/json" http://localhost:8000, but without Aura.Web.

@harikt
Copy link
Member Author

harikt commented Jan 19, 2014

Thank you @jelofson .

@garoevans
Copy link
Contributor

Are you going to attempt to check for 'HTTP_CONTENT_TYPE' just for the use case, or not worth it?

I'm only using the built in server for quick local setup purpose, but have the following in the top of my define.php:

// TODO: remove little hack once https://github.com/auraphp/Aura.Web/issues/23 is resolved
$_SERVER['CONTENT_TYPE'] = $_SERVER['HTTP_CONTENT_TYPE'];

Maybe I should just take the 5 mins to setup a vhost for it on apache :)

@harikt
Copy link
Member Author

harikt commented Jan 20, 2014

@garoevans as @pmjones mentioned the fix is only for built in server, but not sure whether it is worth. ( Reason explained by Paul )

@pmjones
Copy link
Member

pmjones commented Jan 31, 2014

This appears to be an issue with the PHP web server, not with Aura.Web per se. I have reported it to the PHP folks at https://bugs.php.net/bug.php?id=66606. I don't think it's worthwhile yet to "fix" this in Aura.Web just yet, but we can revisit that assessment later if we need to.

@pmjones pmjones closed this Jan 31, 2014
@harikt harikt deleted the issue23 branch February 1, 2014 01:15
@harikt
Copy link
Member Author

harikt commented Feb 1, 2014

Thanks Paul .

@harikt harikt restored the issue23 branch July 27, 2014 02:08
@harikt harikt reopened this Jul 27, 2014
@harikt harikt closed this Sep 5, 2014
@harikt harikt deleted the issue23 branch September 5, 2014 05:57
@harikt
Copy link
Member Author

harikt commented Sep 7, 2014

I cleaned my branches again. That is why the branch got lost. Sorry.

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