#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
.
Comment | File | Size | Author |
---|---|---|---|
#7 | interdiff.txt | 5.13 KB | Wim Leers |
#7 | menu_link_cache_tags_regression-2241291-7.patch | 5.61 KB | Wim Leers |
Comments
Comment #1
Wim LeersComment #3
Wim Leers1: menu_link_cache_tags_regression-2241291-1.patch queued for re-testing.
Comment #5
Wim Leers1: menu_link_cache_tags_regression-2241291-1.patch queued for re-testing.
Comment #7
Wim LeersMost 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.
Comment #8
pwolanin CreditAttribution: pwolanin commentedShould wait until we land this work #2227179: Step 2: Move the menu tree storage to a separate service.
Comment #9
Wim LeersBut #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.
Comment #10
msonnabaum CreditAttribution: msonnabaum commentedAgreed, this is fine.
Comment #11
pwolanin CreditAttribution: pwolanin commentedI'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.
Comment #12
catch7: menu_link_cache_tags_regression-2241291-7.patch queued for re-testing.
Comment #14
Wim Leers7: menu_link_cache_tags_regression-2241291-7.patch queued for re-testing.
Comment #15
Wim LeersBack to RTBC per #10.
Comment #16
webchickI 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.
Comment #17
catchThe 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.
Comment #19
Wim Leers#17: exactly.
Comment #20
Wim Leers