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

PageCache storing original header names #19188

Merged
merged 5 commits into from
Jan 26, 2022
Merged
Show file tree
Hide file tree
Changes from 4 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 framework/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ Yii Framework 2 Change Log
- Bug #19148: Fix undefined array key errors in `yii\db\ActiveRelationTrait` (stevekr)
- Bug #19041: Fix PHP 8.1 issues (longthanhtran, samdark, pamparam83, sartor, githubjeka)
- Enh #19171: Added `$pagination` and `$sort` to `\yii\rest\IndexAction` for easy configuration (rhertogh)
- Bug #19187: Fix `yii\filters\PageCache` to store original headers names instead of normalized ones (bizley)


2.0.44 December 30, 2021
Expand Down
13 changes: 11 additions & 2 deletions framework/UPGRADE.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,18 @@ if you want to upgrade from version A to version C and there is
version B between A and C, you need to follow the instructions
for both A and B.

Upgrade from Yii 2.0.44
-----------------------

* `yii\filters\PageCache::$cacheHeaders` now takes a case-sensitive list of header names since PageCache is no longer
storing the normalized (lowercase) versions of them so make sure this list is properly updated and your page cache
is recreated.

Upgrade from Yii 2.0.43
-----------------------

* `Json::encode()` can now handle zero-indexed objects in same way as `json_encode()` and keep them as objects. In order to avoid breaking backwards compatibility this behavior could be enabled by a new option flag but is disabled by default.
* `Json::encode()` can now handle zero-indexed objects in same way as `json_encode()` and keep them as objects. In order
to avoid breaking backwards compatibility this behavior could be enabled by a new option flag but is disabled by default.
* Set `yii/helpers/Json::$keepObjectType = true` anywhere in your application code
* Or configure json response formatter to enable it for all JSON responses:
```php
Expand All @@ -68,7 +76,8 @@ Upgrade from Yii 2.0.43
],
],
```
* `yii\caching\Cache::multiSet()` now uses the default cache duration (`yii\caching\Cache::$defaultDuration`) when no duration is provided. A duration of 0 should be explicitly passed if items should not expire.
* `yii\caching\Cache::multiSet()` now uses the default cache duration (`yii\caching\Cache::$defaultDuration`) when no
duration is provided. A duration of 0 should be explicitly passed if items should not expire.

Upgrade from Yii 2.0.42
-----------------------
Expand Down
47 changes: 33 additions & 14 deletions framework/filters/PageCache.php
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ class PageCache extends ActionFilter implements DynamicContentAwareInterface
public $cacheCookies = false;
/**
* @var bool|array a boolean value indicating whether to cache all HTTP headers, or an array of
* HTTP header names (case-insensitive) indicating which HTTP headers can be cached.
* HTTP header names (case-sensitive) indicating which HTTP headers can be cached.
* Note if your HTTP headers contain sensitive information, you should white-list which headers can be cached.
* @since 2.0.4
*/
Expand Down Expand Up @@ -253,40 +253,59 @@ public function cacheResponse()
foreach (['format', 'version', 'statusCode', 'statusText'] as $name) {
$data[$name] = $response->{$name};
}
$this->insertResponseCollectionIntoData($response, 'headers', $data);
$this->insertResponseCollectionIntoData($response, 'cookies', $data);
$this->insertResponseHeaderCollectionIntoData($response, $data);
$this->insertResponseCookieCollectionIntoData($response, $data);
$this->cache->set($this->calculateCacheKey(), $data, $this->duration, $this->dependency);
$data['content'] = $this->updateDynamicContent($data['content'], $this->getDynamicPlaceholders());
echo $data['content'];
}

/**
* Inserts (or filters/ignores according to config) response headers/cookies into a cache data array.
* Inserts (or filters/ignores according to config) response cookies into a cache data array.
* @param Response $response the response.
* @param string $collectionName currently it's `headers` or `cookies`.
* @param array $data the cache data.
*/
private function insertResponseCollectionIntoData(Response $response, $collectionName, array &$data)
private function insertResponseCookieCollectionIntoData(Response $response, array &$data)
{
$property = 'cache' . ucfirst($collectionName);
if ($this->{$property} === false) {
if ($this->cacheCookies === false) {
return;
}

$all = $response->{$collectionName}->toArray();
if (is_array($this->{$property})) {
$all = $response->cookies->toArray();
if (is_array($this->cacheCookies)) {
$filtered = [];
foreach ($this->{$property} as $name) {
if ($collectionName === 'headers') {
$name = strtolower($name);
foreach ($this->cacheCookies as $name) {
if (isset($all[$name])) {
$filtered[$name] = $all[$name];
}
}
$all = $filtered;
}
$data['cookies'] = $all;
}

/**
* Inserts (or filters/ignores according to config) response headers into a cache data array.
* @param Response $response the response.
* @param array $data the cache data.
*/
private function insertResponseHeaderCollectionIntoData(Response $response, array &$data)
{
if ($this->cacheHeaders === false) {
return;
}

$all = $response->headers->toArray(true);
if (is_array($this->cacheHeaders)) {
$filtered = [];
foreach ($this->cacheHeaders as $name) {
if (isset($all[$name])) {
$filtered[$name] = $all[$name];
}
}
$all = $filtered;
}
$data[$collectionName] = $all;
$data['headers'] = $all;
}

/**
Expand Down
60 changes: 39 additions & 21 deletions framework/web/HeaderCollection.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,13 @@
class HeaderCollection extends BaseObject implements \IteratorAggregate, \ArrayAccess, \Countable
{
/**
* @var array the headers in this collection (indexed by the header names)
* @var array the headers in this collection (indexed by the normalized header names)
*/
private $_headers = [];
/**
* @var array the original names of the headers (indexed by the normalized header names)
*/
private $_originalHeaderNames = [];


/**
Expand Down Expand Up @@ -72,9 +76,9 @@ public function getCount()
*/
public function get($name, $default = null, $first = true)
{
$name = strtolower($name);
if (isset($this->_headers[$name])) {
return $first ? reset($this->_headers[$name]) : $this->_headers[$name];
$normalizedName = strtolower($name);
if (isset($this->_headers[$normalizedName])) {
return $first ? reset($this->_headers[$normalizedName]) : $this->_headers[$normalizedName];
}

return $default;
Expand All @@ -89,8 +93,9 @@ public function get($name, $default = null, $first = true)
*/
public function set($name, $value = '')
{
$name = strtolower($name);
$this->_headers[$name] = (array) $value;
$normalizedName = strtolower($name);
$this->_headers[$normalizedName] = (array) $value;
$this->_originalHeaderNames[$normalizedName] = $name;

return $this;
}
Expand All @@ -105,8 +110,11 @@ public function set($name, $value = '')
*/
public function add($name, $value)
{
$name = strtolower($name);
$this->_headers[$name][] = $value;
$normalizedName = strtolower($name);
$this->_headers[$normalizedName][] = $value;
if (!\array_key_exists($normalizedName, $this->_originalHeaderNames)) {
$this->_originalHeaderNames[$normalizedName] = $name;
}

return $this;
}
Expand All @@ -120,9 +128,10 @@ public function add($name, $value)
*/
public function setDefault($name, $value)
{
$name = strtolower($name);
if (empty($this->_headers[$name])) {
$this->_headers[$name][] = $value;
$normalizedName = strtolower($name);
if (empty($this->_headers[$normalizedName])) {
$this->_headers[$normalizedName][] = $value;
$this->_originalHeaderNames[$normalizedName] = $name;
}

return $this;
Expand All @@ -135,9 +144,7 @@ public function setDefault($name, $value)
*/
public function has($name)
{
$name = strtolower($name);

return isset($this->_headers[$name]);
return isset($this->_headers[strtolower($name)]);
}

/**
Expand All @@ -147,10 +154,10 @@ public function has($name)
*/
public function remove($name)
{
$name = strtolower($name);
if (isset($this->_headers[$name])) {
$value = $this->_headers[$name];
unset($this->_headers[$name]);
$normalizedName = strtolower($name);
if (isset($this->_headers[$normalizedName])) {
$value = $this->_headers[$normalizedName];
unset($this->_headers[$normalizedName], $this->_originalHeaderNames[$normalizedName]);
return $value;
}

Expand All @@ -163,16 +170,25 @@ public function remove($name)
public function removeAll()
{
$this->_headers = [];
$this->_originalHeaderNames = [];
}

/**
* Returns the collection as a PHP array.
* @return array the array representation of the collection.
* The array keys are header names, and the array values are the corresponding header values.
* Since 2.0.45 you can pass true here to get the headers list of original header names (case-sensitive) instead of
* default normalized (case-insensitive) ones.
*/
public function toArray()
public function toArray($originalNames = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

This change breaks BC - if someone overwrites this method he will get fatal error after this change.

Copy link
Member

Choose a reason for hiding this comment

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

@bizley would you please add it to UPGRADE?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why we're introducing such BC break in the first place? We could add another method.

Copy link
Contributor

Choose a reason for hiding this comment

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

@samdark I agree with @rob006 - it's a method signature change for a public method, we should not do that.

Copy link
Member Author

@bizley bizley Jan 27, 2022

Choose a reason for hiding this comment

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

Yes, right, why haven't I thought of that. I'll add a new one.

{
return $this->_headers;
if ($originalNames === false) {
return $this->_headers;
}

return \array_map(function ($normalizedName) {
return $this->_headers[$normalizedName];
}, \array_flip($this->_originalHeaderNames));
}

/**
Expand All @@ -182,7 +198,9 @@ public function toArray()
*/
public function fromArray(array $array)
{
$this->_headers = array_change_key_case($array, CASE_LOWER);
foreach ($array as $name => $value) {
$this->set($name, $value);
}
}

/**
Expand Down
12 changes: 12 additions & 0 deletions tests/framework/filters/PageCacheTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,16 @@ public function cacheTestCaseProvider()
'test-header-2' => false,
],
]],
[[
'name' => 'originalNameHeaders',
'properties' => [
'cacheHeaders' => ['Test-Header-1'],
],
'headers' => [
'Test-Header-1' => true,
'Test-Header-2' => false,
],
]],

// All together
[[
Expand Down Expand Up @@ -233,10 +243,12 @@ public function testCache($testCase)
}
// Headers
if (isset($testCase['headers'])) {
$headersExpected = Yii::$app->response->headers->toArray(true);
foreach ($testCase['headers'] as $name => $expected) {
$this->assertSame($expected, Yii::$app->response->headers->has($name), $testCase['name']);
if ($expected) {
$this->assertSame($headers[$name], Yii::$app->response->headers->get($name), $testCase['name']);
$this->assertArrayHasKey($name, $headersExpected);
}
}
}
Expand Down
88 changes: 88 additions & 0 deletions tests/framework/web/HeaderCollectionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,92 @@ public function testFromArray()
]);
$this->assertEquals($location, $headerCollection->get('Location'));
}

public function testSetter()
{
$headerCollection = new HeaderCollection();

$this->assertSame('default', $headerCollection->get('X-Header', 'default'));
$this->assertFalse($headerCollection->has('X-Header'));

$headerCollection->set('X-Header', '1');
$this->assertTrue($headerCollection->has('X-Header'));
$this->assertTrue($headerCollection->offsetExists('X-Header'));
$this->assertSame('1', $headerCollection->get('X-Header'));
$this->assertSame(['1'], $headerCollection->get('X-Header', null, false));
$this->assertTrue($headerCollection->has('x-header'));
$this->assertSame('1', $headerCollection->get('x-header'));
$this->assertSame(['1'], $headerCollection->get('x-header', null, false));
$this->assertSame('1', $headerCollection->get('x-hEadER'));
$this->assertSame(['1'], $headerCollection->get('x-hEadER', null, false));
$this->assertSame(['x-header' => ['1']], $headerCollection->toArray());
$this->assertSame(['X-Header' => ['1']], $headerCollection->toArray(true));

$headerCollection->set('X-HEADER', '2');
$this->assertSame('2', $headerCollection->get('X-Header'));
$this->assertSame('2', $headerCollection->get('x-header'));
$this->assertSame('2', $headerCollection->get('x-hEadER'));
$this->assertSame(['x-header' => ['2']], $headerCollection->toArray());
$this->assertSame(['X-HEADER' => ['2']], $headerCollection->toArray(true));

$headerCollection->offsetSet('X-HEADER', '3');
$this->assertSame('3', $headerCollection->get('X-Header'));
}

public function testSetterDefault()
{
$headerCollection = new HeaderCollection();
$headerCollection->setDefault('X-Header', '1');
$this->assertSame(['1'], $headerCollection->get('X-Header', null, false));

$headerCollection->setDefault('X-Header', '2');
$this->assertSame(['1'], $headerCollection->get('X-Header', null, false));
}

public function testAdder()
{
$headerCollection = new HeaderCollection();
$headerCollection->add('X-Header', '1');
$this->assertSame('1', $headerCollection->get('X-Header'));
$this->assertSame(['1'], $headerCollection->get('X-Header', null, false));
$this->assertSame('1', $headerCollection->get('x-header'));
$this->assertSame(['1'], $headerCollection->get('x-header', null, false));
$this->assertSame('1', $headerCollection->get('x-hEadER'));
$this->assertSame(['1'], $headerCollection->get('x-hEadER', null, false));
$this->assertSame(['x-header' => ['1']], $headerCollection->toArray());
$this->assertSame(['X-Header' => ['1']], $headerCollection->toArray(true));

$headerCollection->add('X-HEADER', '2');
$this->assertSame('1', $headerCollection->get('X-Header'));
$this->assertSame('1', $headerCollection->get('x-header'));
$this->assertSame('1', $headerCollection->get('x-hEadER'));
$this->assertSame(['1', '2'], $headerCollection->get('x-header', null, false));
$this->assertSame(['x-header' => ['1', '2']], $headerCollection->toArray());
$this->assertSame(['X-Header' => ['1', '2']], $headerCollection->toArray(true));
}

public function testRemover()
{
$headerCollection = new HeaderCollection();
$headerCollection->add('X-Header', '1');
$this->assertSame(1, $headerCollection->count());
$this->assertSame(1, $headerCollection->getCount());
$headerCollection->remove('X-Header');
$this->assertSame(0, $headerCollection->count());

$headerCollection->add('X-Header', '1');
$this->assertSame(1, $headerCollection->count());
$headerCollection->remove('x-header');
$this->assertSame(0, $headerCollection->count());

$headerCollection->add('X-Header', '1');
$headerCollection->offsetUnset('X-HEADER');
$this->assertSame(0, $headerCollection->count());

$headerCollection->add('X-Header-1', '1');
$headerCollection->add('X-Header-2', '1');
$this->assertSame(2, $headerCollection->count());
$headerCollection->removeAll();
$this->assertSame(0, $headerCollection->count());
}
}