Status code must be an integer following psr-7 #319
Conversation
There was a problem hiding this comment.
While I agree we need to ensure the status code is an integer, we were doing that previously via a combination of an is_numeric() check + casting to int. The changes you propose create a BC break in that regard, which is problematic when considering how one parses full HTTP response status lines in order to seed a response.
I'd be fine with removing the check for floats, but requiring an integer is a no-go from a BC standpoint.
src/Response.php
Outdated
| @@ -174,8 +174,7 @@ public function withStatus($code, $reasonPhrase = '') | |||
| */ | |||
| private function setStatusCode($code) | |||
| { | |||
| if (! is_numeric($code) | |||
| || is_float($code) | |||
| if (! is_int($code) | |||
There was a problem hiding this comment.
One potential problem: the status code value may be derived from a string. As an example, it may come from the response line itself:
HTTP/1.1 200 OKIn that case, we previously allowed doing something like the following:
[$protocolAndVersion, $statusCode, $reasonPhrase] = explode(' ', $statusLine, 3);and passing the values directly to the constructor and/or withStatus().
With this change, the consumer is now forced to cast to int before doing so, which becomes a BC break.
src/Response/Serializer.php
Outdated
| @@ -106,6 +106,6 @@ private static function getStatusLine(StreamInterface $stream) | |||
| throw new UnexpectedValueException('No status line detected'); | |||
| } | |||
|
|
|||
| return [$matches['version'], $matches['status'], isset($matches['reason']) ? $matches['reason'] : '']; | |||
| return [$matches['version'], (int) $matches['status'], isset($matches['reason']) ? $matches['reason'] : '']; | |||
There was a problem hiding this comment.
This is exactly the point I was making previously; with the change you suggest above, we now have a BC break requiring consumers make changes such as this.
test/ResponseTest.php
Outdated
| @@ -78,7 +78,7 @@ public function ianaCodesReasonPhrasesProvider() | |||
| $records = $xpath->query('//ns:record'); | |||
|
|
|||
| foreach ($records as $record) { | |||
| $value = $xpath->query('.//ns:value', $record)->item(0)->nodeValue; | |||
| $value = (int) $xpath->query('.//ns:value', $record)->item(0)->nodeValue; | |||
There was a problem hiding this comment.
This also confirms the BC break; previously, casting to (int) was unnecessary.
|
@weierophinney can we keep this changes to v2.0? |
I'd rather not. Since the spec itself doesn't have explicit scalar type hints defined, what the constructors and various If/when we do an update to the PSR-7 specification to add scalar type hints, we can revisit. |
|
That's explanation is enough for me. I will prepare changes |
|
Thanks, @snapshotpl! |
No description provided.