Problem/Motivation

Issue split from #3426576: Add tests for UserNavigationBlock.

The navigation bar implementation has a cache context per user in the global section that can not be placeholdered. This causes the Dynamic page cache to stop working, since the cardinality is too high.

Proposed resolution

Get rid of the context per user in navigation bar.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork navigation-3436937

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

plopesc created an issue. See original summary.

plopesc’s picture

Title: Navigation Bar is uncacheable » Navigation Bar implementation makes all pages uncacheable

plopesc’s picture

As part of the MR creation, commented the line suggested by @m4olivei in #3426576: Add tests for UserNavigationBlock.

It lead to some broken tests.

Debugged it a it and found the issue in NavigationBlockCacheTest::testCachePermissions

Cache tags before the change (Test pass):
config:block_list config:navigation.navigation_block.administration_menu config:navigation.navigation_block.content_menu config:navigation.navigation_block.oxaw6wfu config:navigation.navigation_block.user config:navigation.settings config:system.menu.admin http_response navigation_block_view rendered user:2 user_view
Cache tags after the change (Test fails):
config:block_list config:navigation.navigation_block.administration_menu config:navigation.navigation_block.content_menu config:navigation.navigation_block.user config:navigation.settings config:system.menu.admin http_response navigation_block_view rendered user:2 user_view

I'm missing the cache tag related to the test navigation block added during setup().

My feeling is that reason behind failing test is that navigation bar cache tags are not properly invalidated or refreshed when a new navigation block is added.

More digging in this one will be necessary. I'll try to continue with this next if anybody finds the root cause before.

plopesc’s picture

Status: Active » Needs review

MR created that replaces the problematic user cache context with user.permissions to make the navigation bar cacheable.

As a secondary effect, we found that cache tags were not being implemented properly because navigation bar was not including automatically new navigation blocks added.

Root cause seemed to be that navigation block list cache tag (config:navigation_block_list) was not included in the bar itself.

Once the cache tag is added, tests are back to green and 'X-Drupal-Dynamic-Cache' header is not 'UNCACHEABLE' anymore.

I believe this is ready for review now!

m4olivei’s picture

Status: Needs review » Reviewed & tested by the community

Great work!

That all makes logical sense to me. Ran through some tests on Tugboat, and it appears to be working well as well.

  • m4olivei committed cc02ac59 on 1.x authored by plopesc
    Issue #3436937 by plopesc, m4olivei: Navigation Bar implementation makes...
m4olivei’s picture

Status: Reviewed & tested by the community » Fixed

Thanks again @plopesc.

Merged to 1.x

Status: Fixed » Closed (fixed)

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