psr7
psr7 copied to clipboard
Nyholm
Should getQueryParams() have a default value in objects from PSR-17 factories?
I've discovered this behavior (not to say "bug" yet) when tried ServerRequestInterface validation against the OpenAPI schema via the league/openapi-psr7-validator library. An OpenAPI schema may require a query parameter to be present in the request. And the mentioned library validates the presence of the query parameter by using the getQueryParams() method of the ServerRequestInterface, proof: https://github.com/thephpleague/openapi-psr7-validator/blob/master/src/PSR7/Validators/QueryArgumentsValidator.php#L71-L72
And I support the choice of that library authors in that it's natural to expect the output of the getQueryParams() method to be populated with the query parameters from the URI. Although the interface allows to override them later via withQueryParams(...), so that the method output, and the actual URI query may desync eventually, I still think it should be pre-populated with the params from the URI initially anyway.
If you feel that I, and the league team are wrong, and nobody should expect this method to be reliable, I'll instead open an issue on library's side, linking this one to it.
slim/psr7 follows the behavior expected by the lib: https://github.com/slimphp/Slim-Psr7/blob/master/src/Request.php#L267-L281
I am not sure what the expected behaviour is here. PSR-17 factories make no mention of setting a value for the query string arguments on a PSR-7 Request. PSR-7 says that getQueryParams() will retrieve whatever was defined on the class instance, its value is completely dependent on how the PSR-7 instance is instantiated by the user’s application flow.
How are you instantiating the Request? How do you fill it? I think that is where the root of this problem lies, not with the logic in either this library or in the OpenAPI validator.
For Nyholm/psr7-server we have opted to always set $_GET as the value whenever fromGlobals() is used. This still makes sense to me. If the user chooses to use the fromGlobals() method all the information should be coming from PHP’s global variables. If your server does not fill $_GET, then that might be something to look at.
If fromArrays() is used, it is up to the user to set what they want the query parameters to be.
What would you expect the way forward to be from our side?
If you feel that I, and the
leagueteam are wrong, and nobody should expect this method to be reliable, I'll instead open an issue on library's side, linking this one to it.
The method is reliable. But it should only be expected to reliably retrieve the query params as they were defined when the PSR-7 instance was created. So it depends what it is you are looking to validate: the Uri object associated with the Request object, or the query params that were set on the Request object.
If the application after validation uses getQueryParams(), then obviously the validator should do so as well.
slim/psr7follows the behavior expected by the lib: https://github.com/slimphp/Slim-Psr7/blob/master/src/Request.php#L267-L281
It would be interesting to see if it works if you were to use slim/psr7 instead of Nyholm/psr7. Note that this library can take any PSR factories and is not specifically linked to the Nyholm/psr7 implementation. But because of the way Nyholm/ psr7-server works it probably wouldn’t work with Slim either. Once you call getQueryParams(), we have already set it to whatever $_GET is.
I hope this was able to give some insights!
I am not sure what the expected behaviour is here.
Unfortunately, the PSR-7 standard nor the PSR-17 standard, nor their meta documents do not mention it clearly enough. However, PSR-7 meta document mentions this:
Rationale for ServerRequestInterface ... Access to the query string arguments (usually encapsulated in PHP via the $_GET superglobal).
As a user of your lib, I think it's natural to expect the same behavior slim/psr7 has. Formally you are right that the PSR does not require these params to be pre-populated, but it's a reasonable expectation IMO.
How are you instantiating the Request? How do you fill it?
I instantiate it using the ServerRequestFactoryInterface implementation provided by the https://github.com/Nyholm/psr7/blob/master/src/Factory/Psr17Factory.php
Sorry, I've probably created the issue in the wrong project. If you have the rights, you should probably move the issue there: https://github.com/Nyholm/psr7
It would be interesting to see if it works if you were to use slim/psr7 instead of Nyholm/psr7.
It does indeed work, and this fact forced me switch over to the slim/psr7 lib.
Ah, yes, if you are not actually using Nyholm/psr7-server then maybe my response does not make a whole lot of sense! I have now moved the issue.
Formally you are right that the PSR does not require these params to be pre-populated, but it's a reasonable expectation IMO.
Maybe. However, to keep the portability of PSR-7 implementations, I would say the only expectation that should be made from the PSR-7 spec design is that getQueryParams() will give you what was set by withQueryParams().
In fact, I am not sure if slim/psr7 does what I would expect it to do. Take this example:
$uriFactory = new \Slim\Psr7\Factory\UriFactory();
$requestFactory = new \Slim\Psr7\Factory\ServerRequestFactory(
new \Slim\Psr7\Factory\StreamFactory(),
$uriFactory
);
$request1 = $requestFactory->createServerRequest('GET', 'https://example.com/?query=value');
var_dump($request1->getQueryParams());
$request2 = $requestFactory->createServerRequest('GET', 'https://example.com/?query=value');
$request2 = $request2->withUri($uriFactory->createUri('https://example.com/?after=creation'));
var_dump($request2->getQueryParams());
$request2 = $request2->withUri($uriFactory->createUri('https://example.com/?after=getQuery'));
var_dump($request2->getQueryParams());
I has the following output:
array(1) {
["query"]=>
string(5) "value"
}
array(1) {
["after"]=>
string(8) "creation"
}
array(1) {
["after"]=>
string(8) "creation"
}
The query parameters “SHOULD remain immutable” per the PSR-7 spec, but in their implementation it can change depending on the Uri object you add. But only until you do your first getQuery(), after that it does become immutable. In my opinion this is very confusing behaviour.
So I would like to figure out what the expectation would be coming out of reading the PSR-7 spec. I would even ask from a PSR-17 point of view. If a Request factory is used, should query parameters have been set? Sounds like a valid question, that might even need to go to the PHP-FIG mailing list.
@Zegnat Thank you for the response! It indeed looks like it needs some clarification in the upcoming PSR editions, if there will be any. And yeah, it would be really nice if it's forwarded to php-fig.
The query parameters “SHOULD remain immutable” per the PSR-7 spec, but in their implementation it can change depending on the Uri object you add
These words are related only to setting the params with withQueryParams(...). If you explicitly set them using this method, they will not depend on the URI, so their behavior conforms to the spec, and the example given is a bit irrelevant.
The fact that they "cache" the query params taken from the URI once you call the getter is confusing though. Probably it's reasonable to recalculate the params on each withUri() call, until withQueryParams(...) is called.
I am starting to think, that the method withQueryParams(...) does only complicate the whole thing and should maybe be removed in the next version of the PSR, if any. IMO, the query params should only be derived from the URI. Is there any rationale behind the possibility to manually override them?
@Zegnat WDYT about creating an official unit tests suite for testing PSR-17 + PSR-7 implementations? The test suite takes PSR-17 factories, performs checks and outputs a list of MUST and SHOULD violations. That way we would have an official criteria of the spec conformity.
These already exist. I have contributed to them. So has @Nyholm. But they test for strictness of the specification, and what you have here does not seem to be defined in the spec.
Previously other things that gave conflicts between implementations, the cursor position in Streams, has gotten tests. But as they are not defined by the spec, they have not been merged in.
It would be interesting to do a look around between different implementations and see if their PSR-17 factories set a value for the query params or not. (I personally still think Slim is doing the wrong thing, where the getter on the PSR-7 mutates the instance.)
I've found out that in its internal unit tests, league/openapi-psr7-validator actually does not seem to rely on query params parsing from the URI, and indeed expects them to be set via withQueryParams(). So I was not right about their intent, sorry.
The only small thing left is clarifying the PSR-17 behavior, whether query params should be pre-populated based on URI or not. It seems they shouldn't after all, because the spec doesn't tell anything about that. However, they should be pre-populated based on $_GET as stated in the PSR-7 meta document. Although, there is no PSR for creating a request from the PHP superglobals yet, so this behavior cannot be enforced in any way.
These already exist.
Great, thank you!
It would be interesting to do a look around between different implementations and see if their PSR-17 factories set a value for the query params or not.
It would be great if this and other controversial things either get clarified in the future PSR edition or at least in a comment from PHP-FIG. For now I guess it's undefined behavior.
I personally still think Slim is doing the wrong thing, where the getter on the PSR-7 mutates the instance
Yeah, I also think so (although, admittedly immutability in PSR-7 is broken by design anyway) in regard of its "caching" behavior. It may still do this caching thing for performance optimization, but from the external perspective it should be transparent and behave as immutable anyway. Although, it violates the SHOULD section, so technially I guess it doesn't count as hard violation.
Thank you again for the in-depth discussion. I guess, this issue can be closed now as for now it's basically UB.
Thanks for bringing the discussion to attention! I have changed the title to more reflect the current idea. I definitely think it is worth looking into what other implementations have opted to do!
This is all slightly above me. In my setup \Nyholm\Psr7\Factory\Psr17Factory()->createServerRequest('GET', 'https://example.com/?query=value'); var_dump($request1->getQueryParams()); Doesn't work and the query params are not seen by the application being tested. What should I do instead? If I should use PSR7 instead of 17, how would I do that and what else would be affected? Thanks
I would love to see that coming!
Switched from slim/psr7 and nearly all my test cases using query params failed because $request->getQueryParams(); returns an empty array even when there are query parameters in the uri.
It would also align with the slim documentation:
You can get the query parameters as an associative array on the Request object using getQueryParams().
What is holding us back to implement something like the slim/psr7 method ?
public function getQueryParams(): array
{
$queryParams = $this->serverRequest->getQueryParams();
if (is_array($queryParams) && !empty($queryParams)) {
return $queryParams;
}
$parsedQueryParams = [];
parse_str($this->serverRequest->getUri()->getQuery(), $parsedQueryParams);
return $parsedQueryParams;
}
