#2179083: Rendered menus (e.g. menu blocks) should set cache tags to inform the page cache introduced cache tag handling for menu links, menus and menu blocks. #2217749: Entity base class should provide standardized cache tags and built-in invalidation landed in the last few days and standardized cache tags (and their invalidation) across entity types. But in doing so, it failed to take into account the specialized/optimized situation for menu links vs. menus: as of #2179083, we never set or invalidate menu link-level cache tags, we always only use the menu-level cache tags. #2217749 accidentally changed that.

It's easy to fix: just forward calls to MenuLink::getCacheTag() to Menu::getCacheTag()! (And same for ::getListCacheTags().)

Test coverage for this kind of thing doesn't really make sense: we have test coverage that ensures correctness: MenuCacheTagsTest.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Status: Active » Needs review
FileSize
1.09 KB

Status: Needs review » Needs work

The last submitted patch, 1: menu_link_cache_tags_regression-2241291-1.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: menu_link_cache_tags_regression-2241291-1.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: menu_link_cache_tags_regression-2241291-1.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
5.61 KB
5.13 KB

Most test failures had a simple cause: a menu link must always be associated with a menu, but in the failed tests, menu links were associated with non-existing menus. By simply adding defining these menus, the problem vanishes :)

Also cleaned up/fixed the main changes of the patch.

pwolanin’s picture

Status: Needs review » Postponed
Wim Leers’s picture

Status: Postponed » Needs review

But #2227179: Step 2: Move the menu tree storage to a separate service. is itself postponed on #2227441: New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins. This is not going to be a big merge conflict at all.

Let's please not hold this RTBC-worthy patch up on an issue chain that will clearly take a lot of time before it's ready.

msonnabaum’s picture

Status: Needs review » Reviewed & tested by the community

Agreed, this is fine.

pwolanin’s picture

I'd be happier if this didn't go in since it will just create conflicts with the code in the Step 3 branch.

To be clear, step 2 is no so much postponed, as possibly skipped all together.

catch’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: menu_link_cache_tags_regression-2241291-7.patch, failed testing.

Wim Leers’s picture

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC per #10.

webchick’s picture

Assigned: Wim Leers » catch

I agree this is just a bug fix and should go in when it's ready and not be held up by major architectural refactoring.

I was going to just commit this, but I don't understand how the changes to the test actually catch this problem. The OP says tests aren't needed for this, but since I doubt we want to ever reintroduced this regression I'm not sure that makes sense.

So assigning to catch since he committed the first patch and probably knows this area of the code better to know if what's here is sufficient.

catch’s picture

Status: Reviewed & tested by the community » Fixed

The test coverage just ensures that moving links between menus still invalidates the cache entries for both menus.

Adding specific test coverage for the regression I agree we don't need - we'd be testing the absence of a menu link-specific cache tag. If we sort out automated performance testing for core then this kind of change ought to have been caught by comparing the number of database queries between runs + potentially showing the diff.

  • Commit 8e342b8 on 8.x by catch:
    Issue #2241291 by Wim Leers: Regression: menu link-specific cache tags.
    
Wim Leers’s picture

Issue tags: -sprint

#17: exactly.

Wim Leers’s picture

Assigned: catch » Wim Leers

Status: Fixed » Closed (fixed)

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