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

Move header value normalisation into Response #81

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,5 @@
/laminas-mkdoc-theme/
/phpunit.xml
/vendor/

.idea
6 changes: 1 addition & 5 deletions psalm-baseline.xml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<?xml version="1.0" encoding="UTF-8"?>
<files psalm-version="5.14.1@b9d355e0829c397b9b3b47d0c0ed042a8a70284d">
<files psalm-version="5.15.0@5c774aca4746caf3d239d9c8cadb9f882ca29352">
<file src="src/PubSubHubbub/AbstractCallback.php">
<DocblockTypeContradiction>
<code><![CDATA[$this->httpResponse === null]]></code>
Expand Down Expand Up @@ -2217,11 +2217,7 @@
</UnsafeInstantiation>
</file>
<file src="src/Reader/Http/LaminasHttpClientDecorator.php">
<MixedArgumentTypeCoercion>
<code>$value</code>
</MixedArgumentTypeCoercion>
<MixedAssignment>
<code>$normalized[$name]</code>
<code>$value</code>
<code>$value</code>
</MixedAssignment>
Expand Down
22 changes: 3 additions & 19 deletions src/Reader/Http/LaminasHttpClientDecorator.php
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<?php

Check failure on line 1 in src/Reader/Http/LaminasHttpClientDecorator.php

View workflow job for this annotation

GitHub Actions / ci / QA Checks (Psalm [8.1, locked], ubuntu-latest, laminas/laminas-continuous-integration-action@v1, ...

UnusedBaselineEntry

src/Reader/Http/LaminasHttpClientDecorator.php:0:0: UnusedBaselineEntry: Baseline for issue "MixedAssignment" has 1 extra entry. (see https://psalm.dev/316)

declare(strict_types=1);

Expand All @@ -9,7 +9,6 @@
use Laminas\Http\Headers;

use function gettype;
use function implode;
use function is_array;
use function is_numeric;
use function is_object;
Expand Down Expand Up @@ -47,10 +46,12 @@
}
$response = $this->client->send();

$headers = $response->getHeaders()->toArray();

return new Response(
$response->getStatusCode(),
$response->getBody(),
$this->prepareResponseHeaders($response->getHeaders())
$headers
);
}

Expand Down Expand Up @@ -94,21 +95,4 @@
}
}
}

/**
* Normalize headers to use with HeaderAwareResponseInterface.
*
* Ensures multi-value headers are represented as a single string, via
* comma concatenation.
*
* @return array
*/
private function prepareResponseHeaders(Headers $headers)
{
$normalized = [];
foreach ($headers->toArray() as $name => $value) {
$normalized[$name] = is_array($value) ? implode(', ', $value) : $value;
}
return $normalized;
}
}
21 changes: 19 additions & 2 deletions src/Reader/Http/Response.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
use Laminas\Feed\Reader\Exception;

use function gettype;
use function implode;
use function intval;
use function is_array;
use function is_numeric;
use function is_object;
use function is_string;
Expand Down Expand Up @@ -142,27 +144,42 @@
));
}

if (! is_string($value) && ! is_numeric($value)) {
if (! is_string($value) && ! is_numeric($value) && ! is_array($value)) {
throw new Exception\InvalidArgumentException(sprintf(
'Individual header values provided to %s must be a string or numeric; received %s for header %s',
self::class,
is_object($value) ? $value::class : gettype($value),
$name
));
}

if (is_array($value)) {
foreach ($value as $key => $multiValue) {
if (! is_string($multiValue) && ! is_numeric($multiValue)) {
throw new Exception\InvalidArgumentException(sprintf(
'Individual header values provided to %s must be a string or numeric; received %s for header %s',

Check warning on line 160 in src/Reader/Http/Response.php

View workflow job for this annotation

GitHub Actions / ci / QA Checks (PHPCodeSniffer [8.1, locked], ubuntu-latest, laminas/laminas-continuous-integration-ac...

Line exceeds 120 characters; contains 125 characters
self::class,
is_object($multiValue) ? $multiValue::class : gettype($multiValue),
$name . "[$key]"
));
}
}
}
}
}

/**
* Normalize header names to lowercase.
* Also ensures multi-value headers are represented as a single string, via
* comma concatenation.
*
* @return array
Copy link
Member

Choose a reason for hiding this comment

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

Let's document this to be an array<string> then? Previously, it was an array<string|array<string>>, so this change looks like a breaking change 🤔

Copy link
Author

Choose a reason for hiding this comment

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

Its the opposite I think. Before the normalizeHeaders was expecting array<string, string> and now it can handle array<string, string|array<string>>, as the logic that was in LaminasHttpClientDecorator prepareResponseHeaders now is included in Response normalizeHeaders (see validateHeaders in Response is not accepting multi value (L147 in my branch).

So I thought its not a breaking change as now it can handle what it was handling before and in addition multi-value headers?

*/
private function normalizeHeaders(array $headers)
{
$normalized = [];
foreach ($headers as $name => $value) {
$normalized[strtolower($name)] = $value;
$normalized[strtolower($name)] = is_array($value) ? implode(', ', $value) : $value;

Check failure on line 182 in src/Reader/Http/Response.php

View workflow job for this annotation

GitHub Actions / ci / QA Checks (Psalm [8.1, locked], ubuntu-latest, laminas/laminas-continuous-integration-action@v1, ...

MixedArgumentTypeCoercion

src/Reader/Http/Response.php:182:79: MixedArgumentTypeCoercion: Argument 2 of implode expects array<array-key, null|object{__tostring()}|scalar>, but parent type array<array-key, mixed> provided (see https://psalm.dev/194)
}
return $normalized;
}
Expand Down
2 changes: 2 additions & 0 deletions test/Reader/Http/LaminasHttpClientDecoratorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,8 @@ public static function invalidHeaders(): iterable
foreach ($invalidIndividualValues as $key => $value) {
yield $key => [['X-Test' => [$value]], 'strings or numbers'];
}

// todo: expand here with multi values.
}

/**
Expand Down
28 changes: 24 additions & 4 deletions test/Reader/Http/ResponseTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@
'Location' => 'http://example.org/foo',
'Content-Length' => 1234,
'X-Content-Length' => 1234.56,
'MultiValue' => ['one', 'two'],
'MultiValue2' => [1, 2],

]);

Check failure on line 49 in test/Reader/Http/ResponseTest.php

View workflow job for this annotation

GitHub Actions / ci / QA Checks (PHPCodeSniffer [8.1, locked], ubuntu-latest, laminas/laminas-continuous-integration-ac...

Blank line found at the end of the array
$this->assertEquals(204, $response->getStatusCode());
$this->assertEquals('', $response->getBody());
$this->assertEquals('http://example.org/foo', $response->getHeaderLine('Location'));
Expand Down Expand Up @@ -120,42 +123,59 @@
public static function invalidHeaders(): array
{
return [
'empty-name' => [

Check failure on line 126 in test/Reader/Http/ResponseTest.php

View workflow job for this annotation

GitHub Actions / ci / QA Checks (PHPCodeSniffer [8.1, locked], ubuntu-latest, laminas/laminas-continuous-integration-ac...

Expected 12 spaces before double arrow; 3 found
['' => 'value'],
'non-empty, non-numeric',
],
'zero-name' => [

Check failure on line 130 in test/Reader/Http/ResponseTest.php

View workflow job for this annotation

GitHub Actions / ci / QA Checks (PHPCodeSniffer [8.1, locked], ubuntu-latest, laminas/laminas-continuous-integration-ac...

Expected 13 spaces before double arrow; 4 found
['value'],
'non-empty, non-numeric',
],
'int-name' => [

Check failure on line 134 in test/Reader/Http/ResponseTest.php

View workflow job for this annotation

GitHub Actions / ci / QA Checks (PHPCodeSniffer [8.1, locked], ubuntu-latest, laminas/laminas-continuous-integration-ac...

Expected 14 spaces before double arrow; 5 found
[1 => 'value'],
'non-empty, non-numeric',
],
'numeric-name' => [

Check failure on line 138 in test/Reader/Http/ResponseTest.php

View workflow job for this annotation

GitHub Actions / ci / QA Checks (PHPCodeSniffer [8.1, locked], ubuntu-latest, laminas/laminas-continuous-integration-ac...

Expected 10 spaces before double arrow; 1 found
['1.1' => 'value'],
'non-empty, non-numeric',
],
'null-value' => [

Check failure on line 142 in test/Reader/Http/ResponseTest.php

View workflow job for this annotation

GitHub Actions / ci / QA Checks (PHPCodeSniffer [8.1, locked], ubuntu-latest, laminas/laminas-continuous-integration-ac...

Expected 12 spaces before double arrow; 3 found
['X-Test' => null],
'must be a string or numeric',
],
'true-value' => [

Check failure on line 146 in test/Reader/Http/ResponseTest.php

View workflow job for this annotation

GitHub Actions / ci / QA Checks (PHPCodeSniffer [8.1, locked], ubuntu-latest, laminas/laminas-continuous-integration-ac...

Expected 12 spaces before double arrow; 3 found
['X-Test' => true],
'must be a string or numeric',
],
'false-value' => [

Check failure on line 150 in test/Reader/Http/ResponseTest.php

View workflow job for this annotation

GitHub Actions / ci / QA Checks (PHPCodeSniffer [8.1, locked], ubuntu-latest, laminas/laminas-continuous-integration-ac...

Expected 11 spaces before double arrow; 2 found
['X-Test' => false],
'must be a string or numeric',
],
'array-value' => [
['X-Test' => ['BODY']],
'must be a string or numeric',
],
'object-value' => [

Check failure on line 154 in test/Reader/Http/ResponseTest.php

View workflow job for this annotation

GitHub Actions / ci / QA Checks (PHPCodeSniffer [8.1, locked], ubuntu-latest, laminas/laminas-continuous-integration-ac...

Expected 10 spaces before double arrow; 1 found
['X-Test' => (object) ['body' => 'BODY']],
'must be a string or numeric',
],
'null-value-in-array' => [
['X-Test' => [null]],
'must be a string or numeric',
],
'true-value-in-array' => [
['X-Test' => [true]],
'must be a string or numeric',
],
'false-value-in-array' => [
['X-Test' => [false]],
'must be a string or numeric',
],
'object-value-in-array' => [
['X-Test' => [(object) ['body' => 'BODY']]],
'must be a string or numeric',
],
// todo: expand this with more combinations?
'mixed-values-in-array' => [
['X-Test' => ['valid', false]],
'must be a string or numeric',
]

Check failure on line 178 in test/Reader/Http/ResponseTest.php

View workflow job for this annotation

GitHub Actions / ci / QA Checks (PHPCodeSniffer [8.1, locked], ubuntu-latest, laminas/laminas-continuous-integration-ac...

Multi-line arrays must have a trailing comma after the last element
];
}

Expand Down