WIP: support for targeted cache control #59219
Conversation
|
Hey! Thanks for your PR. You are targeting branch "7.3" but it seems your PR description refers to branch "7.3 for features". Cheers! Carsonbot |
3309794 to
42cb026
Compare
|
@nicolas-grekas finally got to wrap up the pull request for targeted cache-control. we had discussed at the symfony con hackday. i listed what is still to do, but before i invest the time to clean up, i would be happy for feedback if this is the right approach and if i should adjust something. |
| @@ -20,14 +20,21 @@ | |||
| */ | |||
| class HeaderBag implements \IteratorAggregate, \Countable, \Stringable | |||
| { | |||
| public const DEFAULT_CACHE_CONTROL_TARGET = ''; | |||
There was a problem hiding this comment.
i picked empty string to not collide with any valid target like default-cache-control.
| protected const UPPER = '_ABCDEFGHIJKLMNOPQRSTUVWXYZ'; | ||
| protected const LOWER = '-abcdefghijklmnopqrstuvwxyz'; | ||
|
|
||
| /** | ||
| * @var array<string, list<string|null>> | ||
| */ | ||
| protected array $headers = []; | ||
| protected array $cacheControl = []; |
There was a problem hiding this comment.
we need to add __get / __set to build and update this on the fly if somebody extended HeaderBag and manipulates those fields directly.
| } | ||
|
|
||
| return $this->headers; | ||
| $headers = $this->headers; | ||
| // edge case: what if extending class directly changed Cache-Control in the $headers array? |
There was a problem hiding this comment.
should we check for the existence of the header we are about to create and not set it in that case?
There was a problem hiding this comment.
also, should we check if the instruction set is empty to not create an empty header, or should we do as we do now and set the emtpy header with no value? the presence of the header could be used by something downstream even if there are no values.
| @@ -129,27 +148,43 @@ public function get(string $key, ?string $default = null): ?string | |||
| */ | |||
| public function set(string $key, string|array|null $values, bool $replace = true): void | |||
| { | |||
| $key = strtr($key, self::UPPER, self::LOWER); | |||
| $uniqueKey = strtr($key, self::UPPER, self::LOWER); | |||
There was a problem hiding this comment.
for naming consistency, no logic change apart from checking for cache-control just below.
| */ | ||
| public function getCacheControl(string $target = self::DEFAULT_CACHE_CONTROL_TARGET): CacheControl | ||
| { | ||
| // TODO do we need to lowercase the targets as well, and track the desired case as we do for the headers in ResponseHeaderBag |
There was a problem hiding this comment.
should we lowercase? and should we preserve original case to recreate the header exactly as desired, even tough its supposed to be case insensitive?
| return match ($key) { | ||
| 'set-cookie' => array_map('strval', $this->getCookies()), | ||
| 'cache-control' => [$this->computeCacheControl()->getCacheControlHeader()], | ||
| default => $headers[$key] ?? [], |
There was a problem hiding this comment.
TODO: we need to also match targeted cache control headers, not only the default one
| } | ||
|
|
||
| foreach ($this->getCookies() as $cookie) { | ||
| $headers['set-cookie'][] = (string) $cookie; | ||
| } | ||
| if (\array_key_exists(self::DEFAULT_CACHE_CONTROL_TARGET, $this->cacheControls)) { |
There was a problem hiding this comment.
todo: also build the targeted cache control headers, not only the default one
42cb026 to
2608075
Compare
| [1] => Cache-Control: no-cache, private | ||
| [2] => Date: Sat, 12 Nov 1955 20:04:00 GMT | ||
| [1] => Date: Sat, 12 Nov 1955 20:04:00 GMT | ||
| [2] => Cache-Control: no-cache, private |
There was a problem hiding this comment.
cache control now comes after regular headers
| @@ -57,7 +57,7 @@ public function testCacheControlHeader() | |||
| $this->assertFalse($bag->hasCacheControlDirective('max-age')); | |||
|
|
|||
| $bag = new ResponseHeaderBag(['Expires' => 'Wed, 16 Feb 2011 14:17:43 GMT']); | |||
| $this->assertEquals('private, must-revalidate', $bag->get('Cache-Control')); | |||
| $this->assertEquals('must-revalidate, private', $bag->get('Cache-Control')); | |||
There was a problem hiding this comment.
i am actually not sure why the order changes, but the order has no semantical meaning
https://datatracker.ietf.org/doc/rfc9213/ defines additional headers to target cache-control to specific reverse proxies. this PR refactors the header bag: * extract cache control handling into separate class * support having additional targets with cache control
2608075 to
55c5104
Compare
| @@ -221,7 +229,7 @@ public function getCookies(string $format = self::COOKIES_FLAT): array | |||
| */ | |||
| public function clearCookie(string $name, ?string $path = '/', ?string $domain = null, bool $secure = false, bool $httpOnly = true, ?string $sameSite = null /* , bool $partitioned = false */): void | |||
| { | |||
| $partitioned = 6 < \func_num_args() ? \func_get_arg(6) : false; | |||
| $partitioned = 6 < \func_num_args() ? func_get_arg(6) : false; | |||
There was a problem hiding this comment.
not related, cs fixer insists on this change even though i did not change this line...
(running cs fixer over the whole project, there is a bunch of changes - might be good to do that globally to avoid random changes in every pull request)
|
@nicolas-grekas i would be happy to get some feedback here :-) |
|
@dbu comming from your comment on the other PR, I will try to check this as well thx |
|
From API point I missing adjustments in the Response to write and read to a specific Target Cache Control as that methods already are in use: $response
->setPrivate(cacheControlHeader: 'My-Cache-Control')
->setImmutable(true, cacheControlHeader: 'My-Cache-Control')
->setMaxAge(240, cacheControlHeader: 'My-Cache-Control')
->setSharedMaxAge(480, cacheControlHeader: 'My-Cache-Control')
->setStaleIfError(240, cacheControlHeader: 'My-Cache-Control')
->setStaleWhileRevalidate(240, cacheControlHeader: 'My-Cache-Control')$response->mustRevalidate(cacheControlHeader: 'My-Cache-Control');
$response->isImmutable(cacheControlHeader: 'My-Cache-Control');
$response->getTtl(cacheControlHeader: 'My-Cache-Control');
$response->isFresh(cacheControlHeader: 'My-Cache-Control');
$response->isCacheable(cacheControlHeader: 'My-Cache-Control');I know about the getCacheControl but personally don't feel for me as a nice API to communicate edit the response cache control. As I personally not sure if I would make the Think that would make it easier for HttpCache adopt the target cache control header, also lot easier to support a wider range of Symfony Versions. |
https://datatracker.ietf.org/doc/rfc9213/ defines additional headers to target cache-control to specific reverse proxies. this PR refactors the header bag:
This is the current work in progress for targeted cache control.
TODO
''ornullfor example)HeaderBag::$cacheControlarray (that array should have been private, but as it is not, we need to provide BC)__get/__set)#SymfonyHackday