Problem/Motivation
From SystemBreadcrumbBlock
:
/**
* {@inheritdoc}
*
* @todo Make cacheable as part of https://drupal.org/node/1805054
*/
public function getCacheMaxAge() {
return 0;
}
Proposed resolution
Once #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees lands, doing this becomes possible.
But, it doesn't make sense to fix it as part of that issue, because it requires analyzing and potentially updating all BreadcrumbBuilderInterface
implementations, of which there are about half a dozen. That interface may need to be changed as well.
Therefore, it should be done in a separate issue.
Update BreadcrumbBuilderInterface
so it allows cacheability metadata to be returned for the built breadcrumbs, and analyze all existing BreadcrumbBuilderInterface
implementations to have the correct cacheability.
Remaining tasks
None.
User interface changes
None.
API changes
\Drupal\Core\Breadcrumb\BreadcrumbBuilderInterface::build()
needs to return a Breadcrumb object (or NULL?) that can encapsulate what it is returning in HEAD plus cacheability metadata- Added a url.path cache context
- hook_breadcrumb_alter takes $breadcrumb object as a first parameter instead of an array.
Beta phase evaluation
Issue category | Task, because nothing is broken. |
---|---|
Issue priority | Major because cacheability of the breadcrumb block is important for performance. Currently it would make pages completely uncacheable, which is problematic for performance. |
Prioritized changes | the main goal of this issue is performance and cacheability. |
Disruption | @todo Fill out |
Comment | File | Size | Author |
---|---|---|---|
#183 | make_breadcrumb_block-2483183-183.patch | 55.65 KB | Wim Leers |
#176 | make_breadcrumb_block-2483183-176.patch | 54.42 KB | Wim Leers |
#12 | interdiff.txt | 8.08 KB | lauriii |
#12 | make_breadcrumb_block-2483183-12.patch | 12.01 KB | lauriii |
#15 | make_breadcrumb_block-2483183-15.patch | 12.29 KB | moshe weitzman |
Comments
Comment #1
Wim LeersI think this may be a duplicate of #2232609: Cacheable breadcrumbs block, and fix breadcrumb builders.
Comment #2
Wim Leers#1805054: Cache localized, access filtered, URL resolved, and rendered menu trees landed, so this is now finally unblocked!
Comment #3
Wim LeersThis issue is of very high importance, because it is one of the last things present on most pages that has
max-age = 0
, therefore causing SmartCache (#2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!)) to not cache the entire page.Fixing this would go a long way in making SmartCache (and caching in general) more effective.
Comment #4
Wim LeersHere's a start that outlines the necessary changes:
\Drupal\Core\Breadcrumb\BreadcrumbBuilderInterface::build()
needs to return a value object that can encapsulate what it is returning in HEAD plus cacheability metadataBreadcrumbBuilderInterface
must return this new kind of objectIn this patch, I've converted two implementations of that interface to show the thought process. There are four more such implementations that need to be updated.
Comment #6
dawehnerOdd, I would have kinda expected that this kind of metadata should be part of each individual link.
Comment #7
Wim LeersWhy?
We care about the logic used to determine the breadcrumb links. That logic depends on *some* request context.
(Note that individual links, when rendered, will also bubble cacheability metadata, when their URLs are generated. But that's a different level.)
Comment #8
dawehnerWell, because the cacheablity metadata is coming directly from the fact that certain links/URLs appear, not that you have a chain of them. Cache contexts IMHO have no meaning on lists of URLs
Comment #9
Wim LeersBut in case of the path-based breadcrumb builder, each link would specify the
url
cache context. In case of the term-based one, each would specifyroute
and the cache tags of the term. The pattern there: all links have the same cacheability metadata. Because what matters is the context used by the logic to get that list of links.Can you give a concrete example that supports #8?
Comment #12
lauriiiComment #14
dawehnerWhat about: Stores a list of links, along with associated cacheablity metadata.
Maybe its me, but I would have expected a single add method.
Comment #15
moshe weitzman CreditAttribution: moshe weitzman at Acquia commentedLets see what else fails.
Comment #17
moshe weitzman CreditAttribution: moshe weitzman at Acquia commentedFix comment and book breadcrumbs.
Comment #21
lauriiiAdded missing files for the patch
Comment #22
tim.plunkettShould start with a verb ending in 's', not "Used"
Extra space before ;
I think it would be nice to provide an
addLink(Link $link)
methodComment #24
moshe weitzman CreditAttribution: moshe weitzman at Acquia commentedI'm having trouble reproducing some of these test fails. Hopefully someone else can push this along.
Comment #25
lauriiiThis should fix the fatal
Comment #27
lauriiiThat didn't fly because it sucked. Maybe this is better :P
Comment #30
lauriiiThis should be fixing some more of the tests.
Comment #32
lauriiiInstead of setCacheContexts it would be nice to use addCacheContexts but it breaks the unit tests since it needs container to be built. I still didn't manage to figure out how to mock the addCacheContext.
Comment #33
tim.plunkettI don't know if it's worth changing the signature of hook_system_breadcrumb_alter() again, but I think we should build the Breadcrumb and pass that through. How else will the alter have access to setting it's cache contexts?
missing space
Extra
Comment #36
lauriiiDidn't pay attention on #33.1 yet
Comment #38
lauriiiThis should fix rest of the broken tests :) I will take a look to fix the cache contexts later today.
Comment #45
bfr CreditAttribution: bfr at Druid commentedI went through all the breadcrumbs, and seems like you actually fixed all of them so nothing left to do here.
Comment #46
lauriiiComment #47
bfr CreditAttribution: bfr at Druid commentedActually found something afterall, will post patch in a minute.
Comment #48
bfr CreditAttribution: bfr at Druid commentedThere you go.
Comment #49
bfr CreditAttribution: bfr at Druid commentedYeah, something was missing ;)
Comment #50
bfr CreditAttribution: bfr at Druid commentedComment #52
lauriiiComment #53
dawehnerI like the patch in general. Having a more specific breadcrumb object makes sense for me.
Comment #54
tim.plunkett#33.1 was never addressed.
Comment #55
jibranVery nice work @lauriii. Thanks @dawehner for setting it to RTBC IMO it's also ready just minor nits and some documentation stuff is pending.
Can be re-written so we can adjust it in 80 chars.
We can add unittest for this class.
Comment #56
Wim LeersWow, great progress here! :)
My review:
Nit: "append" is more accurate than "add".
#33.1++
With #33.1, this also can go away.
This change can be reverted.
The assignment here is unnecessary.
Are we sure this cannot be
route
instead?If we are sure, then we need a follow-up issue, to add a
url.path
cache context, because thePathBasedBreadcrumbBuilder
does not care about the query string, yet by specifying that the breadcrumb varies byurl
, a consequence is that a cached breadcrumb block cannot be reused across e.g./node?page=1
and/node?page=2
. i.e. it is bad for cacheability.Comment #57
lauriiiFixed #55.1, #33.1, #56.1-3, 5
#56.4: Are you sure? I was trying to use the 'route.book_navigation' cache context and it throwed a fatal error unless that change was made since the container didn't exits in the BookNavigationCacheContext class.
#56.5: I think it cannot be route since one route could have multiple paths and then it could have also have multiple breadcrumbs.
Comment #58
Wim Leers#56.4: Hrm, doh, you're right.
#56.6 (you said .5, but you actually meant .6): Right, so then we do want a
url.path
cache context. I think we should actually add it here, since it's the use case most in need of this. Attached is a suggested interdiff; with that in place, you'll be able to switch fromurl
tourl.path
.Comment #59
jibranSetting it to needs review helps sometime. :P
Comment #60
lauriiiAnd sometimes it makes it worse.. :P
Comment #62
lauriiiComment #64
Wim LeersWoohoo, only 9 failures :) Almost there!
Comment #65
martin107 CreditAttribution: martin107 commentedNeeded a reroll.. nothing but auto merging and a little 3-way. Lucky patch!
Comment #69
martin107 CreditAttribution: martin107 commentedlooking at the foreach loop in
BreadcrumbManager:build()
if there are no sorted builders then the inner code block which defines the variable $breadcrumbs is not executed
So the return value is not defined...
BreadcrumbManager:testBuildWithoutBuilder() codifies the fact that without any builder the return value is array()
So the fix is to insert a return early clause in BreadcrumbManager:build()
All the unit tests now pass... and visually, sanity is returned to the front page.
I'm not sure this will resolve all remaining issues .. but testbot should now give a reduced set of errors.
I take the line my reroll broke it so, this is mine to fix up.... I will unassign when green.
Comment #71
martin107 CreditAttribution: martin107 commentedLooks like testbot is overworked - 6 hours in the queue and then failure - there must be a sprint on.
In any event, I have played with this patch a found a page that highlights a failure.
just visit /admin/modules and things go sideways...
Instead of retesting immediately, I will fix 1 minor thing.
1) PathCacheContext is a new class, and the preferred pattern is to make use of StringTranslationTrait.
BUT more importantly I think I have made a mistake.....
Looking at the patch and BreadcrumbBuilderInterface
I see that text about returning an empty array to suppress all breadcrumbs has been removed.
Is that correctly?, if so my change in #69 is a misstep and it is the test that should be refactored.
Can someone comment?
Comment #72
martin107 CreditAttribution: martin107 commentedLets see what testbot thinks now.
Comment #74
martin107 CreditAttribution: martin107 commentedMore functionality restored.
/admin/modules now renders correctly.
Hmm, looking for a cause, well I can't justify anything working before.... must have had good but stale data in a cache.
Comment #76
Wim LeersPHP Fatal error: Call to undefined method Mock_CacheContextsManager_b56f5a69::validateTokens() in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Cache/Cache.php on line 40
Some of the mocking here is broken.
Comment #77
martin107 CreditAttribution: martin107 commented@Win Leers Thank you,
I was looking at another failing test, which needs more work, and found your comment a healthy diversion :)
So, also, I know in advance that this will only be a partial fix.
When I rerolled I failed to take into account #2483781: Move cache contexts classes from \Drupal\Core\Cache to \Drupal\Core\Cache\Context
Comment #78
martin107 CreditAttribution: martin107 commentedComment #80
martin107 CreditAttribution: martin107 commented#77 as predicted was only a partial fix.
I will point out the reason for the failure in BreadcrumbManagerTest::testBuildWithSingleBuilder(), but unfortunately I may not be able to correct it now until the weekend. :(
The reason for failure ... the $links parameter is no longer simple to recreate - it has become a more complex array of breadcrumbs and links.
To me this test is too tightly coupled to the implementation and so the whole thing needs a careful rethink.
Anyway I will leave this assigned to me, but please feel free to jump in ... I will have more free time on Sunday.
Comment #81
effulgentsia CreditAttribution: effulgentsia at Acquia commentedTagging as an API change. Hopefully, this will land before RC, but if not, we'll need to figure out what else can be done to make this fixable after that.
Comment #82
martin107 CreditAttribution: martin107 commentedAs the number of criticals has jumped this week, so has the projected release date.
https://drupalreleasedate.com/ has jumped from August to late December
@effulgentsia or were you thinking of a more pressing deadline I wasn't aware of?
Comment #83
effulgentsia CreditAttribution: effulgentsia at Acquia commentedhttps://drupalreleasedate.com/ is not a very accurate predictor, because futures are for creating, not predicting. If you look at https://drupalreleasedate.com/chart/samples, you'll see that our critical burndown doesn't follow predictable straight lines. We've had multiple months in the past where we've burned down >20 criticals, and other months where the number's gone up. So we can be anywhere from a month to ??? from an RC.
Comment #84
martin107 CreditAttribution: martin107 commented@effulgentsia
I have been watching that graphs bumpy ride, for about a year, so yeah it is a most unreliable source of information.....
Oh well the next DrupalCon is a good time to knock a few lumps out of the thing :)
two separate fixes.
1)
From #80
Overnight I remembered a trick - with closures - to interrogate the input parameters of a mocked method to check aspects of an input Object. ( In this case, an instance our shiny new breadcrumb object )
2)
From #71
To answer my own question instead of an empty array being returned early to suppress things
I am returning a new, empty breadcrumb - that seems more appropriate
Comment #86
martin107 CreditAttribution: martin107 commentedThose aren't the same 9 errors from before my reroll, ( see #62 )
So its still down to me to to fix this up, sorry it is taking so many attempts.
Comment #87
Wim LeersNo worries — thank you for doing this! :)
(Also: better to have many small patches that fail, then we all see your progress — rather than you working in a dark corner for a week and none of us knowing about it :))
Comment #88
martin107 CreditAttribution: martin107 commentedThanks, I will try and keep this issue under 300 comments :-)
That being said there is a disparity between my local run of PageCacheTagsIntegrationTest and testbots version...
so please excuse the slow down while I try and riddle the problem out by inspection of testbots ouptut.
So the failure at Drupal\page_cache\Tests\PageCacheTagsIntegrationTest->testPageCacheTags() line 113
in natural language the code compares the cache contexts for a page, and does like the answer.
The difference in arrays is
0 => 'languages:language_interface',
1 => 'route.menu_active_trails:account',
2 => 'route.menu_active_trails:footer',
3 => 'route.menu_active_trails:main',
4 => 'route.menu_active_trails:tools',
5 => 'theme',
6 => 'timezone',
7 => 'url.path',
8 => 'user.permissions',
9 => 'user.roles:authenticated',
0 => 'languages:language_interface',
1 => 'route.menu_active_trails:account',
2 => 'route.menu_active_trails:footer',
3 => 'route.menu_active_trails:main',
4 => 'route.menu_active_trails:tools',
5 => 'theme',
6 => 'timezone',
7 => 'url',
8 => 'user.permissions',
9 => 'user.roles:authenticated'
So the pressing question is why is the code in the patch dropping the 'url.path' context and inserting a stray 'url' context value.
is a string manipulation fudging the handling of '.' Hmm I am sure I will figure it out over the weekend!
Comment #89
Fabianx CreditAttribution: Fabianx as a volunteer commentedYes, we have a cache context hierarchy.
So given ['url.path'. 'url'] it gets simplified to just [ 'url' ].
Comment #90
martin107 CreditAttribution: martin107 commented@Fabianx thanks
So my analysis in #88 was wrong headed.
Especially given this helpful primer [#2451661] courtesy of Wim
So to trace the timeline
The update to the golden cache context reference enters in #38, where tests pass for a time.
Its looks like changes #57, as a result of the review in #56 are the start of the test failures.
Anyway I will get to this tomorrow.
Comment #91
Wim LeersThanks for working on this, I'll review your next patch when it's posted :)
Comment #92
martin107 CreditAttribution: martin107 commentedMy special talent for over complicating things is not normally so prominent.
Given this from #58
The fix seems natural.
[One failure remains.]
Comment #94
martin107 CreditAttribution: martin107 commentedLooking at the verbose output from running the test locally
Breadcrumb Home » Administration » Structure » Menus » Tools
is expected
Breadcrumb Home » Administration » Structure » Menus
is found
So the question is why is tools being dropped?
Comment #95
martin107 CreditAttribution: martin107 commentedI'm out of time today ....
My change, which will not fix the problem, is that we are using a definite class when we should be using BreadcrumbInterface
I can confidently point a finger at the point in the code where the unexpected happens
The failing code, from core/modules/menu_ui/menu_ui.module
In my debugger I can see $menu->label() returns our missing 'Title' text.
after the breadcrumb->addLink() the breadcrumb is update ok within the function - everything looks fine but the expected breadcrumb is not passed forward.
I am unassigning as my free time next week is much more unpredictable, that is to say I will actively lurk on this, rather than drive it home.
Comment #97
lauriiiBreadcrumb manager needs to return $build instead of $breadcrumb because otherwise hook_system_breadcrumb_alter doesn't have any affect.
Comment #98
Wim LeersThanks for the laughs! :D
@martin107: thanks again for pushing it forward!
@lauriii: thanks for fixing the final failure so simply! :P
s/array/ordered list/
s/breadcrumb/ordered list of breadcrumb links/
Why
$build
, why not just$breadcrumb
?The comment seems wrong now? I think the comment can simply be deleted.
It's not an interface, so s/BreadcrumbInterface/Breadcrumb/
s/the breadcrumb manager build method/BreadcrumbBuilderInterface::build()/
s/url/url.path/
Let's instead do:
That makes sure that cache max-age & tags are also set on the render array.
Comment #101
martin107 CreditAttribution: martin107 commentedBefore sorting out #98 - this needs a reroll.
The file diff between #97 and #101 shows only changes in code surrounding the actual patch changes and they related to insertion of slashes ( / ) in paths.
A site install worked and I have visited a few pages with long breadcrumb trails - so that is my quick and dirty manual testing ... as usual testbot will give the complete answer.
Comment #102
martin107 CreditAttribution: martin107 commentedFrom, from review in #98
1) Fixed.
2) Fixed.
3)
$breadcumb is already used, in that method, for something else.
4) Yep, I agree, I have removed it.
5) Oh thanks, so I am backing out a incorrect change I made recently ( see #95 )
6) Fixed.
7) Fixed.
8) That is a really good correction :)
Comment #104
martin107 CreditAttribution: martin107 commentedhiding interdiff.patch and uploading interdiff.txt
Comment #105
Wim LeersAlmost there
Nit: 80 cols.
This still needs to be changed to
BreadcrumbInterface
.These can be on the same line, and the curly brace needs to be moved two spaces back.
So:
Broken indentation
Same here.
And here.
Comment #106
martin107 CreditAttribution: martin107 commented'cuse me while I reroll. No conflicts just auto-merging.
Comment #107
martin107 CreditAttribution: martin107 commentedAdded beta evaluation template
This is a change going in very late, so needs justification.
Comment #109
Fabianx CreditAttribution: Fabianx as a volunteer commented#107: Thanks, I improved it with a little different direction.
Not the changes in the patch itself need justification, but the overall issue.
Comment #110
martin107 CreditAttribution: martin107 commentedWhen I test locally, I see only 4 failures, not 8 - lots of cache related patches have gone in since last test - so I just want to see a re-run.
Comment #113
Wim Leers@martin107: try running the tests with
run-tests.sh
. The web test runner in some rare cases can mask some failures.Comment #114
martin107 CreditAttribution: martin107 commentedAh thats interesting, I count 5 commits since last test - and now both browser testing and command line testing have
Drupal\page_cache\Tests\PageCacheTagsIntegrationTest passing.
the only significant changed are the commit, revert and then correct changes associated with #2505669: Inject render service into LinkGenerator instead of calling drupal_render
So I am about to re-test.
Comment #117
martin107 CreditAttribution: martin107 commentedThis will not fix the errors.
We should adhere to this new convention
things that implement CacheContextInterface need to document cache context id.
#2443323: New convention: CacheContextInterface implementations should mention their ID in their class-level docblock
Comment #118
Wim LeersThanks for updating those docs, wonderful!
I think you intentionally kept it at NW, so I'm not changing it to NR.
Comment #120
Wim LeersLOLWUT
Wim--
Comment #121
lauriiiWe have quite a good test coverage for this in \Drupal\Core\Breadcrumb\BreadcrumbManager
Comment #122
lauriiiComment #123
Wim LeersGreat work all :)
Comment #124
Fabianx CreditAttribution: Fabianx as a volunteer commentedOnce this is in, this can be closed as duplicate:
#2521978: core/modules/system/src/Plugin/Condition/RequestPath should use the 'url.path' cache context
Comment #125
Fabianx CreditAttribution: Fabianx as a volunteer commentedComment #126
Fabianx CreditAttribution: Fabianx as a volunteer commentedGreat patch, little things need work:
nit - this should by in:
\Drupal\Core\Cache\Context\PathCacheContext
It is very confusing to change that around.
It should be changed back.
Given the Interface was changed to require a breadcrumb value, I think the instanceof check is superfluous, maybe NULL is allowed to be returned? If so the interface should allow that.
The API change needs to be documented and we need a change record.
Nice!
uber nit - I don't think it can only cache generated urls.
API change
Love the new Interface!
This is a great example how value objects / transport objects can make an API cleaner.
I think this needs to add the cacheableMetadata of the $entity?
That can be follow-up though - issue is big as is already.
I think this needs to add the cache tag of the comment as cacheable metadata?
That can be follow-up though - issue is big as is already.
I think this needs to add cacheable metadata of the vocabulary?
That can be follow-up though - issue is big as is already.
I think this needs to add cacheable metadata of the forum entity?
That can be follow-up though - issue is big as is already.
I think this needs to add cacheable metadata of the term?
That can be follow-up though - issue is big as is already.
I _think_ all the missing cache tags should actually be follow-up as this issue is mainly about the cache contexts and already large as is to keep the scope clean.
Great work!
Comment #127
lauriiiComment #129
Wim LeersA tiny oversight :)
Comment #130
lauriiiyup :)
Comment #131
Fabianx CreditAttribution: Fabianx as a volunteer commentedUhm, this would always be TRUE, so always the first builder would win?
Also the interface does not allow returning NULL right now.
Did you mean:
$breadcrumb !== NULL
instead?
@Wim: Thoughts on #126?
Comment #132
Wim Leers#131: I think you are referring to
I'm fine with that. But I think it could be considered in scope as well. I'm fine with either way; no strong opinion. This patch is definitely a huge step forward, and adding the cache tags would not require additional API changes, thanks to the value object this patch introduces:
So, given that this is an API change, and it's otherwise ready, +1 for Fabian's preference.
Comment #133
Fabianx CreditAttribution: Fabianx as a volunteer commentedCNW for #131, also some more feedback of #126 needs to be addressed.
Comment #134
Wim LeersMarked #2232609: Cacheable breadcrumbs block, and fix breadcrumb builders as a duplicate.
Comment #135
lauriiiI think I have addressed rest of the remaining changes. We still a need change record.
Comment #137
lauriiiThis should fix hopefully all of the remaining test failures
Comment #138
g.oechsler CreditAttribution: g.oechsler as a volunteer and at Wunder commentedI'm going to give the change record a try.
Comment #139
g.oechsler CreditAttribution: g.oechsler as a volunteer and at Wunder commentedI added a draft for a change record. See https://www.drupal.org/node/2543936.
Comment #140
lauriiiThanks for adding the change record! Overall is very good and clear but would you still like to add code examples for the
\Drupal\Core\Breadcrumb\BreadcrumbBuilderInterface
section?Comment #141
g.oechsler CreditAttribution: g.oechsler as a volunteer and at Wunder commentedDone with adding an implemenation example.
Comment #142
lauriiiThat looks good! :)
Comment #143
Wim LeersThis looks very close!
Nit: could've changed this to
[]
while touching this line anyway.I think
setCacheabilityExpectations()
would be clearer.Shouldn't this be typehinted to the new
Breadcrumb
value object?Nit: 80 cols.
As of #2525910: Ensure token replacements have cacheability + attachments metadata and that it is bubbled in any case,
CacheableMetadata
has aaddCacheableDependency()
method. And sinceBreadcrumb extends CacheableMetadata
, this can now be vastly simplified, to simply::)
EDIT: in fact, further down in the patch, that new method is being used already!
s/terms/each term's/
s/as dependency/as a cacheable dependency/
s/its/it is/
s/on/in/
This looks a bit strange.
Can you explain why it was done this way?
(Also: the indentation is off.)
Same here.
And here.
Also further improved the CR, but it already looked great — thanks!
Comment #144
lauriii#143.8-10 were added on #84 which I don't quite understand.
Fixed all points, thanks for the review!
Comment #145
Wim LeersThe docblock wasn't updated accordingly.
Great, thanks for the massive simplification :)
But that means we now have a
use
statement that we can remove!Nit: s/Drupal/\Drupal/
Comment #146
lauriiiFixed again!
Comment #148
lauriiiFixed the fatal
Comment #151
lauriiiTests should be passing now
Comment #152
Wim LeersLooks great!
Comment #153
alexpottNeeds a reroll.
Comment #154
Wim LeersComment #155
alexpottNeeds a reroll :(
Comment #156
Wim LeersDelivered by your friendly reroll bot! :) Conflict in the exact same hunk again :P
Conflicted with #2543554: Clean up Help & Statistics blocks.
Comment #157
alexpottNot used.
Perhaps this should use TranslationWrapper()? But all cache contexts perhaps should so separate issue.
Some changes I would have made on commit.
Comment #158
Wim LeersRerolling this, including the necessary test coverage for #157.4.
Comment #159
Wim Leersurl.path
cache context. But because theurl.path
cache context is implied by theurl
cache context, this is bit tricky to test in an integration test.Furthermore, we've got boatloads of test coverage already for ensuring bubbling works.
Finally, it looks like breadcrumbs have no existing integration test coverage.
So, I think it's sufficient to ensure that the unit tests assert cacheability metadata. But, sadly, that is not testing anything: the changes to the existing unit tests for breadcrumb builders merely ensure that the existing tests pass, but they don't add test coverage to ensure the cacheability of breadcrumbs is correct.
This reroll fixes that. This doesn't test the edge cases as much as I'd like, but neither do the existing breadcrumb unit tests. Plus,
Breadcrumb
extendsCacheableMetadata
, which is very thoroughly unit tested. So we really only need to test whether the breadcrumb builders are indeed adding the right cacheability metadata. All breadcrumb builders that have existing test coverage were updated. Those that don't have existing test coverage did not receive test coverage — fixing that here is out of scope IMHO.Comment #160
tim.plunkettIs this change desired? I don't think we want to prevent the alter from firing...
Comment #161
Wim LeersGood point.
That just further confirms what I said in #159: existing test coverage for breadcrumbs is lacking.
Comment #162
Wim LeersAnd fixed.
Comment #163
tim.plunkettLet's add coverage for that bit. The FAIL patch is just #159 plus the interdiff (reverting #162).
Comment #165
dawehnerDon't we want to basically use the system path here?
I think this is certainly the wrong thing to do. Instead inject the book.manager in here
nitpick, not needed to change: If we use prophecy below, why not use it here as well
Can we add a todo on Breadcrumb to implement Renderable in the future? This would make this even easier here
Comment #166
Wim Leersurl
cache context (hence it is also calledurl.path
, to indicate that relation). So, AFAICT it's correct to use the path information in the request itself. Whenever you'd use the system path in D7, you'd use the route in D8, for which we have theroute
cache context.Or am I missing something?
class BookNavigationCacheContext extends ContainerAware
is pre-existing. I asked the same back in #56.4. Without this change, the cache context fatals.getCache(Contexts|Tags|MaxAge)()
.Comment #167
dawehnerPlease don't assume that I'm stupid and I don't know what I'm talking about. The excuse to not fix it properly is bad IMHO. We have proxies for that now.
Comment #168
Wim LeersPlease relax.
I only stated that I had the same question. And I repeated the answer so you wouldn't have to look it up.
Zero other implications.
So I set out to do what you're asking. But in doing so, I realized why it is
ContainerAware
. It's the same reasonMenuActiveTrailsCacheContext
is container-aware: to not load services needed by a cache context unless we actually use the cache context. This allows us to still get the label of a cache context without instantiating all services a cache context needs. In other words: this is to ensure that UI showing the cache context labels don't have to load every single service needed for every cache context that is installed.See #2429261-5: Replace the hardcoded cache key on the book navigation block with a 'book navigation' cache context, which points to #2428563-8: Introduce parameter-dependent cache contexts.
Comment #169
tim.plunkettWhat about using StringTranslationTrait ?
Unless we add explicit test coverage for this change, this should be moved to a follow-up.
Other than that, RTBC.
Comment #170
Wim LeersComment #172
Wim Leerst()
and notStringTranslationTrait
: because thegetLabel()
method is static.Reverts the last interdiff and adds test coverage.
Comment #173
tim.plunkettThis addresses all of my concerns. Follow-ups were created, CR was written, RTBC!
Comment #174
Wim LeersUpdated IS to make much more sense.
Comment #176
Wim LeersThe new test case runs before
testBook()
. Which means the node IDs begin slightly higher. Which meant that we ended up with node IDs 7, 10 and 11 in the first "check outline" test. But… because the NID is used as the title prefix, it meant that it was sorted as "10, 11, 7", instead of "7, 10, 11".Before the added test case, it was "1, 4, 5", which does not end up getting sorted differently.
And that's why
str_pad()
fixes this!Comment #177
tim.plunketthttps://www.youtube.com/watch?v=RmwqnqL3Hbg
@Wim Leers++
Comment #178
dawehnerHAHA you haven't read my initial comment before d.o. though it would not be the best idea to post it.
You can use parent: container.trait instead but can we please rather more important add a todo + a followup? Oh wait there is no todo,
so there is no need for a follow up :P
Comment #179
tim.plunkettOpened #2550447: Discuss removing ContainerAware from \Drupal\book\Cache\BookNavigationCacheContext.
parent: container.trait is functionally equivalent, so leaving RTBC.
Comment #180
alexpottUnused use.
I think we should throw an exception if $this->links is already set because if you reset this and there is cacheability metadata attached you've got weird-ness. If people need to completely recreate the breadcrumb in hook_system_breadcrumb_alter() then they can just do
$breadcrumb = new Breadcrumb()
and then do what they like.Comment #181
Wim LeersComment #182
alexpottLet's unit test it. (sorry)
Comment #183
Wim LeersRTBC after having discussed with Alex Pott — this is ridiculously trivial :)
Comment #184
alexpottCommitted bb96982 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
Comment #186
dawehnerNote: This issue caused a required container rebuild.
Comment #187
yched CreditAttribution: yched commentedYay ! Minor leftover though :
- The comment is now a lie :-)
- $breadcrumb is a Breadcrumb object, and will thus never be empty() - do we mean "if ($links = $breadcrumb->getLinks()) {", or can we just remove the condition ?
Comment #188
yched CreditAttribution: yched commentedAnd while we're in there :
is a bit puzzling. I know, Breadcrumb also is a CacheableMetadata, and this is what we're "applying" here, but that's not really obvious in the code snippet, so this reads like "apply the breadcrumb to the render array", which is kind of cryptic.
[edit : consequence of inheritance rather than composition, I guess :-) ]
Maybe we could just add a "Apply the breadcrumb cachability metadata" comment to make that more explicit ?
Comment #189
TR CreditAttribution: TR commentedSo are we now just totally ignoring Change records now needed before commit ?
Comment #190
dawehner@TR
In which sense? We have a chance record, see https://www.drupal.org/node/2543936
Comment #191
Wim Leers#187/#188: Oh, heh, I hadn't seen these comments, but I did open #2550729: Consider making the non-metadata bits of GeneratedUrl, GeneratedLink and FilterProcessResult only extensible, not overridable as you already noticed :) AFAICT that addresses all your concerns! :)
#189/#190: Thanks @dawehner. The one thing we forgot — which is forgotten very often — is to publish the CR. Now published.
EDIT: link fixed.
Comment #192
yched CreditAttribution: yched commented@Wim Leers: yep, those remarks are stale if that other issue gets in :-)
(However, you rather mean #2551907: Follow-up for #2483183: make the Breadcrumb value object use composition instead of inheritance, right ?)
Comment #193
Wim LeersOops, yes of course! Link fixed in my comment :)
Comment #194
fgm#190 /me loves the notion of "chance records" :-)
Comment #195
Wim Leers#194: :D
Comment #197
FreMun CreditAttribution: FreMun commentedHi,
the breadcrumb gets cached in a way that it shows the wrong current page:
The problem:
If you browse between the pages from the left-navigation, the breadcrumb isn't changing anymore after the first click. But if you click on page the is a level deeper, then it changes again.
Example:
http://beta.duneroze.be/kortverblijf ==> Home / Kortverblijf
http://beta.duneroze.be/herstelkuren ==> Home / Kortverblijf
http://beta.duneroze.be/herstelkuren/omschrijving ==> Home / Herstelkuren / omschrijving
http://beta.duneroze.be/herstelkuren/procedure ==> Home / Herstelkuren / omschrijving
http://beta.duneroze.be/woonzorgcentrum ==> Home / Kortverblijf
This is how I created the breadcrumb:
1. I use the default breadcrumb block
2. In the .theme file of my custom theme I have this preprocess function:
Comment #198
catchYou need to add the correct cacheability metadata in your preprocess, or better would be to implement a custom breadcrumb builder and do it there instead of preprocess. Please open a new support request.
Comment #199
tjtj CreditAttribution: tjtj commentedBut https://www.drupal.org/project/drupal/issues/2771541 says this fixes it, and it does not. My breadcrumbs are all wrong, and I see no fix in 8.5.5.
drush cr
did nothing. How do I fix it?