Assert values according to PSR standard #250
Conversation
| @@ -91,6 +92,7 @@ public function getMethod() | |||
|
|
|||
| public function withMethod($method) | |||
| { | |||
| $this->assertMethod($method); | |||
| $new = clone $this; | |||
| $new->method = strtoupper($method); | |||
There was a problem hiding this comment.
According to the tests the strtoupper should not be here.. the current PR targets 1,x.. Can this be fixed on v1. and add the change on the current PR or it should be fixed on version 2.x?
gmponos
changed the title
|
I still need to write tests for the changes here.. but let me know if something is going to the wrong direction here. |
|
I added the tests. |
gmponos
changed the title
|
I added a commit where I removed the duplicated code and created a new function |
|
|
||
| private function assertStatusCodeRange($statusCode) | ||
| { | ||
| if ($statusCode < 100 || $statusCode >= 600) { |
There was a problem hiding this comment.
👍 according to https://tools.ietf.org/html/rfc7231#section-6
| } | ||
| $this->assertStatusCodeIsInteger($status); | ||
| $status = (int) $status; | ||
| $this->assertStatusCodeRange($status); | ||
|
|
||
| $this->statusCode = (int) $status; |
There was a problem hiding this comment.
you can remove this cast since you casted already a few lines above
| private function assertMethod($method) | ||
| { | ||
| if (!is_string($method) || $method === '') { | ||
| throw new \InvalidArgumentException('Method should be a non empty string.'); |
There was a problem hiding this comment.
👍 method is defined as token = 1*tchar
| { | ||
| if (!is_array($value)) { | ||
| if (!is_string($value)) { | ||
| throw new \InvalidArgumentException('Header value must be a string or an array of strings.'); |
There was a problem hiding this comment.
In theory that's correct. But I think it's too risky to add this validation in a minor version, e.g. withHeader('Api-Version', 1) would suddenly break for people. Before the integer was simply casted to string which seems like a sensible behavior.
So I would remove this validation for now. It might be something to consider for 2.0.
There was a problem hiding this comment.
There was a problem hiding this comment.
Looks like this change leads to unexpected exceptions.
$request->getBody()->getSize() returns integer:
https://github.com/guzzle/guzzle/blob/master/src/PrepareBodyMiddleware.php#L57
and any request leads to throwing that exception.
| private function assertHeader($header) | ||
| { | ||
| if (!is_string($header) || $header === '') { | ||
| throw new \InvalidArgumentException('Header must be a non empty string.'); |
|
Remarks fixed in #272 |
|
Interestingly this just now broke our Laravel 5.4 project build. Anyone else have this problem? My guess is one of the dependencies we have used via Composer is just pulling the absolute latest from Git rather than a specific version. |
|
I'm not sure where I should put this comment, so just putting it here if it helps anyone else or is valuable to anyone. It appears this commit - for me at leats - breaks the aws/aws-sdk-php package version 3.99.2 S3 client |
|
#272 should address this issue |
Related to issue #249