From 6e6e9a8851f93c2012b4adcd0ab6f3ac6c407420 Mon Sep 17 00:00:00 2001
From: bjorn <bjorn@swis.nl>
Date: Mon, 22 Jul 2024 15:46:57 +0200
Subject: [PATCH] Update vary implementation
---
core/core.services.yml | 2 +
.../VaryHeaderResponseSubscriber.php | 52 +++++++++++
.../LanguageNegotiationBrowser.php | 15 +--
...uageBrowserDetectionAcceptLanguageTest.php | 14 +--
.../src/StackMiddleware/PageCache.php | 91 ++++++++++++++++---
.../src/Functional/PageCacheVaryTest.php | 60 ++++++++++++
.../src/Controller/TestVaryController.php | 62 +++++++++++++
.../test_page_test/test_page_test.routing.yml | 7 ++
8 files changed, 276 insertions(+), 27 deletions(-)
create mode 100644 core/lib/Drupal/Core/EventSubscriber/VaryHeaderResponseSubscriber.php
create mode 100644 core/modules/page_cache/tests/src/Functional/PageCacheVaryTest.php
create mode 100644 core/modules/system/tests/modules/test_page_test/src/Controller/TestVaryController.php
diff --git a/core/core.services.yml b/core/core.services.yml
index 2ee184054a7b..1ffd18f2354d 100644
--- a/core/core.services.yml
+++ b/core/core.services.yml
@@ -370,6 +370,8 @@ services:
tags:
- { name: page_cache_response_policy }
- { name: dynamic_page_cache_response_policy }
+ vary_header_response_subscriber:
+ class: Drupal\Core\EventSubscriber\VaryHeaderResponseSubscriber
config.manager:
class: Drupal\Core\Config\ConfigManager
arguments: ['@entity_type.manager', '@config.factory', '@config.typed', '@string_translation', '@config.storage', '@event_dispatcher', '@entity.repository', '@extension.path.resolver']
diff --git a/core/lib/Drupal/Core/EventSubscriber/VaryHeaderResponseSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/VaryHeaderResponseSubscriber.php
new file mode 100644
index 000000000000..42963b09d412
--- /dev/null
+++ b/core/lib/Drupal/Core/EventSubscriber/VaryHeaderResponseSubscriber.php
@@ -0,0 +1,52 @@
+<?php
+
+namespace Drupal\Core\EventSubscriber;
+
+use Drupal\Core\Cache\CacheableMetadata;
+use Drupal\Core\Cache\CacheableResponseInterface;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\HttpKernel\Event\ResponseEvent;
+use Symfony\Component\HttpKernel\KernelEvents;
+
+class VaryHeaderResponseSubscriber implements EventSubscriberInterface {
+
+ protected array $vary = [];
+
+ public function addVaryHeader(string $header): void {
+ if (!in_array($header, $this->vary)) {
+ $this->vary[] = $header;
+ }
+ }
+
+ public function onRespond(ResponseEvent $event) {
+ if (!$event->isMainRequest()) {
+ return;
+ }
+
+ if ($this->vary) {
+ $response = $event->getResponse();
+ $response->setVary($this->vary, false);
+ if ($response instanceof CacheableResponseInterface) {
+ $metadata = new CacheableMetadata();
+ $contexts = [];
+ foreach ($this->vary as $vary_header) {
+ $contexts[] = 'headers:' . $vary_header;
+ }
+ $metadata->addCacheContexts($contexts);
+ $response->addCacheableDependency($metadata);
+ }
+ }
+ }
+
+ public static function getSubscribedEvents() {
+ /**
+ * Either Drupal\Tests\page_cache\Functional\PageCacheVaryTest::testPageCacheWithVary
+ * fails or Drupal\Tests\language\Functional\LanguageBrowserDetectionAcceptLanguageTest::testAcceptLanguageEmptyDefault
+ * fails if you remove one of those. Not sure why
+ */
+ $events[KernelEvents::RESPONSE][] = ['onRespond', -100];
+ $events[KernelEvents::RESPONSE][] = ['onRespond', 100];
+ return $events;
+ }
+
+}
diff --git a/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationBrowser.php b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationBrowser.php
index a3a5df8ae63b..da0b63029ffc 100644
--- a/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationBrowser.php
+++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationBrowser.php
@@ -3,6 +3,7 @@
namespace Drupal\language\Plugin\LanguageNegotiation;
use Drupal\Component\Utility\UserAgent;
+use Drupal\Core\EventSubscriber\VaryHeaderResponseSubscriber;
use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
use Drupal\Core\StringTranslation\TranslatableMarkup;
use Drupal\language\Attribute\LanguageNegotiation;
@@ -34,12 +35,18 @@ class LanguageNegotiationBrowser extends LanguageNegotiationMethodBase implement
*/
protected $pageCacheKillSwitch;
+ /**
+ * @var \Drupal\Core\EventSubscriber\VaryHeaderResponseSubscriber $varyResponseSubscriber
+ */
+ private VaryHeaderResponseSubscriber $varyResponseSubscriber;
+
/**
* {@inheritdoc}
*/
public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
$instance = new static();
$instance->pageCacheKillSwitch = $container->get('page_cache_kill_switch');
+ $instance->varyResponseSubscriber = $container->get('vary_header_response_subscriber');
return $instance;
}
@@ -49,6 +56,8 @@ public static function create(ContainerInterface $container, array $configuratio
public function getLangcode(?Request $request = NULL) {
$langcode = NULL;
+ $this->varyResponseSubscriber->addVaryHeader('Accept-Language');
+ $this->varyResponseSubscriber->addVaryHeader('Content-Language');
if ($this->languageManager && $request && $request->server->get('HTTP_ACCEPT_LANGUAGE')) {
$http_accept_language = $request->server->get('HTTP_ACCEPT_LANGUAGE');
$langcodes = array_keys($this->languageManager->getLanguages());
@@ -56,12 +65,6 @@ public function getLangcode(?Request $request = NULL) {
$langcode = UserAgent::getBestMatchingLangcode($http_accept_language, $langcodes, $mappings);
}
- // Internal page cache with multiple languages and browser negotiation
- // could lead to wrong cached sites. Therefore disabling the internal page
- // cache.
- // @todo Solve more elegantly in https://www.drupal.org/node/2430335.
- $this->pageCacheKillSwitch->trigger();
-
return $langcode;
}
diff --git a/core/modules/language/tests/src/Functional/LanguageBrowserDetectionAcceptLanguageTest.php b/core/modules/language/tests/src/Functional/LanguageBrowserDetectionAcceptLanguageTest.php
index a6d9d84b2d73..b3f0e0c71987 100644
--- a/core/modules/language/tests/src/Functional/LanguageBrowserDetectionAcceptLanguageTest.php
+++ b/core/modules/language/tests/src/Functional/LanguageBrowserDetectionAcceptLanguageTest.php
@@ -10,7 +10,7 @@
/**
* Tests browser language detection with different accept-language headers.
*
- * @group language
+ * @group vary
*/
class LanguageBrowserDetectionAcceptLanguageTest extends BrowserTestBase {
@@ -89,32 +89,32 @@ public function testAcceptLanguageEmptyDefault(): void {
$this->drupalGet('/system-test/echo/language test', [], ['Accept-Language' => 'en']);
$this->assertSession()->responseHeaderEquals('Content-Language', 'en');
- $this->assertSession()->responseHeaderDoesNotExist('X-Drupal-Cache');
+ $this->assertSession()->responseHeaderEquals('X-Drupal-Cache', 'MISS');
// Check with UK browser.
$this->drupalGet('/system-test/echo/language test', [], ['Accept-Language' => 'en-UK,en']);
$this->assertSession()->responseHeaderEquals('Content-Language', 'en');
- $this->assertSession()->responseHeaderDoesNotExist('X-Drupal-Cache');
+ $this->assertSession()->responseHeaderEquals('X-Drupal-Cache', 'MISS');
// Check with french browser.
$this->drupalGet('/system-test/echo/language test', [], ['Accept-Language' => 'fr-FR,fr']);
$this->assertSession()->responseHeaderEquals('Content-Language', 'fr');
- $this->assertSession()->responseHeaderDoesNotExist('X-Drupal-Cache');
+ $this->assertSession()->responseHeaderEquals('X-Drupal-Cache', 'MISS');
// Check with browser without language settings - should return fallback language.
$this->drupalGet('/system-test/echo/language test', [], ['Accept-Language' => '']);
$this->assertSession()->responseHeaderEquals('Content-Language', 'en');
- $this->assertSession()->responseHeaderDoesNotExist('X-Drupal-Cache');
+ $this->assertSession()->responseHeaderEquals('X-Drupal-Cache', 'MISS');
// Check with french browser again.
$this->drupalGet('/system-test/echo/language test', [], ['Accept-Language' => 'fr-FR,fr']);
$this->assertSession()->responseHeaderEquals('Content-Language', 'fr');
- $this->assertSession()->responseHeaderDoesNotExist('X-Drupal-Cache');
+ $this->assertSession()->responseHeaderEquals('X-Drupal-Cache', 'HIT');
// Check with UK browser.
$this->drupalGet('/system-test/echo/language test', [], ['Accept-Language' => 'en-UK,en']);
$this->assertSession()->responseHeaderEquals('Content-Language', 'en');
- $this->assertSession()->responseHeaderDoesNotExist('X-Drupal-Cache');
+ $this->assertSession()->responseHeaderEquals('X-Drupal-Cache', 'HIT');
// Check if prefixed URLs are still cached.
$this->drupalGet('/en/system-test/echo/language test', [], ['Accept-Language' => 'en']);
diff --git a/core/modules/page_cache/src/StackMiddleware/PageCache.php b/core/modules/page_cache/src/StackMiddleware/PageCache.php
index efe4e3b99c29..16d33f4e48c3 100644
--- a/core/modules/page_cache/src/StackMiddleware/PageCache.php
+++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
@@ -48,11 +48,18 @@ class PageCache implements HttpKernelInterface {
protected $responsePolicy;
/**
- * The cache ID for the (master) request.
+ * The cache IDs for the (main) request.
*
- * @var string
+ * @var string[]
*/
- protected $cid;
+ protected array $cids = [];
+
+ /**
+ * The request used for generating the cache ID.
+ *
+ * @var \Symfony\Component\HttpFoundation\Request
+ */
+ protected Request $requestUsedForCid;
/**
* Constructs a PageCache object.
@@ -304,10 +311,23 @@ protected function storeResponse(Request $request, Response $response) {
*/
protected function get(Request $request, $allow_invalid = FALSE) {
$cid = $this->getCacheId($request);
- if ($cache = $this->cache->get($cid, $allow_invalid)) {
- return $cache->data;
+
+ $cache = $this->cache->get($cid, $allow_invalid);
+ if (!$cache) {
+ return FALSE;
}
- return FALSE;
+
+ // Cached response may contain Vary header.
+ // Try to find exact match by adding relevant request headers to cache ID.
+ $vary = $cache->data->getVary();
+ $cid_vary = $this->getCacheId($request, $vary);
+ if ($cid_vary !== $cid) {
+ $cache = $this->cache->get($cid_vary, $allow_invalid);
+ }
+ if (!$cache) {
+ return FALSE;
+ }
+ return $cache->data;
}
/**
@@ -335,6 +355,14 @@ protected function get(Request $request, $allow_invalid = FALSE) {
protected function set(Request $request, Response $response, $expire, array $tags) {
$cid = $this->getCacheId($request);
$this->cache->set($cid, $response, $expire, $tags);
+ // Additionally, set cache item for varied response.
+ $vary = $response->getVary();
+ if ($vary) {
+ $cid_vary = $this->getCacheId($request, $vary);
+ if ($cid_vary !== $cid) {
+ $this->cache->set($cid_vary, $response, $expire, $tags);
+ }
+ }
}
/**
@@ -342,25 +370,60 @@ protected function set(Request $request, Response $response, $expire, array $tag
*
* @param \Symfony\Component\HttpFoundation\Request $request
* A request object.
+ * @param array $vary_headers
+ * Optional array of vary headers in response object.
*
* @return string
* The cache ID for this request.
*/
- protected function getCacheId(Request $request) {
+ protected function getCacheId(Request $request, array $vary_headers = []) {
+ // Get the cache IDs key.
+ $key = '|';
+ if (!empty($vary_headers)) {
+ sort($vary_headers);
+ $key = implode('|', $vary_headers);
+ }
+
// Once a cache ID is determined for the request, reuse it for the duration
// of the request. This ensures that when the cache is written, it is only
// keyed on request data that was available when it was read. For example,
// the request format might be NULL during cache lookup and then set during
// routing, in which case we want to key on NULL during writing, since that
// will be the value during lookups for subsequent requests.
- if (!isset($this->cid)) {
- $cid_parts = [
- $request->getSchemeAndHttpHost() . $request->getRequestUri(),
- $request->getRequestFormat(NULL),
- ];
- $this->cid = implode(':', $cid_parts);
+ if (isset($this->cids[$key])) {
+ return $this->cids[$key];
+ }
+
+ if (!isset($this->requestUsedForCid)) {
+ $this->requestUsedForCid = clone $request;
+ }
+
+ $cid_parts = [
+ $this->requestUsedForCid->getSchemeAndHttpHost() . $this->requestUsedForCid->getRequestUri(),
+ $this->requestUsedForCid->getRequestFormat(NULL),
+ ];
+
+ // Need to separate cache IDs for responses that contain Vary headers.
+ foreach ($vary_headers as $vary_header) {
+ $vary_header = strtolower($vary_header);
+ // Vary:Cookie is handled separately.
+ // @see PageCache::get()
+ if ($vary_header === 'cookie') {
+ continue;
+ }
+
+ $request_header = $this->requestUsedForCid->headers->get($vary_header);
+
+ if (!$request_header) {
+ $request_header = '_';
+ }
+
+ $cid_parts[] = $vary_header . ':' . $request_header;
}
- return $this->cid;
+
+ $this->cids[$key] = implode(':', $cid_parts);
+
+ return $this->cids[$key];
}
}
diff --git a/core/modules/page_cache/tests/src/Functional/PageCacheVaryTest.php b/core/modules/page_cache/tests/src/Functional/PageCacheVaryTest.php
new file mode 100644
index 000000000000..c212c66f4e24
--- /dev/null
+++ b/core/modules/page_cache/tests/src/Functional/PageCacheVaryTest.php
@@ -0,0 +1,60 @@
+<?php
+
+namespace Drupal\Tests\page_cache\Functional;
+
+use Drupal\Tests\BrowserTestBase;
+use Drupal\Tests\system\Functional\Cache\AssertPageCacheContextsAndTagsTrait;
+
+/**
+ * Enables the page cache and tests it with various HTTP requests.
+ *
+ * @group vary
+ */
+class PageCacheVaryTest extends BrowserTestBase {
+
+ use AssertPageCacheContextsAndTagsTrait;
+
+ protected $dumpHeaders = TRUE;
+
+ /**
+ * Modules to enable.
+ *
+ * @var array
+ */
+ protected static $modules = ['test_page_test', 'system_test', 'entity_test'];
+
+ /**
+ * {@inheritdoc}
+ */
+ protected $defaultTheme = 'stark';
+
+ /**
+ * {@inheritdoc}
+ */
+ protected function setUp(): void {
+ parent::setUp();
+
+ $this->config('system.site')
+ ->set('name', 'Drupal')
+ ->set('page.front', '/test-page')
+ ->save();
+ }
+
+ /**
+ * Tests that custom vary header cached.
+ */
+ public function testPageCacheWithVary(): void {
+ $config = $this->config('system.performance');
+ $config->set('cache.page.max_age', 300);
+ $config->save();
+
+ $this->drupalGet('/test-page-vary-cache', [], ['X-Vary-Test' => 'TestVary1']);
+ $this->assertSession()->pageTextContains('TestVary1');
+ $this->drupalGet('/test-page-vary-cache', [], ['X-Vary-Test' => 'TestVary1']);
+ $this->assertSession()->responseHeaderContains('X-Drupal-Cache', 'HIT');
+ $this->drupalGet('/test-page-vary-cache', [], ['X-Vary-Test' => 'TestVary2']);
+ $this->assertSession()->pageTextNotContains('TestVary1');
+ $this->assertSession()->pageTextContains('TestVary2');
+ }
+
+}
diff --git a/core/modules/system/tests/modules/test_page_test/src/Controller/TestVaryController.php b/core/modules/system/tests/modules/test_page_test/src/Controller/TestVaryController.php
new file mode 100644
index 000000000000..4d7153c77d56
--- /dev/null
+++ b/core/modules/system/tests/modules/test_page_test/src/Controller/TestVaryController.php
@@ -0,0 +1,62 @@
+<?php
+
+namespace Drupal\test_page_test\Controller;
+
+use Drupal\Core\Controller\ControllerBase;
+use Drupal\Core\EventSubscriber\VaryHeaderResponseSubscriber;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Symfony\Component\HttpFoundation\Request;
+
+/**
+ * Page controller that is aware of Vary header.
+ *
+ * @internal
+ */
+class TestVaryController extends ControllerBase {
+
+ /**
+ * The vary policy.
+ *
+ * @var VaryHeaderResponseSubscriber
+ */
+ protected $varyResponseSubscriber;
+
+ /**
+ * Constructs the TestVaryController object.
+ *
+ * @param VaryHeaderResponseSubscriber $varyResponseSubscriber
+ * The vary policy.
+ */
+ public function __construct(VaryHeaderResponseSubscriber $varyResponseSubscriber) {
+ $this->varyResponseSubscriber = $varyResponseSubscriber;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public static function create(ContainerInterface $container) {
+ return new static(
+ $container->get('vary_header_response_subscriber')
+ );
+ }
+
+ /**
+ * Page that displays content that depends on custom request header.
+ *
+ * @param \Symfony\Component\HttpFoundation\Request $request
+ * The request.
+ *
+ * @return array
+ * Renderable array expected by renderer service.
+ */
+ public function pageVary(Request $request): array {
+ $expected = $request->headers->get('x-vary-test');
+ $build = [
+ '#title' => 'Vary test',
+ '#markup' => $expected ?: 'default header value',
+ ];
+ $this->varyResponseSubscriber->addVaryHeader('X-Vary-Test');
+ return $build;
+ }
+
+}
diff --git a/core/modules/system/tests/modules/test_page_test/test_page_test.routing.yml b/core/modules/system/tests/modules/test_page_test/test_page_test.routing.yml
index f3744df5ee61..6be26f930579 100644
--- a/core/modules/system/tests/modules/test_page_test/test_page_test.routing.yml
+++ b/core/modules/system/tests/modules/test_page_test/test_page_test.routing.yml
@@ -152,3 +152,10 @@ test_page_test.test_page_var_dump:
_controller: '\Drupal\test_page_test\Controller\TestPageTestController::testPageVarDump'
requirements:
_access: 'TRUE'
+
+test_page_test.test_vary_cache:
+ path: '/test-page-vary-cache'
+ defaults:
+ _controller: '\Drupal\test_page_test\Controller\TestVaryController::pageVary'
+ requirements:
+ _access: 'TRUE'
--
GitLab