Set default render cache contexts: 'theme' + 'languages:' . LanguageInterface::TYPE_INTERFACE

+1 to the issue title. That D8 HEAD doesn't do this is a regression from D7: https://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_...

Not sure about making these two configurable though. Because in the case of 'theme', doRender() actually calls $this->theme->render(), so seems like it should force it. In the case of 'languages:interface', doRender() doesn't call t() itself, so setting that is more a convenience to allow lots of other places not to have to, but if we then proceed to make all those places not do it themselves, then that essentially is an implicit contract that the renderer will.

Having a container parameter for additional ones might make sense, but perhaps out of scope for this issue?

there are a zillion other ways you can break Drupal by changing settings, configuration or container parameters

Good point about container parameters. But I think D8 has come a long way in reducing the fragility of settings and configuration. So I think my only objection then is having the name "config" in the parameter name. How about renderer.required_cache_contexts?

Unless this is blocking other work, I think it can wait on the other issue.

1. +++ b/core/core.services.yml
   @@ -1,6 +1,8 @@
   +  renderer.config:
   +    required_cache_contexts: ['languages:language_interface', 'theme']
   

   I think we only need this when:

   drupal_is_multilingual()

   which means more than one language is configured at a given time.

   I am not sure what the D8 equivalent is for that though.

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -164,10 +174,19 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +    // Bubbleable rendering metadata that has configurable defaults.
   +    $required_cache_contexts = $this->rendererConfig['required_cache_contexts'];
   +    if (isset($elements['#cache']['contexts'])) {
   +      $elements['#cache']['contexts'] = Cache::mergeContexts($elements['#cache']['contexts'], $required_cache_contexts);
   +    }
   +    else {
   +      $elements['#cache']['contexts'] = $required_cache_contexts;
   +    }
   

   Don't we need this only when setting cache data?

   This is a _lot_ of unnecessary de-duplication we need to do.

3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -164,10 +174,19 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   -    if (isset($elements['#cache'])) {
   +    if (isset($elements['#cache']['keys'])) {
   

   Good idea :)

CNW per 2.

Does it really hurt if the context is always there? If there's only one language, it will simple always have the same value, that shouldn't hurt much?

Yes, indeed we can only set this when:

if ($is_root_call || isset($elements['#cache']['keys'])) {
}

I think adding this if around the won't hurt or do you see a case where we would need it on another level?

That would be almost as clean as the proposed solution, but avoid an even larger percentage of Cache Context merging per page rendering.

--

Besides that this is RTBC from my side.

I thought for debuggability reasons we should maybe make 'rendered' now also a required cache context.

Then I changed my mind as we never want anyone to remove that one as its kinda internal ...

Re-classifying as a bug:

This is the replacement for drupal_render_cid_parts() and hence can be considered a regression from Drupal 7.

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -164,10 +174,25 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +    if ($is_root_call || isset($elements['#cache']['keys'])) {
   +      $required_cache_contexts = $this->rendererConfig['required_cache_contexts'];
   

   Nice! :)

2. +++ b/core/modules/node/src/Tests/NodeListBuilderTest.php
   @@ -36,9 +37,9 @@ public function testCacheContexts() {
   -    $this->container->get('renderer')->render($build);
   +    $this->container->get('renderer')->renderRoot($build);
   

   I am honestly not sure, but might imagine it might be safer to use renderPlain instead?

   But we can leave it also as renderRoot for now.

This looks great now!

RTBC!

We could use some doc updates and do we need to update any of the old CRs? I am not sure.

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 2847b59 and pushed to 8.0.x. Thanks!