If you install this module with the Mercury theme and visit different 404 pages you will get a dynamic page cache entry page per 404.
Not sure. Core's path based breadcrumb builder uses
// Add the url.path.parent cache context. This code ignores the last path
// part so the result only depends on the path parents.
$breadcrumb->addCacheContexts(['url.path.parent', 'url.path.is_front']);
But given in default configuration to add the current page title to breadcrumb I don't think that is would work.
Also you can't add a reponse status cache to breadcromb block because that also adds a url.path to cache context. There might be a core issue here for how url.path works with 404s and 403s.
Show commandsStart 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:
changes, plain diff MR !228changes, plain diff MR !189 / changes, plain diff MR !188changes, plain diff MR !186changes, plain diff MR !184compare changes, plain diff MR !176 / changes, plain diff MR !175
Comments
Comment #2
alexpottHere's a decent blog post on the subject - https://bkosborne.com/blog/how-drupals-dynamic-page-cache-delivers-extre...
Comment #3
catchOpened #3516173: Block status code visibility condition should use a status code cache context to core, but just that issue would require sites to configure the breadcrumb block not to show on 404 pages.
However once the cache context is there, it might be possible to check for a 404/403 first, early return there while adding the status code cache context, then only add the url cache context if it gets past that. Or set the breadcrumb builder not to apply on 404 pages at all.
Comment #4
greg boggsI had noticed this problem on my own Drupal website, and didn't realize it was easy breadcrumb module doing it.
This cache improvement sounds fantastic. Hiding breadcrumbs on 404 also sounds good. If y'all want to just look over the cache code and make sure it's correct that would be ideal. The cache code is grouped in the module here:
https://git.drupalcode.org/project/easy_breadcrumb/-/blob/2.x/src/EasyBr...
I think this is an unrelated cache issue:
https://www.drupal.org/project/easy_breadcrumb/issues/3500483
Comment #5
catchSo #3516173: Block status code visibility condition should use a status code cache context will make the performance issue fixable with the status code block condition.
But I think it would be good to fix this directly in easy breadcrumb too - it can look for the exception on the request (see code in that issue), and if it's there, early return and only add the status code cache context instead of the URL one - probably best to do in the ::applies() method? Not familiar with the code so take with a pinch of salt but should be enough to get started. Only problem is that cache context will be in 11.2, maybe 10.5, but not a patch release, so either needs to wait or maybe copy it for a while.
Comment #6
greg boggsMakes sense. I'll see what I can do. That code is pretty messy at this point after many years of small public contributions.
Comment #7
yonailoHello,
Any movement on this ? We are experiencing enormous "cache_dynamic_page" table sizes and I am wondering if it could be related to this issue, we have a lot of bots trying 404 pages at random.
Comment #8
greg boggsNot yet! As a temporary fix, you can exclude the breadcrumb block from 404 pages.
Comment #9
yonailoI’ve been doing some debugging with my client’s site and I don’t understand what is happening.
It seems that the dynamic cache grows because the breadcrumb block has a visibility condition with path != front_page
This makes all 404 pages to create a dynamic cache entry that includes the path.url context.
If I remove this visibility condition, the dynamic cache does not grow, which I don’t understand because I see that the breadcrumb builder is adding path.url …
Comment #11
loopduplicate commentedComment #13
loopduplicate commentedComment #14
loopduplicate commentedWell, as I expected before I went to sleep last night, the code I wrote was going to crash d10. Tested this morning. I was right. So, more work is needed for sure.
Comment #16
loopduplicate commentedComment #17
loopduplicate commentedComment #19
loopduplicate commentedOK, tested on d10 and d11. Ready for review again with a new MR.
Comment #20
loopduplicate commentedComment #21
catch@yonailo you might be running into #3589822: Try to reduce cache context granularity from path conditions
Comment #26
loopduplicate commentedI'm working on writing a test for this now.
Comment #27
loopduplicate commentedI've added a test. The problem for me is that this bug is not reproducible except when using Drupal CMS. So, with just this module enabled, the test passes even before the chanages I made while working on this issue. So, I'm trying to figure out exactly what combination of modules and config can make this happen so I can get a failing test here.
Comment #28
loopduplicate commentedThis bug is not reproducible when choosing the Blank site template.
It is reproducible with the Starter and the Mercury Demo.
Comment #29
loopduplicate commentedAs far as I can tell, it's the Mercury theme combined with Easy Breadcrumb that is causing the cache to grow.
Comment #32
loopduplicate commentedOK! Good progress.
I've created a test that installs the Mercury theme to reproduce this bug!
Then, I applied the code fix I wrote earlier in this issue.
It's all in the new MR, and pretty easy to understand.
Excited to see what people think and if we can get this reviewed.
Peace.
Comment #33
loopduplicate commentedComment #35
loopduplicate commentedWell, I rerolled this after #3500483 was committed. Since we now set cache contexts in the applies() method, the code from MR!188 no longer works. I'll work on a solution. The good news is that the test I wrote failed and that's how I knew the code didn't work anymore. +1 for TDD, eh?
Comment #38
loopduplicate commentedOk, tests pass again. The change was to remove the 'url' cache context in the applies() method. When all versions of core that are supported by this module support adding cacheable metadata in the applies() method, we can refactor.
Comment #39
loopduplicate commentedI'm working on this now to get it working with the current 2.x version again. It would have been nice to get this in the release yesterday, but it will have to wait until the next one now.
Comment #43
loopduplicate commentedOK, all set and up to date with the latest changes. Ready for review again.
Comment #44
greg boggsTest results:
- Reverted just the fix code (kept MR's new regression test EasyBreadcrumbDynamicPageCacheTest) - test failed, table grew 4→5 rows on a second distinct 404, reproducing the bug exactly as described.
- Restored the MR's fix - same test passed, row count stayed flat across multiple 404s.
MR 228 does fix what issue 3516169 claims it fixes.
Manual testing revealed nothing unusual in caching or the breadcrumbs
---
Disclosure: AI was used in assisting to test this issue.
Comment #45
greg boggsComment #46
loopduplicate commentedGreat Greg! I'll work on getting this merged in now.
Comment #48
loopduplicate commentedThanks! @alexpott, @catch, @greg boggs, @yonailo