NavigationRenderer manually adds required cache tags

Seems to have been moved to another place in the same class by ✨ Allow modules to hook into top of content/footer sections of new core navigation Active

kristiaanvandeneynde → closed merge request !11147

Removing the required contexts reintroduces all the queries we managed to get rid off. Now I'd like to find out why we have to manually specify them.

Hmm, seems this got fixed to use the required cache contexts from the services file in ✨ Integrate with Workspaces Active already. Somehow that didn't make it into my git blame. Must have been on an old branch. Either way, would like to know why we have to add them in the first place. Going to look into this for a bit.

Okay, so the local test failed is a mystery to me. Now after a ddev restart it goes green and so do the tests here.

Which makes sense. By manually setting the following:

  public function buildNavigation(array &$page_top): void {
    $page_top['navigation'] = [
      '#cache' => [
        'keys' => ['navigation', 'navigation'],
        'max-age' => CacheBackendInterface::CACHE_PERMANENT,
      ],
      '#lazy_builder' => ['navigation.renderer:doBuildNavigation', []],
      '#create_placeholder' => TRUE,
    ];
  }

We're telling the renderer to immediately placeholder the whole thing. This means the lazy_builder never gets called until we get to either HtmlRenderer/HtmlResponseAttachmentsProcessor or other render/placeholder strategies.

When we replace placeholders, all we do is generate the following code:

$element_to_replace = [
      '#cache' => [
        'keys' => ['navigation', 'navigation'],
        'max-age' => CacheBackendInterface::CACHE_PERMANENT,
      ],
      '#lazy_builder' => ['navigation.renderer:doBuildNavigation', []],
      '#create_placeholder' => FALSE,
];

And run that through a renderRoot() call. The renderer will retrieve the saved placeholder from the cache and call $this->replacePlaceholders($elements); or it will not find the cached placeholder and, because #create_placeholder is FALSE now, will instantly call the lazy builder and actually fill it into the original element.

Either way, there is no need to manually add the required cache contexts. They are automatically added to both the original element and therefore also the reduced one we use to replace placeholders.

I do think I found a bug in the Renderer while investigating this, but want to do a bit more digging before I report on that.

I was also surprised to see them, even hardcoded before I changed that in ✨ Integrate with Workspaces Active , but didn't really look into why...

Posted a comment on the MR.

Done, think it's fine like this. Class is both internal and final.

I also think it's fine to remove it, Navigation is still marked experimental.

Have a feeling I added these originally but no idea where.

Committed/pushed to 11.x, thanks!

catch → closed merge request !11148

Automatically closed - issue fixed for 2 weeks with no activity.