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!