feature(resolver): Add support for get parameters #472
Conversation
| @@ -103,6 +103,23 @@ public function testGetMetaDataWithPost() | |||
| $this->assertSame(['request' => $data], $this->resolver->resolve()->getMetaData()); | |||
| } | |||
|
|
|||
| public function testGetMetaDataWithGet() | |||
| { | |||
| $_SERVER['REQUEST_METHOD'] = 'GET'; | |||
There was a problem hiding this comment.
What would happen with lesser used request methods like HEAD or PATCH? Is it worth covering them?
There was a problem hiding this comment.
They don't have server variables and the data should be dropped into either the json_decode section due to application/json being present, or the parse_str section if not. I'll do some manual testing on this to verify these cases are covered however.
src/Request/BasicResolver.php
Outdated
| { | ||
| static $result; | ||
|
|
||
| if ($result !== null) { | ||
| return $result ?: null; | ||
| } | ||
|
|
||
| $result = $post ?: static::parseInput($server, static::readInput()); | ||
| if (isset($server['REQUEST_METHOD']) && strtoupper($server['REQUEST_METHOD']) === 'GET') { | ||
| $result = $_GET; |
There was a problem hiding this comment.
Access to superglobals was intentionally omitted from anything other than the resolve method. Perhaps put it back there, so that this method is not messing with global state?
There was a problem hiding this comment.
Does it matter if the method is only being called statically from within the resolve method?
src/Request/BasicResolver.php
Outdated
| @@ -16,7 +16,7 @@ public function resolve() | |||
| empty($_SESSION) ? [] : $_SESSION, | |||
| empty($_COOKIE) ? [] : $_COOKIE, | |||
| static::getRequestHeaders($_SERVER), | |||
| static::getInputParams($_SERVER, $_POST)); | |||
| static::getInputParams($_SERVER)); | |||
There was a problem hiding this comment.
Checking the request method should instead happen here, and pass the correct variable array (get vs post variables) into getInputParams. It reduces the amount of "magic" going on throughout.
There was a problem hiding this comment.
In the current implementation if $_GET is empty in a GET request the method will attempt to pull body parameters from php://input, something that it shouldn't do for GET requests.
I'd suggest splitting getInputParams into multiple methods to handle GET and other request methods separately, or add an argument that determines whether the php://input source should be read or not.
There was a problem hiding this comment.
Correct, checking whether to read from php://input should be done by request type, either in a single place or via a flag. I'm not suggesting that input should be read for GET requests.
- Refactor to ensure superglobals are only accessed in the "resolve" function
|
Ok, I've refactored it somewhat so that the superglobals I've added a flag, The result doesn't effect the output, so the tests have remained the same. |
- Ensure type hinting isn't negatively reacting with input parameters
kattrali
changed the title
Show that the parameter is intended to determine whether input should be used as a params fallback if it is not otherwise present
getparameters in the basic resolver$_POSTand other method parameters.