slimphp / Slim-Psr7

PSR-7 implementation for use with Slim 4
MIT License
133 stars 45 forks source link

Bug with empty PSR-7 body? #205

Closed badmansan closed 3 years ago

badmansan commented 3 years ago

I have the latest Slim version (at this time 4.7.1)

The code:

<?php

use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface;

require __DIR__ . '/../vendor/autoload.php';

$app = \Slim\Factory\AppFactory::create();
$app->addErrorMiddleware(TRUE, TRUE, TRUE);

$serverRequestCreator = \Slim\Factory\ServerRequestCreatorFactory::create();
$request = $serverRequestCreator->createServerRequestFromGlobals();

$app->post('/', function (ServerRequestInterface $request, ResponseInterface $response) {
    $input = file_get_contents('php://input'); // {"t":1}

    $clone = $request->withHeader('X-SomeHeader', 'foo');
    $cloneContent = $clone->getBody()->getContents(); // {"t":1}

    $c1 = $request->getBody()->getContents(); // "" ?!
    $c2 = $request->getBody()->getContents(); // {"t":1}
    $c3 = $request->getBody()->getContents(); // {"t":1}
    $c4 = $request->getBody()->getContents(); // {"t":1}

    $body = 'input: ' . var_export($input, TRUE) . PHP_EOL
        . 'cloneContent: ' . var_export($cloneContent, TRUE) . PHP_EOL
        . 'c1: ' . var_export($c1, TRUE) . PHP_EOL
        . 'c2: ' . var_export($c2, TRUE) . PHP_EOL
        . 'c3: ' . var_export($c3, TRUE) . PHP_EOL
        . 'c4: ' . var_export($c4, TRUE) . PHP_EOL;

    $response->getBody()->write($body);

    return $response;
});

$response = $app->handle($request);
$responseEmitter = new \Slim\ResponseEmitter();
$responseEmitter->emit($response);

Expected:

input: '{"t":1}'
cloneContent: '{"t":1}'
c1: '{"t":1}'
c2: '{"t":1}'
c3: '{"t":1}'
c4: '{"t":1}'

Got:

input: '{"t":1}'
cloneContent: '{"t":1}'
c1: ''
c2: '{"t":1}'
c3: '{"t":1}'
c4: '{"t":1}'

Why c1 is empty?

odan commented 3 years ago

Hi @badmansan

PS: For questions like this we have a discourse forum.

badmansan commented 3 years ago

I know about the forum, but it looks like a bug.

It is an original Slim\Psr7\Request. If I use (string) than the content in second and all next getBody() doubles:

input: '{"t":1}'
cloneContent: '{"t":1}'
c1: '{"t":1}'
c2: '{"t":1}{"t":1}'
c3: '{"t":1}{"t":1}'
c4: '{"t":1}{"t":1}'

If we use nyholm/psr7 implementation instead slim/psr7 then all work as expected in case (string) $request->getBody() using:

<?php

use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface;

require __DIR__ . '/../vendor/autoload.php';

$app = \Slim\Factory\AppFactory::create();
$app->addErrorMiddleware(TRUE, TRUE, TRUE);

$psr17Factory = new \Nyholm\Psr7\Factory\Psr17Factory();
$serverRequestCreator = new \Nyholm\Psr7Server\ServerRequestCreator($psr17Factory, $psr17Factory, $psr17Factory, $psr17Factory);
$request = $serverRequestCreator->fromGlobals();

$app->post('/', function (ServerRequestInterface $request, ResponseInterface $response) {
    $input = file_get_contents('php://input'); // {"t":1}

    $clone = $request->withHeader('X-SomeHeader', 'foo');
    $cloneContent = (string) $clone->getBody(); // {"t":1}

    $c1 = (string) $request->getBody(); // {"t":1}
    $c2 = (string) $request->getBody(); // {"t":1}
    $c3 = (string) $request->getBody(); // {"t":1}
    $c4 = (string) $request->getBody(); // {"t":1}

    $body = 'input: ' . var_export($input, TRUE) . PHP_EOL
        . 'cloneContent: ' . var_export($cloneContent, TRUE) . PHP_EOL
        . 'c1: ' . var_export($c1, TRUE) . PHP_EOL
        . 'c2: ' . var_export($c2, TRUE) . PHP_EOL
        . 'c3: ' . var_export($c3, TRUE) . PHP_EOL
        . 'c4: ' . var_export($c4, TRUE) . PHP_EOL;

    $response->getBody()->write($body);

    return $response;
});

$response = $app->handle($request);
$responseEmitter = new \Slim\ResponseEmitter();
$responseEmitter->emit($response);

Result:

input: '{"t":1}'
cloneContent: '{"t":1}'
c1: '{"t":1}'
c2: '{"t":1}'
c3: '{"t":1}'
c4: '{"t":1}'
l0gicgate commented 3 years ago

That's weird. We should write an appropriate test case to try and duplicate this to take isolate the issue so we can fix it.

odan commented 3 years ago

I have tried to reproduce this Bug on my machine and I can confirm this behavior.

Example 1: With empty, then normal content

$clone = $request->withHeader('X-SomeHeader', 'foo');
$cloneContent = (string)$clone->getBody(); // {"t": 1}

$c1 = $request->getBody()->getContents(); // ""
$c2 = $request->getBody()->getContents(); // {"t": 1}
$c3 = $request->getBody()->getContents(); // {"t": 1}

Example 2: With normal, then doubled content

$clone = $request->withHeader('X-SomeHeader', 'foo');
$cloneContent = (string)$clone->getBody(); // {"t": 1}

$c1 = (string)$request->getBody(); // {"t": 1}
$c2 = (string)$request->getBody(); // {"t": 1}{"t": 1}
$c3 = (string)$request->getBody(); // {"t": 1}{"t": 1}

Example 3: Everything normal

$clone = $request->withHeader('X-SomeHeader', 'foo');

$c1 = (string)$request->getBody(); // {"t": 1}
$c2 = (string)$request->getBody(); // {"t": 1}
$c3 = (string)$request->getBody(); // {"t": 1}
l0gicgate commented 3 years ago

Moving this issue to the Slim-Psr7 repo as this is not a Slim core issure

l0gicgate commented 3 years ago

So I found the issue for this and wrote a test case:

public function testBodyIsRewoundAfterContentsAreAccessed()
{
    $contents = 'test';

    $resource = fopen('php://temp', 'rw+');
    fwrite($resource, $contents);
    rewind($resource);

    $cache = fopen('php://temp', 'wb+');
    $cacheStream = new Stream($cache);

    $body = (new StreamFactory())->createStreamFromResource($resource, $cacheStream);

    $request = $this->requestFactory();
    $request = $request->withBody($body);

    $clone = $request->withHeader('x-header', 'value');

    $this->assertEquals($contents, $clone->getBody()->getContents());
    $this->assertEquals($contents, $request->getBody()->getContents());
}

The problem lies with the PSR-7 standard and not being able to mutate stream objects. So the way most of the withX methods work is that they use $clone = clone $this and return the cloned object.

Those clones share the same resource objects from original to clone. Which is why when you call $clone->getBody()->getContents() you get the contents and subsequently calling $request->getBody()->getContents() returns an empty string. The order is interchangeable as well, if you call $request->getBody()->getContents() first you will get the contents, then if you call $clone->getBody()->getContents() you will get an empty string.

PSR-7 Integration tests have the following test for the getContents() method:

public function testGetContents()
{
    if (isset($this->skippedTests[__FUNCTION__])) {
        $this->markTestSkipped($this->skippedTests[__FUNCTION__]);
    }

    $resource = fopen('php://memory', 'rw');
    fwrite($resource, 'abcdef');
    $stream = $this->createStream($resource);
    $stream->rewind();

    $stream->seek(3);
    $this->assertEquals('def', $stream->getContents());
    $this->assertSame('', $stream->getContents());
}

The only way to circumvent this is to rewind the stream whenever getContents() gets called. Unfortunately, that breaks the tests so we can't do that:

screenshot

I'm closing this as won't fix as we would not be respecting the PSR-7 standards by rewinding the stream after getContents()