zend-diactoros icon indicating copy to clipboard operation
zend-diactoros copied to clipboard

`ServerRequestFactory::fromGlobals()` checks if args are truthy rather than set

Open 0b10011 opened this issue 6 years ago • 3 comments

In ServerRequestFactory::fromGlobals(), most of the code checks if the arguments are truthy, rather than set. For example:

https://github.com/zendframework/zend-diactoros/blob/89d471cc09850c7d47839cf16ba7a1fed54d45e5/src/ServerRequestFactory.php#L57-L58

https://github.com/zendframework/zend-diactoros/blob/89d471cc09850c7d47839cf16ba7a1fed54d45e5/src/ServerRequestFactory.php#L72-L74

This becomes an issue during testing (or a long-running process that accepts multiple requests). For example:

// In a previous test
$_POST = ['foo' => 'bar'];

// In current test
$request = ServerRequestFactory::fromGlobals(null, null, []);
echo serialize($request->getParsedBody());
// Expected: a:0:{}
// Actual: a:1:{s:3:"foo";s:3:"bar";}

Improved versions would be:

        $server  = static::normalizeServer($server ?? $_SERVER); 
        $files   = static::normalizeFiles($files ?? $_FILES); 
            $cookies ?? $_COOKIE, 
            $query ?? $_GET, 
            $body ?? $_POST, 

Or for under PHP 7:

        $server  = static::normalizeServer(isset($server) ? $server : $_SERVER); 
        $files   = static::normalizeFiles(isset($files) ? $files : $_FILES); 
            isset($cookies) ? $cookies : $_COOKIE, 
            isset($query) ? $query : $_GET, 
            isset($body) ? $body : $_POST, 

(This code should fix the issues, but I didn't have time to write any tests right now, so I'm opening an issue instead of a pr.)

0b10011 avatar Dec 07 '17 22:12 0b10011

fromGlobals() is meant to be used for requests parsed by php and injected into process as global variables. That rules out multiple requests in long running processes or requests assembled for testing.

Other than apparent misuse, this looks like a legit issue.

Xerkus avatar Dec 07 '17 23:12 Xerkus

I would definitely consider a patch that uses the isset-ternary approach outlined above. That said, as noted by @Xerkus, this is a rare possibility in standard usage.

weierophinney avatar Jul 09 '18 21:07 weierophinney

This repository has been closed and moved to laminas/laminas-diactoros; a new issue has been opened at https://github.com/laminas/laminas-diactoros/issues/12.

weierophinney avatar Dec 31 '19 22:12 weierophinney