From 0871d28bf14cb6a0e9b95baeb55066df00dae944 Mon Sep 17 00:00:00 2001 From: catch <6915-catch@users.noreply.drupalcode.org> Date: Fri, 30 Aug 2024 08:52:31 +0900 Subject: [PATCH] Issue #3452181 by kristiaanvandeneynde, smustgrave, mxr576, catch: VariationCache needs to be more defensive about cache context manipulation to avoid broken redirects (cherry picked from commit 97f169b63ad0f10a2cce597378cfa0c073ccc4de) --- core/lib/Drupal/Core/Cache/VariationCache.php | 45 ++++- .../Tests/Core/Cache/VariationCacheTest.php | 189 +++++++++++++++++- 2 files changed, 231 insertions(+), 3 deletions(-) diff --git a/core/lib/Drupal/Core/Cache/VariationCache.php b/core/lib/Drupal/Core/Cache/VariationCache.php index 6f1bd2d058b..123f865c92f 100644 --- a/core/lib/Drupal/Core/Cache/VariationCache.php +++ b/core/lib/Drupal/Core/Cache/VariationCache.php @@ -44,7 +44,10 @@ class VariationCache implements VariationCacheInterface { $contexts = $cacheability->getCacheContexts(); if ($missing_contexts = array_diff($initial_contexts, $contexts)) { - throw new \LogicException(sprintf('The complete set of cache contexts for a variation cache item must contain all of the initial cache contexts, missing: %s.', implode(', ', $missing_contexts))); + throw new \LogicException(sprintf( + 'The complete set of cache contexts for a variation cache item must contain all of the initial cache contexts, missing: %s.', + implode(', ', $missing_contexts) + )); } // Don't store uncacheable items. @@ -94,6 +97,7 @@ class VariationCache implements VariationCacheInterface { // the data we're trying to set. Next time someone tries to load the // initial AB object, it will restore its redirect path by adding an AB // redirect step after A. + $previous_step_contexts = $initial_contexts; foreach ($chain as $chain_cid => $result) { if ($result && $result->data instanceof CacheRedirect) { $result_contexts = $result->data->getCacheContexts(); @@ -101,6 +105,44 @@ class VariationCache implements VariationCacheInterface { // Check whether we have an overlap scenario as we need to manually // create an extra redirect in that case. $common_contexts = array_intersect($result_contexts, $contexts); + + // If the only common contexts are those we've seen before, it means + // we are trying to set a redirect at an address that is completely + // different from the one that was already there. This cannot be + // allowed as it completely breaks the redirect system. + // + // Example: The value for context A is 'foo' and we are trying to + // store a redirect with AB at A:foo. Then, for a different value of + // B, we are trying to store a redirect at A:foo with AC. This makes + // no sense as there would now no longer be a way to find the first + // item that triggered the initial redirect. + // + // This usually occurs when using calculated cache contexts and the + // author tried to manually optimize them. E.g.: When using + // user.roles:anonymous and in one of the outcomes we end up varying + // by user.roles. In that case, both user.roles:anonymous and + // user.roles need to be present on the cacheable metadata, even + // though they will eventually be optimized into user.roles. The + // cache needs all the initial information to do its job and if an + // author were to manually optimize this prematurely, it would be + // impossible to properly store a redirect chain. + // + // Another way this might happen is if a new object that can specify + // cacheable metadata is instantiated without inheriting the cache + // contexts of all the logic that happened up until that point. A + // common example of this is when people immediately return the + // result of one of the factory methods on AccessResult, without + // adding the cacheability from previous access checks that did not + // lead to a value being returned. + if (!array_diff($common_contexts, $previous_step_contexts)) { + trigger_error(sprintf( + 'Trying to overwrite a cache redirect with one that has nothing in common, old one at address "%s" was pointing to "%s", new one points to "%s".', + implode(', ', $previous_step_contexts), + implode(', ', array_diff($result_contexts, $previous_step_contexts)), + implode(', ', array_diff($contexts, $previous_step_contexts)), + ), E_USER_WARNING); + } + // != is the most appropriate comparison operator here, since we // only want to know if any keys or values don't match. if ($common_contexts != $contexts) { @@ -118,6 +160,7 @@ class VariationCache implements VariationCacheInterface { } break; } + $previous_step_contexts = $result_contexts; } } diff --git a/core/tests/Drupal/Tests/Core/Cache/VariationCacheTest.php b/core/tests/Drupal/Tests/Core/Cache/VariationCacheTest.php index 1c7a34ac43d..de4625ccf65 100644 --- a/core/tests/Drupal/Tests/Core/Cache/VariationCacheTest.php +++ b/core/tests/Drupal/Tests/Core/Cache/VariationCacheTest.php @@ -397,12 +397,12 @@ class VariationCacheTest extends UnitTestCase { } /** - * Tests exception for a cache item that has incompatible variations. + * Tests exception for a cache item that has incomplete variations. * * @covers ::get * @covers ::set */ - public function testIncompatibleVariationsException(): void { + public function testIncompleteVariationsException(): void { // This should never happen. When someone first stores something in the // cache using context A and then tries to store something using context B, // something is wrong. There should always be at least one shared context at @@ -422,6 +422,191 @@ class VariationCacheTest extends UnitTestCase { $this->setVariationCacheItem('You have a nice house!', $house_cacheability, $garden_cacheability); } + /** + * Tests exception for a cache item that has an incomplete redirect. + * + * @covers ::get + * @covers ::set + */ + public function testIncompleteRedirectException(): void { + // @todo Remove in Drupal 12.0.0. For more information, see: + // https://www.drupal.org/project/drupal/issues/3468921 + set_error_handler(static function (int $errno, string $errstr): never { + throw new \LogicException($errstr, $errno); + }, E_USER_WARNING); + + // This should never happen. When we have a cache redirect at address A, + // pointing to 'A,B:foo' and then someone tries to store a cache redirect at + // A pointing to 'A,B', something is wrong. The cache contexts leading up to + // a cache redirect should always be present on the redirect itself. In this + // example, the final cache redirect should be for 'A,B:foo,B'. + $this->expectException(\LogicException::class); + $this->expectExceptionMessage('Trying to overwrite a cache redirect with one that has nothing in common, old one at address "house.type" was pointing to "garden.type:zen", new one points to "garden.type".'); + + $this->housingType = 'house'; + $house_cacheability = (new CacheableMetadata()) + ->setCacheContexts(['house.type']); + + $this->gardenType = '1'; + $calculated_garden_cacheability = (new CacheableMetadata()) + ->setCacheContexts(['house.type', 'garden.type:zen']); + + $this->setVariationCacheItem('You have a house with zen garden!', $calculated_garden_cacheability, $house_cacheability); + + $this->gardenType = 'baroque garden'; + $garden_cacheability = (new CacheableMetadata()) + ->setCacheContexts(['house.type', 'garden.type']); + + try { + $this->setVariationCacheItem('You have a house with a baroque garden!', $garden_cacheability, $house_cacheability); + } + finally { + restore_error_handler(); + } + } + + /** + * Tests exception for a cache item that has incompatible cache redirects. + * + * @covers ::get + * @covers ::set + */ + public function testIncompatibleRedirectsException(): void { + // @todo Remove in Drupal 12.0.0. For more information, see: + // https://www.drupal.org/project/drupal/issues/3468921 + set_error_handler(static function (int $errno, string $errstr): never { + throw new \LogicException($errstr, $errno); + }, E_USER_WARNING); + + // This should never happen. When someone first triggers the storing of a + // redirect using context A and then tries to store another redirect in the + // same spot using context B, something is wrong. The cache contexts of all + // previous redirects should always be present on the next redirect or item + // you're trying to store. + $this->expectException(\LogicException::class); + $this->expectExceptionMessage('Trying to overwrite a cache redirect with one that has nothing in common, old one at address "house.type" was pointing to "garden.type", new one points to "house.orientation".'); + + $this->housingType = 'house'; + $house_cacheability = (new CacheableMetadata()) + ->setCacheContexts(['house.type']); + + $this->gardenType = 'garden'; + $garden_cacheability = (new CacheableMetadata()) + ->setCacheContexts(['house.type', 'garden.type']); + + $this->houseOrientation = 'north'; + $orientation_cacheability = (new CacheableMetadata()) + ->setCacheContexts(['house.type', 'house.orientation']); + + $this->setVariationCacheItem('You have a nice house with a garden!', $garden_cacheability, $house_cacheability); + try { + $this->setVariationCacheItem('You have a nice north-facing house!', $orientation_cacheability, $house_cacheability); + } + finally { + restore_error_handler(); + } + } + + /** + * Tests the same as above, but with more redirects. + * + * @covers ::get + * @covers ::set + * + * @depends testIncompatibleRedirectsException + */ + public function testIncompatibleChainedRedirectsException(): void { + // @todo Remove in Drupal 12.0.0. For more information, see: + // https://www.drupal.org/project/drupal/issues/3468921 + set_error_handler(static function (int $errno, string $errstr): never { + throw new \LogicException($errstr, $errno); + }, E_USER_WARNING); + + $this->expectException(\LogicException::class); + $this->expectExceptionMessage('Trying to overwrite a cache redirect with one that has nothing in common, old one at address "house.type, garden.type" was pointing to "house.orientation", new one points to "solar.type".'); + + $this->housingType = 'house'; + $house_cacheability = (new CacheableMetadata()) + ->setCacheContexts(['house.type']); + + $this->gardenType = 'no-garden'; + $garden_cacheability = (new CacheableMetadata()) + ->setCacheContexts(['house.type', 'garden.type']); + + // This should set a redirect at ht.house specifying garden.type. So the + // redirects below should find this redirect to be fine before getting to + // the problematic one. + $this->setVariationCacheItem('You have a nice house with no garden!', $garden_cacheability, $house_cacheability); + $this->gardenType = 'garden'; + + $this->houseOrientation = 'north'; + $orientation_cacheability = (new CacheableMetadata()) + ->setCacheContexts(['house.type', 'garden.type', 'house.orientation']); + + $this->solarType = 'solar'; + $solar_cacheability = (new CacheableMetadata()) + ->setCacheContexts(['house.type', 'garden.type', 'solar.type']); + + $this->setVariationCacheItem('You have a nice north-facing house with a garden!', $orientation_cacheability, $house_cacheability); + try { + $this->setVariationCacheItem('You have a nice house with solar panels and a garden!', $solar_cacheability, $house_cacheability); + } + finally { + restore_error_handler(); + } + } + + /** + * Tests the same as above, but even more complex. + * + * @covers ::get + * @covers ::set + * + * @depends testIncompatibleChainedRedirectsException + */ + public function testIncompatibleChainedRedirectsComplexException(): void { + // @todo Remove in Drupal 12.0.0. For more information, see: + // https://www.drupal.org/project/drupal/issues/3468921 + set_error_handler(static function (int $errno, string $errstr): never { + throw new \LogicException($errstr, $errno); + }, E_USER_WARNING); + + $this->expectException(\LogicException::class); + $this->expectExceptionMessage('Trying to overwrite a cache redirect with one that has nothing in common, old one at address "house.type, garden.type" was pointing to "house.orientation", new one points to "solar.type".'); + + $this->housingType = 'house'; + $house_cacheability = (new CacheableMetadata()) + ->setCacheContexts(['house.type']); + + $this->gardenType = 'garden'; + $this->houseOrientation = 'north'; + $orientation_cacheability = (new CacheableMetadata()) + ->setCacheContexts(['house.type', 'garden.type', 'house.orientation']); + + $this->solarType = 'solar'; + $solar_cacheability = (new CacheableMetadata()) + ->setCacheContexts(['house.type', 'garden.type', 'solar.type']); + + // This time, nothing primes the redirects so the first set will create a + // redirect at ht.house, pointing to house.type, garden.type and solar.type. + $this->setVariationCacheItem('You have a nice house with solar panels and a garden!', $solar_cacheability, $house_cacheability); + + // The second set will try to store a redirect at ht.house, pointing to + // house.type, garden.type and house.orientation. This will trigger the + // creation of a common redirect at ht.house, pointing to garden.type. + $this->setVariationCacheItem('You have a nice north-facing house with a garden!', $orientation_cacheability, $house_cacheability); + + // Now we arrive at the same scenario as the test above. We have a redirect + // chain at house.type of garden.type and finally house.orientation, but are + // trying to set solar.type at that last address. + try { + $this->setVariationCacheItem('You have a nice house with solar panels and a garden!', $solar_cacheability, $house_cacheability); + } + finally { + restore_error_handler(); + } + } + /** * Creates the sorted cache ID from cache ID parts. *