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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
48 changes: 33 additions & 15 deletions src/MessageTrait.php
Expand Up @@ -66,11 +66,8 @@ public function getHeaderLine($header)

public function withHeader($header, $value)
{
if (!is_array($value)) {
$value = [$value];
}

$value = $this->trimHeaderValues($value);
$this->assertHeader($header);
$value = $this->normalizeHeaderValue($value);
$normalized = strtolower($header);

$new = clone $this;
Expand All @@ -85,11 +82,8 @@ public function withHeader($header, $value)

public function withAddedHeader($header, $value)
{
if (!is_array($value)) {
$value = [$value];
}

$value = $this->trimHeaderValues($value);
$this->assertHeader($header);
$value = $this->normalizeHeaderValue($value);
$normalized = strtolower($header);

$new = clone $this;
Expand Down Expand Up @@ -144,11 +138,8 @@ private function setHeaders(array $headers)
{
$this->headerNames = $this->headers = [];
foreach ($headers as $header => $value) {
if (!is_array($value)) {
$value = [$value];
}

$value = $this->trimHeaderValues($value);
$this->assertHeader($header);
$value = $this->normalizeHeaderValue($value);
$normalized = strtolower($header);
if (isset($this->headerNames[$normalized])) {
$header = $this->headerNames[$normalized];
Expand All @@ -160,6 +151,23 @@ private function setHeaders(array $headers)
}
}

private function normalizeHeaderValue($value)
{
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

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

Choose a reason for hiding this comment

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

Copy link

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.

}

return $this->trimHeaderValues([$value]);
}

if (count($value) === 0) {
throw new \InvalidArgumentException('Header value can not be an empty array.');
}

return $this->trimHeaderValues($value);
}

/**
* Trims whitespace from the header values.
*
Expand All @@ -177,7 +185,17 @@ private function setHeaders(array $headers)
private function trimHeaderValues(array $values)
{
return array_map(function ($value) {
if (!is_string($value)) {
throw new \InvalidArgumentException('Header value must be a string.');
}
return trim($value, " \t");
}, $values);
}

private function assertHeader($header)
{
if (!is_string($header) || $header === '') {
throw new \InvalidArgumentException('Header must be a non empty string.');
Copy link
Member

Choose a reason for hiding this comment

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

typo: non-empty

}
}
}
9 changes: 9 additions & 0 deletions src/Request.php
Expand Up @@ -36,6 +36,7 @@ public function __construct(
$body = null,
$version = '1.1'
) {
$this->assertMethod($method);
if (!($uri instanceof UriInterface)) {
$uri = new Uri($uri);
}
Expand Down Expand Up @@ -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?

return $new;
Expand Down Expand Up @@ -139,4 +141,11 @@ private function updateHostFromUri()
// See: http://tools.ietf.org/html/rfc7230#section-5.4
$this->headers = [$header => [$host]] + $this->headers;
}

private function assertMethod($method)
{
if (!is_string($method) || $method === '') {
throw new \InvalidArgumentException('Method should be a non empty string.');
Copy link
Member

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

}
}
}
26 changes: 22 additions & 4 deletions src/Response.php
Expand Up @@ -93,9 +93,9 @@ public function __construct(
$version = '1.1',
$reason = null
) {
if (filter_var($status, FILTER_VALIDATE_INT) === false) {
throw new \InvalidArgumentException('Status code must be an integer value.');
}
$this->assertStatusCodeIsInteger($status);
$status = (int) $status;
$this->assertStatusCodeRange($status);

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

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


Expand Down Expand Up @@ -125,12 +125,30 @@ public function getReasonPhrase()

public function withStatus($code, $reasonPhrase = '')
{
$this->assertStatusCodeIsInteger($code);
$code = (int) $code;
$this->assertStatusCodeRange($code);

$new = clone $this;
$new->statusCode = (int) $code;
$new->statusCode = $code;
if ($reasonPhrase == '' && isset(self::$phrases[$new->statusCode])) {
$reasonPhrase = self::$phrases[$new->statusCode];
}
$new->reasonPhrase = $reasonPhrase;
return $new;
}

private function assertStatusCodeIsInteger($statusCode)
{
if (filter_var($statusCode, FILTER_VALIDATE_INT) === false) {
throw new \InvalidArgumentException('Status code must be an integer value.');
}
}

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

Choose a reason for hiding this comment

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

throw new \InvalidArgumentException('Status code must be an integer value between 1xx and 5xx.');
}
}
}
30 changes: 30 additions & 0 deletions tests/RequestTest.php
Expand Up @@ -6,6 +6,7 @@
use GuzzleHttp\Psr7\Uri;

/**
* @covers GuzzleHttp\Psr7\MessageTrait
* @covers GuzzleHttp\Psr7\Request
*/
class RequestTest extends BaseTest
Expand Down Expand Up @@ -90,6 +91,35 @@ public function testWithUri()
$this->assertSame($u1, $r1->getUri());
}

/**
* @dataProvider invalidMethodsProvider
*/
public function testConstructWithInvalidMethods($method)
{
$this->expectException('InvalidArgumentException');
new Request($method, '/');
}

/**
* @dataProvider invalidMethodsProvider
*/
public function testWithInvalidMethods($method)
{
$r = new Request('get', '/');
$this->expectException('InvalidArgumentException');
$r->withMethod($method);
}

public function invalidMethodsProvider()
{
return [
[null],
[false],
[['foo']],
[new \stdClass()],
];
}

public function testSameInstanceWhenSameUri()
{
$r1 = new Request('GET', 'http://foo.com');
Expand Down
91 changes: 88 additions & 3 deletions tests/ResponseTest.php
Expand Up @@ -237,6 +237,51 @@ public function testSameInstanceWhenRemovingMissingHeader()
$this->assertSame($r, $r->withoutHeader('foo'));
}

/**
* @dataProvider invalidHeaderProvider
*/
public function testConstructResponseInvalidHeader($header, $headerValue, $expectedMessage)
{
$this->expectException('InvalidArgumentException', $expectedMessage);
new Response(200, [$header => $headerValue]);
}

public function invalidHeaderProvider()
{
return [
['foo', [], 'Header value can not be an empty array.'],
['', '', 'Header must be a non empty string.'],
['foo', false, 'Header value must be a string or an array of strings.'],
[false, 'foo', 'Header must be a non empty string.'],
['foo', new \stdClass(), 'Header value must be a string or an array of strings.'],
['foo', new \ArrayObject(), 'Header value must be a string or an array of strings.'],
];
}

/**
* @dataProvider invalidWithHeaderProvider
*/
public function testWithInvalidHeader($header, $headerValue, $expectedMessage)
{
$r = new Response();
$this->expectException('InvalidArgumentException', $expectedMessage);
$r->withHeader($header, $headerValue);
}

public function invalidWithHeaderProvider()
{
return [
[[], 'foo', 'Header must be a non empty string.'],
['foo', [], 'Header value can not be an empty array.'],
['', '', 'Header must be a non empty string.'],
['foo', false, 'Header value must be a string or an array of strings.'],
[false, 'foo', 'Header must be a non empty string.'],
['foo', new \stdClass(), 'Header value must be a string or an array of strings.'],
['foo', new \ArrayObject(), 'Header value must be a string or an array of strings.'],
[new \stdClass(), 'foo', 'Header must be a non empty string.'],
];
}

public function testHeaderValuesAreTrimmed()
{
$r1 = new Response(200, ['OWS' => " \t \tFoo\t \t "]);
Expand All @@ -251,16 +296,27 @@ public function testHeaderValuesAreTrimmed()
}

/**
* @dataProvider responseInitializedWithNonIntegerStatusCodeProvider
* @dataProvider nonIntegerStatusCodeProvider
* @param mixed $invalidValues
*/
public function testResponseInitializedWithNonIntegerStatusCodeProvider($invalidValues)
public function testConstructResponseWithNonIntegerStatusCode($invalidValues)
{
$this->expectException('InvalidArgumentException', 'Status code must be an integer value.');
new Response($invalidValues);
}

public function responseInitializedWithNonIntegerStatusCodeProvider()
/**
* @dataProvider nonIntegerStatusCodeProvider
* @param mixed $invalidValues
*/
public function testResponseChangeStatusCodeWithNonInteger($invalidValues)
{
$response = new Response();
$this->expectException('InvalidArgumentException', 'Status code must be an integer value.');
$response->withStatus($invalidValues);
}

public function nonIntegerStatusCodeProvider()
{
return [
['whatever'],
Expand All @@ -269,4 +325,33 @@ public function responseInitializedWithNonIntegerStatusCodeProvider()
[new \stdClass()],
];
}

/**
* @dataProvider invalidStatusCodeRangeProvider
* @param mixed $invalidValues
*/
public function testConstructResponseWithInvalidRangeStatusCode($invalidValues)
{
$this->expectException('InvalidArgumentException', 'Status code must be an integer value between 1xx and 5xx.');
new Response($invalidValues);
}

/**
* @dataProvider invalidStatusCodeRangeProvider
* @param mixed $invalidValues
*/
public function testResponseChangeStatusCodeWithWithInvalidRange($invalidValues)
{
$response = new Response();
$this->expectException('InvalidArgumentException', 'Status code must be an integer value between 1xx and 5xx.');
$response->withStatus($invalidValues);
}

public function invalidStatusCodeRangeProvider()
{
return [
[600],
[99],
];
}
}