Add render caching for the navigation render array

A couple comments on the MR.

Looks like all my comments were addressed. One test failed, but it seems a random know failure that's unrelated, and the test likely just needs re-running.

One remaining question is whether there needs to be test coverage that the navigation is being cached correctly?

I think I can probably write a performance test with navigation enabled that will detect the extra 10-20 database queries due to lack of caching, or the absence of them with this MR applied. Although it will end up being identical to an admin performance test against the standard profile once navigation replaces toolbar (not sure we have an admin test yet though, might just be regular authenticated user so far).

Sounds good. Are you planning to write that test in this MR or do it in a separate issue?

Thank you for raising the issue.

Code is simple and it can make a difference in the long term.

I'm not marking it as RTBC yet, waiting for the decision about test implementation.

Only moving to NW for the mentioned performance test coverage.

Very neat!

OK test coverage:

Had to think about where to put it, in the end I decided it should be in Navigation module because it's experimental, but that we should move it to StandardPerformanceTest when Navigation is stable (and we don't need to uninstall toolbar etc.).

First of all I wrote test coverage for the status quo in HEAD - so a 'tests-only' branch here except it should pass.

https://git.drupalcode.org/project/drupal/-/merge_requests/10546

Then I cherry-picked that test coverage into the branch and pushed, giving a test failure here: https://git.drupalcode.org/project/drupal/-/jobs/3689239

Then I updated the test coverage to match the state with the fix.

You can see 17 database queries and 31 cache gets and one cache tag checksum fetch less https://git.drupalcode.org/project/drupal/-/merge_requests/10531/diffs?c...

As you can see from the test coverage, the toolbar does get cached in dynamic_page_cache itself, but only at the level of the whole page, which means it's per URL/route, so this saves rebuilding it every time you browse to a different page.

Also apart from the menu tree/access queries and rendering, it's also saving a path alias lookup for the help link and maybe some other logic as well so I think this will probably make sense to keep even with 📌 Implement a caching strategy for the menu links Active .

I did wonder if we want to put the entire navigation block in a lazy builder so the bigpipe can placeholder it. This would increase the cache hit rate for the assets (because they'll be served in their own aggregate) and also reduce the cache size in the dynamic page cache (because it would only be a placeholder in there), but needs its own issue because it will also mean it's loaded by BigPipe which could introduce jank. Will open that separately since it's not strictly related to this.

Code looks good and test coverage ensures that this is going to make a difference.

I think this can be marked as RTBC.

Thank you @catch!

Oh I also manually checked what the cache_render cid looks like in the database, and it's this:

navigation:navigation:[languages:language_interface]=en:[theme]=stark:[user.permissions]=is-admin 

So that confirms nothing is creeping in that is per-user or per-route or anything like this. The test coverage does not explicitly test that it's not cached per-route because we hit the same page each time. We avoid per-user caching because the user menu and shortcuts links are already placeholdered within the navigation (otherwise dynamic page cache would be broken with or without this MR).

After typing out the above sentence I was about to say not sure if we need explicit test coverage for this, because pretty sure route-specific stuff is supposed to go in the top bar and user-specific stuff would probably break dynamic_page_cache entirely. But then realised it would take longer to type all that than adding the test coverage. Since that's just a one liner of extra test coverage, leaving RTBC but a +1 would be great.

RTBC + 1. The the assert in the latest commit confirms there user, route, and query params cache contexts are not part of the cache key.

Opened 📌 Render the navigation toolbar in a placeholder Active for the (possible) next step.

catch → changed the visibility of the branch 3493406-test-coverage to hidden.

You can see 17 database queries and 31 cache gets and one cache tag checksum fetch less https://git.drupalcode.org/project/drupal/-/merge_requests/10531/diffs?c...

🔥 love it

Committed to 11.x - awesome work

larowlan → closed merge request !10531

I think we could probably backport this to 11.1.x given that navigation is experimental and there's no bc implications here.

My tests fail at latest 10154d1a commit 📌 Move helpers in node_access_test.module and delete it Needs work .
Change $this->assertSame(61, $performance_data->getCacheGetCount()); fix the error.
I am not able to find any updates about this. Is it something on my side?

@nikolay shapovalov it's possible that's a random failure but this is the first time I've seen it reported anywhere, just re-ran the test over there. If it happens again, we should make the assertion between 60 and 61 then open a follow-up to try to track down what's changing.

Something similar happened consistently to me when rebased ✨ Allow modules to hook into top of content/footer sections of new core navigation Active . But assumed that extra cache get would be related to the extra hook invocation added in the MR and bumped the count in the test .

Just to mention it here, maybe unrelated to the reason mentioned above.

I didn't have enough time to explorer, but I get same error on my localhost. I believe this isn't random failure. I think I will increase count, but I want to find the reason. Thanks for your suggestion.

Issue come from 9177c489 and issue 🐛 Navigation Top Bar hides entity local tasks even if the user has no access to the bar Active . You can check pipeline. We can just change number of assertion to 61.

Test fixed at 🐛 Navigation Top Bar hides entity local tasks even if the user has no access to the bar Active commit 738036cc.

Went ahead and backported this to 11.1.x, thanks!