[HttpFoundation] Get response content as resource several times for PHP >= 5.6 #14738

dunglas · May 24, 2015

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	n/a

Since PHP 5.6, php://input can be opened several times.

…HP > 5.6

fabpot · May 26, 2015

👍

nicolas-grekas · May 26, 2015

This is tricky: this will make apps behave differently based on the PHP version. Could e.g. work on dev, not on prod env. Also, is it really a bug fix? It looks like a new feature to me.
I don't know if it's possible, but if yes, I'd prefer finding a workaround that makes this a new feature working on any PHP version (thus on 2.8).

dunglas · May 27, 2015

It was a (painful) limitation in PHP and I think this check was added to help users understanding what is happening (instead of just returning an empty string - the old default behavior of PHP). That PHP limitation is gone, so IMO we can tell it's a bug fix.

The only way I see is changing the method signature to getContent($asResource = false, $allowReuseResource = false) but it looks ugly.

Applications behaving differently depending of the PHP version will be a edge case:

having different PHP versions in prod and in dev (already a bad idea)
a server running on PHP < 5.6 (5.6 is now the PHP version distributed in Debian stable)
getting twice the php://input (and keep in mind that with or without Symfony, it will not work on PHP <= 5.6)

In many case this can provide a performance boost. Look the PSR-7 bridge for instance: https://github.com/symfony/psr-http-message-bridge/pull/1/files#diff-7e5b3956b6ae2a5924719b9d4eecb8cdR47

nicolas-grekas · May 27, 2015

Ok, I did some research on this and I'm 👍 ...

nicolas-grekas · May 27, 2015

src/Symfony/Component/HttpFoundation/Request.php

@@ -1459,7 +1459,7 @@ public function isMethodSafe()
     */
    public function getContent($asResource = false)
    {
-        if (false === $this->content || (true === $asResource && null !== $this->content)) {
+        if (PHP_VERSION_ID < 50600 && (false === $this->content || (true === $asResource && null !== $this->content))) {
            throw new \LogicException('getContent() can only be called once when using the resource return type.');


What about adding the PHP version requirement for this exception?
Request->getContent() can only be called once when using the resource return type and PHP below 5.6.

stof · May 28, 2015

src/Symfony/Component/HttpFoundation/Tests/RequestTest.php

+        if (PHP_VERSION_ID < 50600) {
+            $this->markTestSkipped('PHP < 5.6 does not allow to open php://input several times.');
+        }
+
        $req = new Request();
        $req->getContent($first);
        $req->getContent($second);


you don't have any assertion here, so there is nothing ensuring that it worked

After looking again, this is intended. Here we test if a exception is thrown or not, not the returned content.

the point is, you don't check anything here

if you run this test on 5.3 after removing the condition in the code, it will still work even if the second call always gives an empty string. So it does not ensure that the feature actually works when calling it a second time

On 5.3 if you remove the markTestSkipped() call it will fail because a \LogicException is thrown.
The only thing that this test do is checking that we don't throw a \LogicException when it's not necessary (on PHP 5.6+).

I try to explain me better:
if you run that test on PHP 5.6 without the patch, this test will fail because a \LogicException will be thrown at the second call.

tests should have one assertion at least. otherwise they are marked as risky in phpunit strict mode

Assertion added on the other test (this one was preexisting and as an @expectedException annotation).

nicolas-grekas · Jun 4, 2015

src/Symfony/Component/HttpFoundation/Tests/RequestTest.php

        $req = new Request();
        $req->getContent($first);
        $req->getContent($second);
    }

+    /**
+     *


blank line to be removed :)

fabpot · Jun 5, 2015

Thank you @dunglas.

…l times for PHP >= 5.6 (dunglas) This PR was squashed before being merged into the 2.3 branch (closes #14738). Discussion ---------- [HttpFoundation] Get response content as resource several times for PHP >= 5.6 | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Since PHP 5.6, `php://input` can be opened several times. Commits ------- 9f9b0f7 [HttpFoundation] Get response content as resource several times for PHP >= 5.6

dunglas changed the title ~~Allow getting php://input several times~~ [HttpFoundation] Allow getting php://input several times May 24, 2015

[HttpFoundation] Get response content as resource several times for P…

6a88664

…HP > 5.6

dunglas force-pushed the reusable_php_input branch from b750f5e to 6a88664 Compare May 24, 2015 20:28

dunglas changed the title ~~[HttpFoundation] Allow getting php://input several times~~ [HttpFoundation] Get response content as resource several times for PHP > 5.6 May 24, 2015

dunglas added the Ready label May 25, 2015

dunglas changed the title ~~[HttpFoundation] Get response content as resource several times for PHP > 5.6~~ [HttpFoundation] Get response content as resource several times for PHP >= 5.6 May 26, 2015

nicolas-grekas reviewed May 27, 2015
View reviewed changes

[HttpFoundation] Clarified resource exception message.

07b0fc4

stof reviewed May 28, 2015
View reviewed changes

stof removed the Ready label May 28, 2015

[HttpFoundation] Add an assertion in resource test.

c0d6356

nicolas-grekas reviewed Jun 4, 2015
View reviewed changes

fabpot closed this Jun 5, 2015

dunglas deleted the reusable_php_input branch December 5, 2015 09:02

Search code, repositories, users, issues, pull requests...

Uh oh!

[HttpFoundation] Get response content as resource several times for PHP >= 5.6 #14738

[HttpFoundation] Get response content as resource several times for PHP >= 5.6 #14738

Uh oh!

Conversation

dunglas commented May 24, 2015

Uh oh!

fabpot commented May 26, 2015

Uh oh!

nicolas-grekas commented May 26, 2015

Uh oh!

dunglas commented May 27, 2015

Uh oh!

nicolas-grekas commented May 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Jun 5, 2015

Uh oh!

Uh oh!