Drupal
/
drupal
mirror of https://git.drupalcode.org/project/drupal.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

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 97f169b63a)
merge-requests/9437/head
catch 2024-08-30 08:52:31 +09:00
parent b65b0637bf
commit 0871d28bf1
2 changed files with 231 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

45
core/lib/Drupal/Core/Cache/VariationCache.php
View File

@ -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;
}
}

189
core/tests/Drupal/Tests/Core/Cache/VariationCacheTest.php
View File

@ -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.
*