Possible regression due to introduction of variation cache

To summarize the slack discussion we just had. that those cache contexts are there on the page level isn't really relevant, they must always be there on each element that's cached separately.

The code is wrong as it replaces existing cacheability info from $build, which could cause very weird caching issues if the node is rendered in a different theme or isn't invalidated properly for cache tags and contexts that get lost.

And, if those two are the only things it adds, it can be removed entirely because both display and the node are already considered cache dependencies.

So the change is that subtle, hard to track down cache bugs are replaced with an exception.

I think we should either merge any existing cache metadata on the renderable, like the patch in #5,

or throw an exception if the CacheableMetadata removes any existing cache metadata when applied to a renderable (diffing the new resulting #cache with the old existing ##cache)

I agree that applyTo() is a gotcha. The merge or the Exception sound good to me.

It sounds like we can reopen this issue as a feature request? The patch in #5 contains the suggested merge. I think the merge would be the most logical solution, because the name applyTo doesn't suggest that it overwrites the existing data.

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

timohuisman → changed the visibility of the branch 3412444-make-cacheablemetadataapplyto-merge to hidden.

Can the issue summary be completed please.

Have not reviewed but ran test-only feature which confirms test coverage.

Thanks, patch #5 works for me.

I've updated the issue summary to attempt to better describe this feature request.

// @todo determine if we want to use Cache::mergeContexts() here.
// Note that we cannot use Cache::mergeContexts() here,

This comment doesn't make much sense to me. Should it be updated ?

Comment still doesn't really make sense. Think at minimum the todo part should be removed. Leaving in review for other thoughts

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

This code change helped me install the latest core security update 11.1.3. Running 11.1.1 with a contrib module group_domain worked fine, but either 11.1.2 or the security patch release 11.1.3 gave the "The complete set of cache contexts for a variation cache item must contain all of the initial cache contexts" error for that module.

I'm not saying if this is the correct place or the correct way to fix the issue properly, but it does help as a workaround to be able to do the security update.

I had a project that I upgraded from D10.4.3 to D11.1.3

After update instead of seeing the website, I was seeing this error:

The website encountered an unexpected error. Try again later.

LogicException: The complete set of cache contexts for a variation cache item must contain all of the initial cache contexts, missing: url.site. in Drupal\Core\Cache\VariationCache->set() (line 47 of core/lib/Drupal/Core/Cache/VariationCache.php).
Drupal\Core\Render\RenderCache->set() (Line: 120)
Drupal\Core\Render\PlaceholderingRenderCache->set() (Line: 539)
Drupal\Core\Render\Renderer->doRender() (Line: 459)
Drupal\Core\Render\Renderer->doRender() (Line: 203)
Drupal\Core\Render\Renderer->render() (Line: 484)
Drupal\Core\Template\TwigExtension->escapeFilter() (Line: 88)
__TwigTemplate_e713a3461e456cb3d4ba7da0e93bf6aa->doDisplay() (Line: 388)
Twig\Template->yield() (Line: 344)
Twig\Template->display() (Line: 359)
Twig\Template->render() (Line: 51)
Twig\TemplateWrapper->render() (Line: 33)
twig_render_template() (Line: 348)
Drupal\Core\Theme\ThemeManager->render() (Line: 446)
Drupal\Core\Render\Renderer->doRender() (Line: 203)
Drupal\Core\Render\Renderer->render() (Line: 158)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 593)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 153)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse() (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray() (Line: 246)
Symfony\Component\EventDispatcher\EventDispatcher::Symfony\Component\EventDispatcher\{closure}() (Line: 206)
Symfony\Component\EventDispatcher\EventDispatcher->callListeners() (Line: 56)
Symfony\Component\EventDispatcher\EventDispatcher->dispatch() (Line: 188)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 53)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 116)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 90)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 709)
Drupal\Core\DrupalKernel->handle() (Line: 19)

I have no idea what contrib or custom module is responsible for invoking that error. I do not use workspaces. But the MR fixed the problem and the project is now working again after the D11 update.

Been a few weeks and the comment still doesn't make much sense. Todo check if we can use Cache::mergeContexts(), then note we cannot use Cache::mergeContexts() ??