Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

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

Open
0b10011 opened this issue Dec 7, 2017 · 3 comments
Labels

Comments

@0b10011
Copy link

0b10011 commented Dec 7, 2017

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

$server = static::normalizeServer($server ?: $_SERVER);
$files = static::normalizeFiles($files ?: $_FILES);

$cookies ?: $_COOKIE,
$query ?: $_GET,
$body ?: $_POST,

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.)

@Xerkus
Copy link
Member

Xerkus commented Dec 7, 2017

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 Xerkus added the bug label Jan 6, 2018
@weierophinney
Copy link
Member

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
Copy link
Member

This repository has been closed and moved to laminas/laminas-diactoros; a new issue has been opened at laminas/laminas-diactoros#12.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants