Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assert values according to PSR standard #250

Merged
merged 6 commits into from Jun 5, 2019
Merged

Conversation

gmponos
Copy link
Member

@gmponos gmponos commented Dec 8, 2018

Related to issue #249

@@ -91,6 +92,7 @@ public function getMethod()

public function withMethod($method)
{
$this->assertMethod($method);
$new = clone $this;
$new->method = strtoupper($method);
Copy link
Member Author

@gmponos gmponos Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 gmponos changed the title Assert values according to PSR standard [WIP] Assert values according to PSR standard Dec 8, 2018
@gmponos
Copy link
Member Author

@gmponos gmponos commented Dec 8, 2018

I still need to write tests for the changes here.. but let me know if something is going to the wrong direction here.

@gmponos
Copy link
Member Author

@gmponos gmponos commented Dec 9, 2018

I added the tests.

@gmponos gmponos changed the title [WIP] Assert values according to PSR standard Assert values according to PSR standard Dec 9, 2018
src/MessageTrait.php Outdated Show resolved Hide resolved
@gmponos
Copy link
Member Author

@gmponos gmponos commented Dec 29, 2018

I added a commit where I removed the duplicated code and created a new function

@sagikazarmark sagikazarmark added this to the 1.6.0 milestone Apr 1, 2019

private function assertStatusCodeRange($statusCode)
{
if ($statusCode < 100 || $statusCode >= 600) {
Copy link
Member

@Tobion Tobion Jun 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
$this->assertStatusCodeIsInteger($status);
$status = (int) $status;
$this->assertStatusCodeRange($status);

$this->statusCode = (int) $status;
Copy link
Member

@Tobion Tobion Jun 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.');
Copy link
Member

@Tobion Tobion Jun 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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.');
Copy link
Member

@Tobion Tobion Jun 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Tobion Tobion Jun 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@hutnikau hutnikau Jun 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.');
Copy link
Member

@Tobion Tobion Jun 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: non-empty

Tobion
Tobion approved these changes Jun 5, 2019
@Tobion Tobion merged commit c5aea30 into guzzle:master Jun 5, 2019
1 check passed
@Tobion
Copy link
Member

@Tobion Tobion commented Jun 6, 2019

Remarks fixed in #272

@agwidarsito
Copy link

@agwidarsito agwidarsito commented Jun 6, 2019

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.

@agwidarsito
Copy link

@agwidarsito agwidarsito commented Jun 6, 2019

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

@sagikazarmark
Copy link
Member

@sagikazarmark sagikazarmark commented Jun 6, 2019

#272 should address this issue

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

Successfully merging this pull request may close these issues.

None yet

5 participants