Skip to content

[HttpFoundation] deprecate HeaderBag::get() returning an array and add all($key) instead #32122

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

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions UPGRADE-4.4.md
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ HttpFoundation
--------------

* `ApacheRequest` is deprecated, use `Request` class instead.
* Passing a third argument to `HeaderBag::get()` is deprecated since Symfony 4.4, use method `all()` instead

HttpKernel
----------
Expand Down
1 change: 1 addition & 0 deletions UPGRADE-5.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@ HttpFoundation
* The `FileinfoMimeTypeGuesser` class has been removed,
use `Symfony\Component\Mime\FileinfoMimeTypeGuesser` instead.
* `ApacheRequest` has been removed, use the `Request` class instead.
* The third argument of the `HeaderBag::get()` method has been removed, use method `all()` instead.

HttpKernel
----------
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Component/HttpFoundation/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ CHANGELOG

* passing arguments to `Request::isMethodSafe()` is deprecated.
* `ApacheRequest` is deprecated, use the `Request` class instead.
* passing a third argument to `HeaderBag::get()` is deprecated, use method `all()` instead

4.3.0
-----
Expand Down
35 changes: 18 additions & 17 deletions src/Symfony/Component/HttpFoundation/HeaderBag.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,18 @@ public function __toString()
/**
* Returns the headers.
*
* @param string|null $key The name of the headers to return or null to get them all
*
* @return array An array of headers
*/
public function all()
public function all(/*string $key = null*/)
Copy link
Contributor

Choose a reason for hiding this comment

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

That is a no-go for me. It introduces another of #31317 which this PR tried to fix.
Depending on the arguments it returns an array of arrays or an array of strings.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know it's the same as on the Messenger Envelope but that was on my todo to fix as was a pain to work with.

{
if (1 <= \func_num_args() && null !== $key = func_get_arg(0)) {
$key = str_replace('_', '-', strtolower($key));

return $this->headers[$key] ?? [];
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't solve the problem in the linked issue (the get method having 2 different return types depending on a parameter), now it's all that can return either string[] or string[][] based on the presence of a parameter.

Copy link
Member

@nicolas-grekas nicolas-grekas Aug 6, 2019

Choose a reason for hiding this comment

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

that's fine to me, the proposed interface is a nice and consistent update

}

return $this->headers;
}

Expand Down Expand Up @@ -103,28 +111,21 @@ public function add(array $headers)
*
* @param string $key The header name
* @param string|null $default The default value
* @param bool $first Whether to return the first value or all header values
*
* @return string|string[]|null The first header value or default value if $first is true, an array of values otherwise
* @return string|null The first header value or default value
*/
public function get($key, $default = null, $first = true)
public function get($key, $default = null)
{
$key = str_replace('_', '-', strtolower($key));
$headers = $this->all();
$headers = $this->all((string) $key);
if (2 < \func_num_args()) {
@trigger_error(sprintf('Passing a third argument to "%s()" is deprecated since Symfony 4.4, use method "all()" instead', __METHOD__), E_USER_DEPRECATED);

if (!\array_key_exists($key, $headers)) {
if (null === $default) {
return $first ? null : [];
if (!func_get_arg(2)) {
return $headers;
}

return $first ? $default : [$default];
}

if ($first) {
return \count($headers[$key]) ? $headers[$key][0] : $default;
}

return $headers[$key];
return $headers[0] ?? $default;
}

/**
Expand Down Expand Up @@ -181,7 +182,7 @@ public function has($key)
*/
public function contains($key, $value)
{
return \in_array($value, $this->get($key, null, false));
return \in_array($value, $this->all((string) $key));
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/HttpFoundation/Response.php
Original file line number Diff line number Diff line change
Expand Up @@ -1024,7 +1024,7 @@ public function hasVary(): bool
*/
public function getVary(): array
{
if (!$vary = $this->headers->get('Vary', null, false)) {
if (!$vary = $this->headers->all('Vary')) {
return [];
}

Expand Down
11 changes: 10 additions & 1 deletion src/Symfony/Component/HttpFoundation/ResponseHeaderBag.php
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,19 @@ public function replace(array $headers = [])

/**
* {@inheritdoc}
*
* @param string|null $key The name of the headers to return or null to get them all
*/
public function all()
public function all(/*string $key = null*/)
{
$headers = parent::all();

if (1 <= \func_num_args() && null !== $key = func_get_arg(0)) {
$key = str_replace('_', '-', strtolower($key));

return 'set-cookie' !== $key ? $headers[$key] ?? [] : array_map('strval', $this->getCookies());
}

foreach ($this->getCookies() as $cookie) {
$headers['set-cookie'][] = (string) $cookie;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public function toString(): string
*/
protected function matches($response): bool
{
return $this->expectedValue === $response->headers->get($this->headerName, null, true);
return $this->expectedValue === $response->headers->get($this->headerName, null);
}

/**
Expand Down
18 changes: 14 additions & 4 deletions src/Symfony/Component/HttpFoundation/Tests/HeaderBagTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -88,24 +88,34 @@ public function testGet()
$bag = new HeaderBag(['foo' => 'bar', 'fuzz' => 'bizz']);
$this->assertEquals('bar', $bag->get('foo'), '->get return current value');
$this->assertEquals('bar', $bag->get('FoO'), '->get key in case insensitive');
$this->assertEquals(['bar'], $bag->get('foo', 'nope', false), '->get return the value as array');
$this->assertEquals(['bar'], $bag->all('foo'), '->get return the value as array');

// defaults
$this->assertNull($bag->get('none'), '->get unknown values returns null');
$this->assertEquals('default', $bag->get('none', 'default'), '->get unknown values returns default');
$this->assertEquals(['default'], $bag->get('none', 'default', false), '->get unknown values returns default as array');
$this->assertEquals([], $bag->all('none'), '->get unknown values returns an empty array');

$bag->set('foo', 'bor', false);
$this->assertEquals('bar', $bag->get('foo'), '->get return first value');
$this->assertEquals(['bar', 'bor'], $bag->get('foo', 'nope', false), '->get return all values as array');
$this->assertEquals(['bar', 'bor'], $bag->all('foo'), '->get return all values as array');
}

/**
* @group legacy
* @expectedDeprecation Passing a third argument to "Symfony\Component\HttpFoundation\HeaderBag::get()" is deprecated since Symfony 4.4, use method "all()" instead
*/
public function testGetIsEqualToNewMethod()
{
$bag = new HeaderBag(['foo' => 'bar', 'fuzz' => 'bizz']);
$this->assertSame($bag->all('none'), $bag->get('none', [], false), '->get unknown values returns default as array');
}

public function testSetAssociativeArray()
{
$bag = new HeaderBag();
$bag->set('foo', ['bad-assoc-index' => 'value']);
$this->assertSame('value', $bag->get('foo'));
$this->assertEquals(['value'], $bag->get('foo', 'nope', false), 'assoc indices of multi-valued headers are ignored');
$this->assertSame(['value'], $bag->all('foo'), 'assoc indices of multi-valued headers are ignored');
}

public function testContains()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,13 +89,13 @@ public function testCacheControlHeader()

$bag = new ResponseHeaderBag();
$bag->set('Cache-Control', ['public', 'must-revalidate']);
$this->assertCount(1, $bag->get('Cache-Control', null, false));
$this->assertCount(1, $bag->all('Cache-Control'));
$this->assertEquals('must-revalidate, public', $bag->get('Cache-Control'));

$bag = new ResponseHeaderBag();
$bag->set('Cache-Control', 'public');
$bag->set('Cache-Control', 'must-revalidate', false);
$this->assertCount(1, $bag->get('Cache-Control', null, false));
$this->assertCount(1, $bag->all('Cache-Control'));
$this->assertEquals('must-revalidate, public', $bag->get('Cache-Control'));
}

Expand Down Expand Up @@ -166,7 +166,7 @@ public function testCookiesWithSameNames()
'foo=bar; path=/path/bar; domain=foo.bar; httponly; samesite=lax',
'foo=bar; path=/path/bar; domain=bar.foo; httponly; samesite=lax',
'foo=bar; path=/; httponly; samesite=lax',
], $bag->get('set-cookie', null, false));
], $bag->all('set-cookie'));

$this->assertSetCookieHeader('foo=bar; path=/path/foo; domain=foo.bar; httponly; samesite=lax', $bag);
$this->assertSetCookieHeader('foo=bar; path=/path/bar; domain=foo.bar; httponly; samesite=lax', $bag);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ public function testSessionWithNewSessionIdAndNewCookieDoesNotSendAnotherCookie(

$response = $this->filterResponse(new Request(), HttpKernelInterface::MASTER_REQUEST, $response);

$this->assertSame($expected, $response->headers->get('Set-Cookie', null, false));
$this->assertSame($expected, $response->headers->all()['set-cookie']);
}

public function anotherCookieProvider()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public function testOnKernelResponse()
'</foo>; rel="preload"',
];

$this->assertEquals($expected, $response->headers->get('Link', null, false));
$this->assertEquals($expected, $response->headers->all()['link']);
}

public function testSubscribedEvents()
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/WebLink/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
"symfony/http-kernel": ""
},
"require-dev": {
"symfony/http-foundation": "^3.4|^4.0|^5.0",
"symfony/http-foundation": "^4.4|^5.0",
"symfony/http-kernel": "^4.3|^5.0"
},
"conflict": {
Expand Down