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

  1. \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
  2. Added a url.path cache context
  3. hook_breadcrumb_alter takes $breadcrumb object as a first parameter instead of an array.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
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
CommentFileSizeAuthor
#183 interdiff.txt1.21 KBWim Leers
#183 make_breadcrumb_block-2483183-183.patch55.65 KBWim Leers
#181 interdiff.txt2.1 KBWim Leers
#181 make_breadcrumb_block-2483183-181.patch54.59 KBWim Leers
#179 interdiff.txt1.17 KBtim.plunkett
#179 2483183-breadcrumb-block-179.patch55.08 KBtim.plunkett
#176 interdiff.txt897 bytesWim Leers
#176 make_breadcrumb_block-2483183-176.patch54.42 KBWim Leers
#172 interdiff.txt4.04 KBWim Leers
#172 make_breadcrumb_block-2483183-172.patch53.85 KBWim Leers
#170 interdiff.txt543 bytesWim Leers
#170 make_breadcrumb_block-2483183-170.patch50.13 KBWim Leers
#166 interdiff.txt584 bytesWim Leers
#166 make_breadcrumb_block-2483183-166.patch50.59 KBWim Leers
#4 cacheable_breadcrumb_block-2483183-4.patch6.46 KBWim Leers
#12 interdiff.txt8.08 KBlauriii
#12 make_breadcrumb_block-2483183-12.patch12.01 KBlauriii
#15 make_breadcrumb_block-2483183-15.patch12.29 KBmoshe weitzman
#17 make_breadcrumb_block-2483183-17.patch12.14 KBmoshe weitzman
#21 make_breadcrumb_block-2483183-21.patch13.19 KBlauriii
#21 interdiff.txt1.55 KBlauriii
#25 make_breadcrumb_block-2483183-25.patch14.09 KBlauriii
#25 interdiff.txt739 byteslauriii
#27 make_breadcrumb_block-2483183-27.patch14.39 KBlauriii
#27 interdiff.txt1.36 KBlauriii
#30 make_breadcrumb_block-2483183-30.patch20.3 KBlauriii
#30 interdiff.txt7.65 KBlauriii
#32 make_breadcrumb_block-2483183-32.patch27.45 KBlauriii
#32 interdiff.txt11.65 KBlauriii
#36 make_breadcrumb_block-2483183-36.patch27.18 KBlauriii
#36 interdiff.txt1.8 KBlauriii
#38 make_breadcrumb_block-2483183-38.patch28.21 KBlauriii
#38 interdiff.txt813 byteslauriii
#48 interdiff-2483183-38-48.txt1.19 KBbfr
#49 make_breadcrumb_block-2483183-48.patch28.56 KBbfr
#49 interdiff-2483183-38-48.txt1.19 KBbfr
#52 make_breadcrumb_block-2483183-52.patch29.99 KBlauriii
#52 interdiff.txt2.21 KBlauriii
#57 make_breadcrumb_block-2483183-57.patch29.75 KBlauriii
#57 interdiff.txt2.88 KBlauriii
#58 suggested-interdiff.txt1.69 KBWim Leers
#62 make_breadcrumb_block-2483183-62.patch33.84 KBlauriii
#62 interdiff.txt4.32 KBlauriii
#65 make_breadcrumb_block-2483183-65.patch33.85 KBmartin107
#69 interdiff-65-69.txt1.01 KBmartin107
#69 make_breadcrumb_block-2483183-69.patch34.16 KBmartin107
#71 make_breadcrumb_block-2483183-71.patch34.26 KBmartin107
#71 interdiff.txt702 bytesmartin107
#74 make_breadcrumb_block-2483183-74.patch34.32 KBmartin107
#74 interdiff-71-74.txt494 bytesmartin107
#77 make_breadcrumb_block-2483183-76.patch34.33 KBmartin107
#77 interdiff-74-76.txt1.45 KBmartin107
#84 interdiff-77-84.txt4.43 KBmartin107
#84 make_breadcrumb_block-2483183-84.patch36.17 KBmartin107
#92 make_breadcrumb_block-2483183-92.patch36.17 KBmartin107
#92 interdiff-84-92.txt608 bytesmartin107
#95 make_breadcrumb_block-2483183-95.patch36.19 KBmartin107
#95 interdiff-92-95.txt1.22 KBmartin107
#97 interdiff.txt455 byteslauriii
#97 make_breadcrumb_block-2483183-97.patch36.22 KBlauriii
#101 make_breadcrumb_block-2483183-101.patch36.22 KBmartin107
#102 make_breadcrumb_block-2483183-102.patch36.21 KBmartin107
#102 interdiff-101-102.patch4.34 KBmartin107
#104 interdiff-101-102.txt4.34 KBmartin107
#106 make_breadcrumb_block-2483183-106.patch36.2 KBmartin107
#117 interdiff-106-117.txt509 bytesmartin107
#117 make_breadcrumb_block-2483183-117.patch36.24 KBmartin107
#121 make_breadcrumb_block-2483183-120.patch35.67 KBlauriii
#121 interdiff.txt1017 byteslauriii
#127 make_breadcrumb_block-2483183-127.patch35.46 KBlauriii
#127 interdiff.txt3.64 KBlauriii
#130 make_breadcrumb_block-2483183-130.patch35.47 KBlauriii
#130 interdiff.txt490 byteslauriii
#135 make_breadcrumb_block-2483183-135.patch37.07 KBlauriii
#135 interdiff.txt8.92 KBlauriii
#137 make_breadcrumb_block-2483183-137.patch43.04 KBlauriii
#137 interdiff.txt9.95 KBlauriii
#144 make_breadcrumb_block-2483183-144.patch41.57 KBlauriii
#144 interdiff.txt11.2 KBlauriii
#146 make_breadcrumb_block-2483183-146.patch41.46 KBlauriii
#146 interdiff.txt2.27 KBlauriii
#148 make_breadcrumb_block-2483183-148.patch41.46 KBlauriii
#148 interdiff.txt564 byteslauriii
#151 make_breadcrumb_block-2483183-151.patch42.12 KBlauriii
#151 interdiff.txt1.37 KBlauriii
#154 make_breadcrumb_block-2483183-154.patch42.09 KBWim Leers
#154 rebase-interdiff.txt447 bytesWim Leers
#156 make_breadcrumb_block-2483183-156.patch41.61 KBWim Leers
#159 make_breadcrumb_block-2483183-159.patch50.54 KBWim Leers
#159 interdiff.txt28.75 KBWim Leers
#162 make_breadcrumb_block-2483183-162.patch50.26 KBWim Leers
#162 interdiff.txt1.14 KBWim Leers
#163 interdiff.txt961 bytestim.plunkett
#163 make_breadcrumb_block-2483183-163-PASS.patch50.5 KBtim.plunkett
#163 make_breadcrumb_block-2483183-163-FAIL.patch50.77 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Wim Leers’s picture

Title: [PP-1] Make breadcrumb block cacheable » Make breadcrumb block cacheable
Status: Postponed » Active
Wim Leers’s picture

This 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.

Wim Leers’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
6.46 KB

Here's a start that outlines the necessary changes:

  1. \Drupal\Core\Breadcrumb\BreadcrumbBuilderInterface::build() needs to return a value object that can encapsulate what it is returning in HEAD plus cacheability metadata
  2. every implementation of BreadcrumbBuilderInterface must return this new kind of object

In 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.

Status: Needs review » Needs work

The last submitted patch, 4: cacheable_breadcrumb_block-2483183-4.patch, failed testing.

dawehner’s picture

Odd, I would have kinda expected that this kind of metadata should be part of each individual link.

Wim Leers’s picture

Why?

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.)

dawehner’s picture

Well, 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

Wim Leers’s picture

But 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 specify route 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?

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: cacheable_breadcrumb_block-2483183-4.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
8.08 KB
12.01 KB

Status: Needs review » Needs work

The last submitted patch, 12: make_breadcrumb_block-2483183-12.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
  1. +++ b/core/lib/Drupal/Core/Breadcrumb/Breadcrumb.php
    @@ -0,0 +1,47 @@
    + * Used to return generated breadcrumbs, along with associated cacheability metadata.
    

    What about: Stores a list of links, along with associated cacheablity metadata.

  2. +++ b/core/lib/Drupal/Core/Breadcrumb/Breadcrumb.php
    @@ -0,0 +1,47 @@
    +class Breadcrumb extends CacheableMetadata {
    ...
    +  public function setLinks(array $links) {
    

    Maybe its me, but I would have expected a single add method.

moshe weitzman’s picture

  1. Fixed parameters in $breadcrumb->merge() call in \Drupal\system\Plugin\Block\SystemBreadcrumbBlock::build
  2. Updated \Drupal\book\BookBreadcrumbBuilder::build. I declared a 'route' context here but perhaps sibling book pages can share same context? Not sure thats worth it. I'd add this if anyone wants to figure it out.

Lets see what else fails.

Status: Needs review » Needs work

The last submitted patch, 15: make_breadcrumb_block-2483183-15.patch, failed testing.

moshe weitzman’s picture

Status: Needs work » Needs review
FileSize
12.14 KB

Fix comment and book breadcrumbs.

Status: Needs review » Needs work

The last submitted patch, 17: make_breadcrumb_block-2483183-17.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 17: make_breadcrumb_block-2483183-17.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
13.19 KB
1.55 KB

Added missing files for the patch

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Breadcrumb/Breadcrumb.php
    @@ -0,0 +1,47 @@
    + * Used to return generated breadcrumbs, along with associated cacheability metadata.
    

    Should start with a verb ending in 's', not "Used"

  2. +++ b/core/lib/Drupal/Core/Breadcrumb/Breadcrumb.php
    @@ -0,0 +1,47 @@
    +    return $this->links ;
    

    Extra space before ;

  3. +++ b/core/lib/Drupal/Core/Breadcrumb/Breadcrumb.php
    @@ -0,0 +1,47 @@
    +  public function setLinks(array $links) {
    

    I think it would be nice to provide an addLink(Link $link) method

Status: Needs review » Needs work

The last submitted patch, 21: make_breadcrumb_block-2483183-21.patch, failed testing.

moshe weitzman’s picture

I'm having trouble reproducing some of these test fails. Hopefully someone else can push this along.

lauriii’s picture

Status: Needs work » Needs review
FileSize
14.09 KB
739 bytes

This should fix the fatal

Status: Needs review » Needs work

The last submitted patch, 25: make_breadcrumb_block-2483183-25.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
14.39 KB
1.36 KB

That didn't fly because it sucked. Maybe this is better :P

Status: Needs review » Needs work

The last submitted patch, 27: make_breadcrumb_block-2483183-27.patch, failed testing.

The last submitted patch, 27: make_breadcrumb_block-2483183-27.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
20.3 KB
7.65 KB

This should be fixing some more of the tests.

Status: Needs review » Needs work

The last submitted patch, 30: make_breadcrumb_block-2483183-30.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
27.45 KB
11.65 KB

Instead 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.

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php
    @@ -99,8 +104,13 @@ public function build(RouteMatchInterface $route_match) {
    -    $this->moduleHandler->alter('system_breadcrumb', $breadcrumb, $route_match, $context);
    ...
    +    $this->moduleHandler->alter('system_breadcrumb', $build['links'], $route_match, $context);
    ...
    +    // Create new Breadcrumb object.
    +    $breadcrumb = new Breadcrumb();
    +    $breadcrumb->setLinks($build['links']);
    +    $breadcrumb->setCacheContexts($build['contexts']);
    

    I 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?

  2. +++ b/core/modules/forum/src/Breadcrumb/ForumNodeBreadcrumbBuilder.php
    @@ -28,19 +28,21 @@ public function applies(RouteMatchInterface $route_match) {
    -    $breadcrumb = parent::build($route_match);
    +    $breadcrumb= parent::build($route_match);
    

    missing space

  3. +++ b/core/modules/forum/tests/src/Unit/Breadcrumb/ForumNodeBreadcrumbBuilderTest.php
    @@ -162,6 +161,8 @@ public function testBuild() {
           )));
     
    +
    +
         $config_factory = $this->getConfigFactoryStub(
    

    Extra

Status: Needs review » Needs work

The last submitted patch, 32: make_breadcrumb_block-2483183-32.patch, failed testing.

The last submitted patch, 32: make_breadcrumb_block-2483183-32.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
27.18 KB
1.8 KB

Didn't pay attention on #33.1 yet

Status: Needs review » Needs work

The last submitted patch, 36: make_breadcrumb_block-2483183-36.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
28.21 KB
813 bytes

This should fix rest of the broken tests :) I will take a look to fix the cache contexts later today.

Status: Needs review » Needs work

The last submitted patch, 38: make_breadcrumb_block-2483183-38.patch, failed testing.

The last submitted patch, 38: make_breadcrumb_block-2483183-38.patch, failed testing.

The last submitted patch, 38: make_breadcrumb_block-2483183-38.patch, failed testing.

bfr’s picture

I went through all the breadcrumbs, and seems like you actually fixed all of them so nothing left to do here.

lauriii’s picture

Status: Needs work » Needs review
bfr’s picture

Assigned: Unassigned » bfr
Status: Needs review » Needs work

Actually found something afterall, will post patch in a minute.

bfr’s picture

Status: Needs work » Needs review
FileSize
1.19 KB

There you go.

bfr’s picture

Yeah, something was missing ;)

bfr’s picture

Assigned: bfr » Unassigned

Status: Needs review » Needs work

The last submitted patch, 49: make_breadcrumb_block-2483183-48.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
29.99 KB
2.21 KB
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I like the patch in general. Having a more specific breadcrumb object makes sense for me.

tim.plunkett’s picture

#33.1 was never addressed.

jibran’s picture

Status: Reviewed & tested by the community » Needs work

Very nice work @lauriii. Thanks @dawehner for setting it to RTBC IMO it's also ready just minor nits and some documentation stuff is pending.

  1. +++ b/core/lib/Drupal/Core/Breadcrumb/Breadcrumb.php
    @@ -0,0 +1,62 @@
    + * Used to return generated breadcrumbs, along with associated cacheability metadata.
    

    Can be re-written so we can adjust it in 80 chars.

  2. +++ b/core/lib/Drupal/Core/Breadcrumb/Breadcrumb.php
    @@ -0,0 +1,62 @@
    +class Breadcrumb extends CacheableMetadata {
    

    We can add unittest for this class.

  3. Can we have some kind of performance stats please?
  4. I think #33.1 makes sense. Thanks @tim.plunkett bringing that up in #54 again.
  5. It would be great if we'd add a change record or update existing change records.
  6. It's a major task so could use beta evaluation.
Wim Leers’s picture

Wow, great progress here! :)

My review:

  1. +++ b/core/lib/Drupal/Core/Breadcrumb/Breadcrumb.php
    @@ -0,0 +1,62 @@
    +   * Add a single link to the end of the breadcrumb.
    ...
    +   *   The link added to the breadcrumb.
    ...
    +  public function addLink(Link $link) {
    

    Nit: "append" is more accurate than "add".

  2. +++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php
    @@ -86,11 +89,13 @@ public function build(RouteMatchInterface $route_match) {
    +        // The builder returned a Breadcrumb object which has to be added to an
    +        // array so it could be altered.
    +        $build['contexts'] = $breadcrumb->getCacheContexts();
    +        $build['links'] = $breadcrumb->getLinks();
             $context['builder'] = $builder;
    

    #33.1++

  3. +++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php
    @@ -99,8 +104,13 @@ public function build(RouteMatchInterface $route_match) {
    +    // Create new Breadcrumb object.
    +    $breadcrumb = new Breadcrumb();
    +    $breadcrumb->setLinks($build['links']);
    +    $breadcrumb->setCacheContexts($build['contexts']);
    

    With #33.1, this also can go away.

  4. +++ b/core/modules/book/book.services.yml
    @@ -26,6 +26,8 @@ services:
    +    calls:
    +      - [setContainer, ['@service_container']]
    

    This change can be reverted.

  5. +++ b/core/modules/comment/src/CommentBreadcrumbBuilder.php
    @@ -47,19 +48,21 @@ public function applies(RouteMatchInterface $route_match) {
    +    $breadcrumb = $breadcrumb->setCacheContexts(['route']);
    

    The assignment here is unnecessary.

  6. +++ b/core/modules/system/src/PathBasedBreadcrumbBuilder.php
    @@ -139,17 +142,21 @@ public function build(RouteMatchInterface $route_match) {
    +    // Because this breadcrumb builder is entirely path-based, vary by the 'url'
    +    // cache context.
    +    $breadcrumb->setCacheContexts(['url']);
    

    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 the PathBasedBreadcrumbBuilder does not care about the query string, yet by specifying that the breadcrumb varies by url, 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.

lauriii’s picture

Issue summary: View changes
FileSize
29.75 KB
2.88 KB

Fixed #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.

Wim Leers’s picture

FileSize
1.69 KB

#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 from url to url.path.

jibran’s picture

Status: Needs work » Needs review

Setting it to needs review helps sometime. :P

lauriii’s picture

And sometimes it makes it worse.. :P

Status: Needs review » Needs work

The last submitted patch, 57: make_breadcrumb_block-2483183-57.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
33.84 KB
4.32 KB

Status: Needs review » Needs work

The last submitted patch, 62: make_breadcrumb_block-2483183-62.patch, failed testing.

Wim Leers’s picture

Woohoo, only 9 failures :) Almost there!

martin107’s picture

Status: Needs work » Needs review
FileSize
33.85 KB

Needed a reroll.. nothing but auto merging and a little 3-way. Lucky patch!

Status: Needs review » Needs work

The last submitted patch, 65: make_breadcrumb_block-2483183-65.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 65: make_breadcrumb_block-2483183-65.patch, failed testing.

martin107’s picture

Assigned: Unassigned » martin107
Status: Needs work » Needs review
FileSize
1.01 KB
34.16 KB

looking 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.

Status: Needs review » Needs work

The last submitted patch, 69: make_breadcrumb_block-2483183-69.patch, failed testing.

martin107’s picture

Looks 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?

    * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
    *   The current route match.
    *
-   * @return \Drupal\Core\Link[]
-   *   An array of links for the breadcrumb. Returning an empty array will
-   *   suppress all breadcrumbs.
+   * @return \Drupal\Core\Breadcrumb\Breadcrumb
+   *   A breadcrumb.
    */ 
martin107’s picture

Status: Needs work » Needs review

Lets see what testbot thinks now.

Status: Needs review » Needs work

The last submitted patch, 71: make_breadcrumb_block-2483183-71.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
34.32 KB
494 bytes

More 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.

Status: Needs review » Needs work

The last submitted patch, 74: make_breadcrumb_block-2483183-74.patch, failed testing.

Wim Leers’s picture

PHP 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.

martin107’s picture

@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

martin107’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 77: make_breadcrumb_block-2483183-76.patch, failed testing.

martin107’s picture

#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. :(

    $route_match = $this->getMock('Drupal\Core\Routing\RouteMatchInterface');
    $this->moduleHandler->expects($this->once())
      ->method('alter')
      ->with('system_breadcrumb', $links, $route_match, array('builder' => $builder));

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.

effulgentsia’s picture

Issue tags: +API change

Tagging 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.

martin107’s picture

As 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?

effulgentsia’s picture

https://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.

martin107’s picture

Status: Needs work » Needs review
FileSize
4.43 KB
36.17 KB

@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

The reason for failure ... the $links parameter is no longer simple to recreate - it has become a more complex array of breadcrumbs and links.

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

I see that text about returning an empty array to suppress all breadcrumbs has been removed.
Is that correctly?

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

Status: Needs review » Needs work

The last submitted patch, 84: make_breadcrumb_block-2483183-84.patch, failed testing.

martin107’s picture

Those 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.

Wim Leers’s picture

So its still down to me to to fix this up, sorry it is taking so many attempts.

No 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 :))

martin107’s picture

Thanks, 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!

Fabianx’s picture

Yes, we have a cache context hierarchy.

So given ['url.path'. 'url'] it gets simplified to just [ 'url' ].

martin107’s picture

@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.

Wim Leers’s picture

Thanks for working on this, I'll review your next patch when it's posted :)

martin107’s picture

Status: Needs work » Needs review
FileSize
36.17 KB
608 bytes

My special talent for over complicating things is not normally so prominent.

Given this from #58

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 from url to url.path.

The fix seems natural.

[One failure remains.]

Status: Needs review » Needs work

The last submitted patch, 92: make_breadcrumb_block-2483183-92.patch, failed testing.

martin107’s picture

Looking 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?

martin107’s picture

Assigned: martin107 » Unassigned
Status: Needs work » Needs review
FileSize
36.19 KB
1.22 KB

I'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

/**
 * Implements hook_system_breadcrumb_alter().
 */
function menu_ui_system_breadcrumb_alter(&$breadcrumb, RouteMatchInterface $route_match, array $context) {
  // Custom breadcrumb behavior for editing menu links, we append a link to
  // the menu in which the link is found.
  if (($route_match->getRouteName() == 'menu_ui.link_edit') && $menu_link = $route_match->getParameter('menu_link_plugin')) {
    if (($menu_link instanceof MenuLinkInterface)) {
      // Add a link to the menu admin screen.
      $menu = Menu::load($menu_link->getMenuName());
      $breadcrumb->addLink(Link::createFromRoute($menu->label(), 'entity.menu.edit_form', array('menu' => $menu->id())));
      }
  }

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.

Status: Needs review » Needs work

The last submitted patch, 95: make_breadcrumb_block-2483183-95.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
455 bytes
36.22 KB

Breadcrumb manager needs to return $build instead of $breadcrumb because otherwise hook_system_breadcrumb_alter doesn't have any affect.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +Needs beta evaluation

My special talent for over complicating things is not normally so prominent.

Thanks for the laughs! :D


@martin107: thanks again for pushing it forward!
@lauriii: thanks for fixing the final failure so simply! :P


  1. +++ b/core/lib/Drupal/Core/Breadcrumb/Breadcrumb.php
    @@ -0,0 +1,62 @@
    +   * An array of links for the breadcrumb.
    

    s/array/ordered list/

  2. +++ b/core/lib/Drupal/Core/Breadcrumb/Breadcrumb.php
    @@ -0,0 +1,62 @@
    +   * Appends a link to the end of the breadcrumb.
    

    s/breadcrumb/ordered list of breadcrumb links/

  3. +++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php
    @@ -76,21 +76,30 @@ public function applies(RouteMatchInterface $route_match) {
    +    $build = new Breadcrumb();
    
    @@ -99,9 +108,9 @@ public function build(RouteMatchInterface $route_match) {
    +    $this->moduleHandler->alter('system_breadcrumb', $build, $route_match, $context);
    

    Why $build, why not just $breadcrumb?

  4. +++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php
    @@ -76,21 +76,30 @@ public function applies(RouteMatchInterface $route_match) {
    +        // The builder returned a Breadcrumb object which has to be added to an
    +        // array so it could be altered.
    +        $build->setCacheContexts($breadcrumb->getCacheContexts());
    +        $build->setLinks($breadcrumb->getLinks());
             $context['builder'] = $builder;
    

    The comment seems wrong now? I think the comment can simply be deleted.

  5. +++ b/core/lib/Drupal/Core/Menu/menu.api.php
    @@ -533,12 +533,8 @@ function hook_contextual_links_plugins_alter(array &$contextual_links) {
    + * @param \Drupal\Core\Breadcrumb\BreadcrumbInterface $breadcrumb
    

    It's not an interface, so s/BreadcrumbInterface/Breadcrumb/

  6. +++ b/core/lib/Drupal/Core/Menu/menu.api.php
    @@ -533,12 +533,8 @@ function hook_contextual_links_plugins_alter(array &$contextual_links) {
    + *   A breadcrumb object returned by the breadcrumb manager build method.
    

    s/the breadcrumb manager build method/BreadcrumbBuilderInterface::build()/

  7. +++ b/core/modules/system/src/PathBasedBreadcrumbBuilder.php
    @@ -139,17 +142,21 @@ public function build(RouteMatchInterface $route_match) {
    +    // Because this breadcrumb builder is entirely path-based, vary by the 'url'
    

    s/url/url.path/

  8. +++ b/core/modules/system/src/Plugin/Block/SystemBreadcrumbBlock.php
    @@ -79,18 +79,12 @@ public function build() {
           return array(
             '#theme' => 'breadcrumb',
    -        '#links' => $breadcrumb,
    +        '#links' => $breadcrumb->getLinks(),
    +        '#cache' => [
    +          'contexts' => $breadcrumb->getCacheContexts(),
    +        ],
    

    Let's instead do:

    $build = [
      '#theme' => 'breadcrumb',
      '#links' => $breadcrumb->getLinks(),
    ];
    $breadcrumb->applyTo($build);
    return $build;
    

    That makes sure that cache max-age & tags are also set on the render array.

The last submitted patch, 92: make_breadcrumb_block-2483183-92.patch, failed testing.

The last submitted patch, 95: make_breadcrumb_block-2483183-95.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
36.22 KB

Before 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.

martin107’s picture

From, from review in #98

1) Fixed.
2) Fixed.
3)

Why $build, why not just $breadcrumb?

$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 :)

Status: Needs review » Needs work

The last submitted patch, 102: interdiff-101-102.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
4.34 KB

hiding interdiff.patch and uploading interdiff.txt

Wim Leers’s picture

Status: Needs review » Needs work

Almost there

  1. +++ b/core/modules/system/src/PathBasedBreadcrumbBuilder.php
    @@ -142,7 +142,7 @@ public function build(RouteMatchInterface $route_match) {
    +    // Because this breadcrumb builder is entirely path-based, vary by the 'url.path'
         // cache context.
    

    Nit: 80 cols.

  2. +++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbBuilderInterface.php
    @@ -32,9 +32,8 @@ public function applies(RouteMatchInterface $route_match);
    +   * @return \Drupal\Core\Breadcrumb\Breadcrumb
    

    This still needs to be changed to BreadcrumbInterface.

  3. +++ b/core/tests/Drupal/Tests/Core/Breadcrumb/BreadcrumbManagerTest.php
    @@ -59,17 +70,24 @@ public function testBuildWithSingleBuilder() {
    +        }
    +      );
    

    These can be on the same line, and the curly brace needs to be moved two spaces back.

    So:

        });
    
  4. +++ b/core/tests/Drupal/Tests/Core/Breadcrumb/BreadcrumbManagerTest.php
    @@ -83,25 +101,33 @@ public function testBuildWithMultipleApplyingBuilders() {
    -      ->with('system_breadcrumb', $breadcrumb2, $route_match, array('builder' => $builder2));
    +         ->willReturnCallback(function($type, $data, $context1, $context2) use ($links2, $route_match, $builder2) {
    

    Broken indentation

  5. +++ b/core/tests/Drupal/Tests/Core/Breadcrumb/BreadcrumbManagerTest.php
    @@ -83,25 +101,33 @@ public function testBuildWithMultipleApplyingBuilders() {
    +        }
    +      );
    

    Same here.

  6. +++ b/core/tests/Drupal/Tests/Core/Breadcrumb/BreadcrumbManagerTest.php
    @@ -116,25 +142,33 @@ public function testBuildWithOneNotApplyingBuilders() {
    +        }
    +      );
    

    And here.

martin107’s picture

Status: Needs work » Needs review
FileSize
36.2 KB

'cuse me while I reroll. No conflicts just auto-merging.

martin107’s picture

Issue summary: View changes
Issue tags: -Needs beta evaluation

Added beta evaluation template

This is a change going in very late, so needs justification.

Status: Needs review » Needs work

The last submitted patch, 106: make_breadcrumb_block-2483183-106.patch, failed testing.

Fabianx’s picture

Issue summary: View changes

#107: Thanks, I improved it with a little different direction.

Not the changes in the patch itself need justification, but the overall issue.

martin107’s picture

Status: Needs work » Needs review

When 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.

Status: Needs review » Needs work

The last submitted patch, 106: make_breadcrumb_block-2483183-106.patch, failed testing.

Wim Leers’s picture

@martin107: try running the tests with run-tests.sh. The web test runner in some rare cases can mask some failures.

martin107’s picture

Status: Needs work » Needs review

Ah 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.

Status: Needs review » Needs work

The last submitted patch, 106: make_breadcrumb_block-2483183-106.patch, failed testing.

martin107’s picture

This 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

Wim Leers’s picture

Status: Needs work » Needs review

Thanks for updating those docs, wonderful!

I think you intentionally kept it at NW, so I'm not changing it to NR.

Status: Needs review » Needs work

The last submitted patch, 117: make_breadcrumb_block-2483183-117.patch, failed testing.

Wim Leers’s picture

LOLWUT

Wim--

lauriii’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
35.67 KB
1017 bytes

We have quite a good test coverage for this in \Drupal\Core\Breadcrumb\BreadcrumbManager

lauriii’s picture

Issue tags: +D8 Accelerate London
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Great work all :)

Fabianx’s picture

Fabianx’s picture

Issue summary: View changes
Fabianx’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

Great patch, little things need work:

  1. +++ b/core/core.services.yml
    @@ -39,6 +39,11 @@ services:
    +  cache_context.url.path:
    +    class: Drupal\Core\Cache\PathCacheContext
    

    nit - this should by in:

    \Drupal\Core\Cache\Context\PathCacheContext

  2. +++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php
    @@ -76,21 +76,28 @@ public function applies(RouteMatchInterface $route_match) {
    -    $breadcrumb = array();
    +    $build = new Breadcrumb();
    ...
    -      $build = $builder->build($route_match);
    +      $breadcrumb = $builder->build($route_match);
    

    It is very confusing to change that around.

    It should be changed back.

  3. +++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php
    @@ -76,21 +76,28 @@ public function applies(RouteMatchInterface $route_match) {
    +      if ($breadcrumb instanceof Breadcrumb) {
    +        $build->setCacheContexts($breadcrumb->getCacheContexts());
    +        $build->setLinks($breadcrumb->getLinks());
             $context['builder'] = $builder;
             break;
    

    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.

  4. +++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php
    @@ -99,9 +106,9 @@ public function build(RouteMatchInterface $route_match) {
    -    $this->moduleHandler->alter('system_breadcrumb', $breadcrumb, $route_match, $context);
    -    // Fall back to an empty breadcrumb.
    -    return $breadcrumb;
    +    $this->moduleHandler->alter('system_breadcrumb', $build, $route_match, $context);
    

    The API change needs to be documented and we need a change record.

  5. +++ b/core/lib/Drupal/Core/Cache/PathCacheContext.php
    @@ -0,0 +1,42 @@
    + * Defines the PathCacheContext service, for "per URL path" caching.
    + *
    + * Cache context ID: 'url.path'.
    

    Nice!

  6. +++ b/core/lib/Drupal/Core/Cache/PathCacheContext.php
    @@ -0,0 +1,42 @@
    + * (This allows for caching generated relative URLs.)
    

    uber nit - I don't think it can only cache generated urls.

  7. +++ b/core/lib/Drupal/Core/Menu/menu.api.php
    @@ -549,9 +545,9 @@ function hook_contextual_links_plugins_alter(array &$contextual_links) {
    -function hook_system_breadcrumb_alter(array &$breadcrumb, \Drupal\Core\Routing\RouteMatchInterface $route_match, array $context) {
    +function hook_system_breadcrumb_alter(Drupal\Core\Breadcrumb\Breadcrumb &$breadcrumb, \Drupal\Core\Routing\RouteMatchInterface $route_match, array $context) {
    

    API change

  8. +++ b/core/lib/Drupal/Core/Menu/menu.api.php
    @@ -549,9 +545,9 @@ function hook_contextual_links_plugins_alter(array &$contextual_links) {
    -  $breadcrumb[] = Drupal::l(t('Text'), 'example_route_name');
    +  $breadcrumb->addLink(Drupal::l(t('Text'), 'example_route_name'));
    

    Love the new Interface!

    This is a great example how value objects / transport objects can make an API cleaner.

  9. +++ b/core/modules/comment/src/CommentBreadcrumbBuilder.php
    @@ -47,19 +48,21 @@ public function applies(RouteMatchInterface $route_match) {
    -    $breadcrumb[] = new Link($entity->label(), $entity->urlInfo());
    +    $links[] = new Link($entity->label(), $entity->urlInfo());
    

    I think this needs to add the cacheableMetadata of the $entity?

    That can be follow-up though - issue is big as is already.

  10. +++ b/core/modules/comment/src/CommentBreadcrumbBuilder.php
    @@ -47,19 +48,21 @@ public function applies(RouteMatchInterface $route_match) {
    -      $breadcrumb[] = new Link($comment->getSubject(), $comment->urlInfo());
    +      $links[] = new Link($comment->getSubject(), $comment->urlInfo());
    

    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.

  11. +++ b/core/modules/forum/src/Breadcrumb/ForumBreadcrumbBuilderBase.php
    @@ -65,14 +66,17 @@ public function __construct(EntityManagerInterface $entity_manager, ConfigFactor
    -    $breadcrumb[] = Link::createFromRoute($vocabulary->label(), 'forum.index');
    +    $links[] = Link::createFromRoute($vocabulary->label(), 'forum.index');
    

    I think this needs to add cacheable metadata of the vocabulary?

    That can be follow-up though - issue is big as is already.

  12. +++ b/core/modules/forum/src/Breadcrumb/ForumListingBreadcrumbBuilder.php
    @@ -34,9 +36,9 @@ public function build(RouteMatchInterface $route_match) {
    -          $breadcrumb[] = Link::createFromRoute($parent->label(), 'forum.page', array(
    +          $breadcrumb->addLink(Link::createFromRoute($parent->label(), 'forum.page', array(
    
    +++ b/core/modules/forum/src/Breadcrumb/ForumNodeBreadcrumbBuilder.php
    @@ -29,18 +29,20 @@ public function applies(RouteMatchInterface $route_match) {
    -        $breadcrumb[] = Link::createFromRoute($parent->label(), 'forum.page',
    +        $breadcrumb->addLink(Link::createFromRoute($parent->label(), 'forum.page',
    

    I think this needs to add cacheable metadata of the forum entity?

    That can be follow-up though - issue is big as is already.

  13. +++ b/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
    @@ -67,9 +68,14 @@ public function build(RouteMatchInterface $route_match) {
           $breadcrumb[] = Link::createFromRoute($term->getName(), 'entity.taxonomy_term.canonical', array('taxonomy_term' => $term->id()));
    ...
         $breadcrumb[] = Link::createFromRoute($this->t('Home'), '<front>');
    

    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!

lauriii’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
35.46 KB
3.64 KB

Status: Needs review » Needs work

The last submitted patch, 127: make_breadcrumb_block-2483183-127.patch, failed testing.

Wim Leers’s picture

Drupal\Tests\Core\Breadcrumb\BreadcrumbManagerTest::testBuildWithoutBuilder Undefined variable: build /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php:82 /var/lib/drupaltestbot/sites/default/files/checkout/core/tests/Drupal/Tests/Core/Breadcrumb/BreadcrumbManagerTest.php:55

A tiny oversight :)

lauriii’s picture

Status: Needs work » Needs review
FileSize
35.47 KB
490 bytes

yup :)

Fabianx’s picture

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php
@@ -95,9 +93,7 @@
         $context['builder'] = $builder;
         break;

Uhm, 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?

Wim Leers’s picture

Issue summary: View changes

#131: I think you are referring to

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.

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:

+++ b/core/lib/Drupal/Core/Breadcrumb/Breadcrumb.php
@@ -0,0 +1,62 @@
+class Breadcrumb extends CacheableMetadata {

So, given that this is an API change, and it's otherwise ready, +1 for Fabian's preference.

Fabianx’s picture

Status: Needs review » Needs work

CNW for #131, also some more feedback of #126 needs to be addressed.

Wim Leers’s picture

lauriii’s picture

Status: Needs work » Needs review
FileSize
37.07 KB
8.92 KB

I think I have addressed rest of the remaining changes. We still a need change record.

Status: Needs review » Needs work

The last submitted patch, 135: make_breadcrumb_block-2483183-135.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
43.04 KB
9.95 KB

This should fix hopefully all of the remaining test failures

g.oechsler’s picture

Assigned: Unassigned » g.oechsler

I'm going to give the change record a try.

g.oechsler’s picture

Assigned: g.oechsler » Unassigned
Issue tags: -Needs change record

I added a draft for a change record. See https://www.drupal.org/node/2543936.

lauriii’s picture

Thanks 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?

g.oechsler’s picture

Done with adding an implemenation example.

lauriii’s picture

That looks good! :)

Wim Leers’s picture

Status: Needs review » Needs work

This looks very close!

  1. +++ b/core/modules/forum/src/Breadcrumb/ForumListingBreadcrumbBuilder.php
    @@ -27,19 +28,26 @@ public function applies(RouteMatchInterface $route_match) {
    +          $breadcrumb->addLink(Link::createFromRoute($parent->label(), 'forum.page', array(
    

    Nit: could've changed this to [] while touching this line anyway.

  2. +++ b/core/modules/forum/tests/src/Unit/Breadcrumb/ForumListingBreadcrumbBuilderTest.php
    @@ -206,7 +228,24 @@ public function testBuild() {
    +  protected function addEmptyCacheableMetadata(&$object) {
    

    I think setCacheabilityExpectations() would be clearer.

  3. +++ b/core/modules/menu_ui/menu_ui.module
    @@ -486,14 +486,14 @@ function menu_ui_preprocess_block(&$variables) {
    +function menu_ui_system_breadcrumb_alter(&$breadcrumb, RouteMatchInterface $route_match, array $context) {
    

    Shouldn't this be typehinted to the new Breadcrumb value object?

  4. +++ b/core/modules/system/src/PathBasedBreadcrumbBuilder.php
    @@ -139,17 +142,21 @@ public function build(RouteMatchInterface $route_match) {
    +    // Because this breadcrumb builder is entirely path-based, vary by the 'url.path'
    +    // cache context.
    

    Nit: 80 cols.

  5. +++ b/core/modules/system/src/PathBasedBreadcrumbBuilder.php
    @@ -139,17 +142,21 @@ public function build(RouteMatchInterface $route_match) {
    +        $breadcrumb = $breadcrumb->merge(CacheableMetadata::createFromObject($access));
    

    As of #2525910: Ensure token replacements have cacheability + attachments metadata and that it is bubbled in any case, CacheableMetadata has a addCacheableDependency() method. And since Breadcrumb extends CacheableMetadata, this can now be vastly simplified, to simply:

    $breadcrumb->addCacheableDependency($access);
    

    :)

    EDIT: in fact, further down in the patch, that new method is being used already!

  6. +++ b/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
    @@ -56,18 +58,28 @@ public function applies(RouteMatchInterface $route_match) {
    +    // Breadcrumb needs to have terms cacheable metadata as dependency even
    

    s/terms/each term's/
    s/as dependency/as a cacheable dependency/

  7. +++ b/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
    @@ -56,18 +58,28 @@ public function applies(RouteMatchInterface $route_match) {
    +    // though its not shown on the breadcrumb because e.g. its parent might have
    

    s/its/it is/
    s/on/in/

  8. +++ b/core/tests/Drupal/Tests/Core/Breadcrumb/BreadcrumbManagerTest.php
    @@ -59,17 +70,24 @@ public function testBuildWithSingleBuilder() {
    +      ->willReturnCallback(function($type, $data, $context1, $context2) use ($route_match, $links, $builder) {
    +        Assert::assertEquals($type, 'system_breadcrumb', 'SingleTest: Module handler expected alter call to system_breadcrumb');
    +        $extracted_links = $data->getLinks();
    +        Assert::assertEquals($extracted_links, $links, 'SingleTest: Module handler was passed the correct link');
    +        Assert::assertEquals($context1, $route_match, 'SingleTest: Module handler was passed route_match.');
    +        Assert::assertEquals($context2, array('builder' => $builder), 'SingleTest: Module handler received the builder context');
    +        }
    +      );
    

    This looks a bit strange.

    Can you explain why it was done this way?

    (Also: the indentation is off.)

  9. +++ b/core/tests/Drupal/Tests/Core/Breadcrumb/BreadcrumbManagerTest.php
    @@ -83,25 +101,33 @@ public function testBuildWithMultipleApplyingBuilders() {
    +         ->willReturnCallback(function($type, $data, $context1, $context2) use ($links2, $route_match, $builder2) {
    +        Assert::assertEquals($type, 'system_breadcrumb', 'MultipleApplying: Module handler expected alter call to system_breadcrumb');
    +        $extracted_links = $data->getLinks();
    +        Assert::assertEquals($extracted_links, $links2, 'MultipleApplying: Module handler was passed the correct link');
    +        Assert::assertEquals($context1, $route_match, 'MultipleApplying: Module handler was passed route_match.');
    +        Assert::assertEquals($context2, array('builder' => $builder2), 'MultipleApplying: Module handler received the builder context');
    +        }
    

    Same here.

  10. +++ b/core/tests/Drupal/Tests/Core/Breadcrumb/BreadcrumbManagerTest.php
    @@ -116,25 +142,33 @@ public function testBuildWithOneNotApplyingBuilders() {
    +      ->willReturnCallback(function($type, $data, $context1, $context2) use ($links2, $route_match, $builder2) {
    +        Assert::assertEquals($type, 'system_breadcrumb', 'NotApplying: Module handler expected alter call to system_breadcrumb');
    +        $extracted_links = $data->getLinks();
    +        Assert::assertEquals($extracted_links, $links2, 'NotApplying: Module handler was passed the correct link');
    +        Assert::assertEquals($context1, $route_match, 'NotApplying: Module handler was passed route_match.');
    +        Assert::assertEquals($context2, array('builder' => $builder2), 'NotApplying: Module handler received the builder context');
    +        }
    

    And here.


Also further improved the CR, but it already looked great — thanks!

lauriii’s picture

Status: Needs work » Needs review
FileSize
41.57 KB
11.2 KB

#143.8-10 were added on #84 which I don't quite understand.

Fixed all points, thanks for the review!

Wim Leers’s picture

  1. +++ b/core/modules/forum/tests/src/Unit/Breadcrumb/ForumListingBreadcrumbBuilderTest.php
    @@ -236,7 +236,7 @@
    -  protected function addEmptyCacheableMetadata(&$object) {
    +  protected function setCacheabilityExpectations(&$object) {
    

    The docblock wasn't updated accordingly.

  2. +++ b/core/tests/Drupal/Tests/Core/Breadcrumb/BreadcrumbManagerTest.php
    @@ -75,14 +75,7 @@
    -      ->willReturnCallback(function($type, $data, $context1, $context2) use ($route_match, $links, $builder) {
    -        Assert::assertEquals($type, 'system_breadcrumb', 'SingleTest: Module handler expected alter call to system_breadcrumb');
    -        $extracted_links = $data->getLinks();
    -        Assert::assertEquals($extracted_links, $links, 'SingleTest: Module handler was passed the correct link');
    -        Assert::assertEquals($context1, $route_match, 'SingleTest: Module handler was passed route_match.');
    -        Assert::assertEquals($context2, array('builder' => $builder), 'SingleTest: Module handler received the builder context');
    -        }
    -      );
    +      ->with('system_breadcrumb', $this->breadcrumb, $route_match, array('builder' => $builder));
    

    Great, thanks for the massive simplification :)

    But that means we now have a use statement that we can remove!

  3. +++ b/core/lib/Drupal/Core/Menu/menu.api.php
    @@ -562,9 +558,9 @@ function hook_contextual_links_plugins_alter(array &$contextual_links) {
    +function hook_system_breadcrumb_alter(Drupal\Core\Breadcrumb\Breadcrumb &$breadcrumb, \Drupal\Core\Routing\RouteMatchInterface $route_match, array $context) {
    

    Nit: s/Drupal/\Drupal/

lauriii’s picture

The last submitted patch, 144: make_breadcrumb_block-2483183-144.patch, failed testing.

lauriii’s picture

The last submitted patch, 146: make_breadcrumb_block-2483183-146.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 148: make_breadcrumb_block-2483183-148.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
42.12 KB
1.37 KB

Tests should be passing now

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Looks great!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
42.09 KB
447 bytes
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll :(

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
41.61 KB

Delivered by your friendly reroll bot! :) Conflict in the exact same hunk again :P

Conflicted with #2543554: Clean up Help & Statistics blocks.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Cache/Context/PathCacheContext.php
    @@ -0,0 +1,50 @@
    +  use StringTranslationTrait;
    

    Not used.

  2. +++ b/core/lib/Drupal/Core/Cache/Context/PathCacheContext.php
    @@ -0,0 +1,50 @@
    +  public static function getLabel() {
    +    return t('Path');
    +  }
    

    Perhaps this should use TranslationWrapper()? But all cache contexts perhaps should so separate issue.

  3. diff --git a/core/lib/Drupal/Core/Cache/Context/PathCacheContext.php b/core/lib/Drupal/Core/Cache/Context/PathCacheContext.php
    index d4c3eee..e0b99a7 100644
    --- a/core/lib/Drupal/Core/Cache/Context/PathCacheContext.php
    +++ b/core/lib/Drupal/Core/Cache/Context/PathCacheContext.php
    @@ -9,7 +9,6 @@
     
     use Drupal\Core\Cache\CacheableMetadata;
     use Drupal\Core\StringTranslation\StringTranslationTrait;
    -use Drupal\Core\Cache\Context\RequestStackCacheContextBase;
     
     /**
      * Defines the PathCacheContext service, for "per URL path" caching.
    diff --git a/core/modules/forum/src/Breadcrumb/ForumListingBreadcrumbBuilder.php b/core/modules/forum/src/Breadcrumb/ForumListingBreadcrumbBuilder.php
    index e421d5c..494af46 100644
    --- a/core/modules/forum/src/Breadcrumb/ForumListingBreadcrumbBuilder.php
    +++ b/core/modules/forum/src/Breadcrumb/ForumListingBreadcrumbBuilder.php
    @@ -7,7 +7,6 @@
     
     namespace Drupal\forum\Breadcrumb;
     
    -use Drupal\Core\Breadcrumb\Breadcrumb;
     use Drupal\Core\Link;
     use Drupal\Core\Routing\RouteMatchInterface;
     
    diff --git a/core/modules/system/src/PathBasedBreadcrumbBuilder.php b/core/modules/system/src/PathBasedBreadcrumbBuilder.php
    index ba3a606..4acbee2 100644
    --- a/core/modules/system/src/PathBasedBreadcrumbBuilder.php
    +++ b/core/modules/system/src/PathBasedBreadcrumbBuilder.php
    @@ -11,7 +11,6 @@
     use Drupal\Core\Access\AccessManagerInterface;
     use Drupal\Core\Breadcrumb\Breadcrumb;
     use Drupal\Core\Breadcrumb\BreadcrumbBuilderInterface;
    -use Drupal\Core\Cache\CacheableMetadata;
     use Drupal\Core\Config\ConfigFactoryInterface;
     use Drupal\Core\Controller\TitleResolverInterface;
     use Drupal\Core\Link;
    diff --git a/core/modules/taxonomy/src/TermBreadcrumbBuilder.php b/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
    index e3b1335..c2387c4 100644
    --- a/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
    +++ b/core/modules/taxonomy/src/TermBreadcrumbBuilder.php
    @@ -9,7 +9,6 @@
     
     use Drupal\Core\Breadcrumb\BreadcrumbBuilderInterface;
     use Drupal\Core\Breadcrumb\Breadcrumb;
    -use Drupal\Core\Cache\CacheableMetadata;
     use Drupal\Core\Entity\EntityManagerInterface;
     use Drupal\Core\Link;
     use Drupal\Core\Routing\RouteMatchInterface;
    @@ -31,7 +30,7 @@ class TermBreadcrumbBuilder implements BreadcrumbBuilderInterface {
       /**
        * The taxonomy storage.
        *
    -   * @var \Drupal\Core\Entity\TermStorageInterface
    +   * @var \Drupal\Taxonomy\TermStorageInterface
        */
       protected $termStorage;
     
    

    Some changes I would have made on commit.

  4. Shouldn't the be some tests that assert that request headers have the correct cache information?
Wim Leers’s picture

Assigned: Unassigned » Wim Leers

Rerolling this, including the necessary test coverage for #157.4.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
FileSize
50.54 KB
28.75 KB
  1. Fixed.
  2. Indeed.
  3. Done.
  4. Until #154, we had an assertion for the path-based breadcrumb builder's url.path cache context. But because the url.path cache context is implied by the url 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 extends CacheableMetadata, 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.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php
@@ -75,21 +75,24 @@ public function applies(RouteMatchInterface $route_match) {
+    // Without builders return early.
...
+    if (empty($sorted_builders)) {
+      return new Breadcrumb();

@@ -99,7 +102,7 @@ public function build(RouteMatchInterface $route_match) {
     $this->moduleHandler->alter('system_breadcrumb', $breadcrumb, $route_match, $context);

Is this change desired? I don't think we want to prevent the alter from firing...

Wim Leers’s picture

Status: Needs review » Needs work

Good point.

That just further confirms what I said in #159: existing test coverage for breadcrumbs is lacking.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
50.26 KB
1.14 KB

And fixed.

tim.plunkett’s picture

Let's add coverage for that bit. The FAIL patch is just #159 plus the interdiff (reverting #162).

The last submitted patch, 163: make_breadcrumb_block-2483183-163-FAIL.patch, failed testing.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Cache/Context/PathCacheContext.php
    @@ -0,0 +1,47 @@
    +
    +/**
    + * Defines the PathCacheContext service, for "per URL path" caching.
    + *
    + * Cache context ID: 'url.path'.
    + *
    + * (This allows for caching relative URLs.)
    + *
    + * @see \Symfony\Component\HttpFoundation\Request::getBasePath()
    + * @see \Symfony\Component\HttpFoundation\Request::getPathInfo()
    + */
    +class PathCacheContext extends RequestStackCacheContextBase implements CacheContextInterface {
    +
    

    Don't we want to basically use the system path here?

  2. +++ b/core/modules/book/book.services.yml
    @@ -26,6 +26,8 @@ services:
       cache_context.route.book_navigation:
         class: Drupal\book\Cache\BookNavigationCacheContext
         arguments: ['@request_stack']
    +    calls:
    +      - [setContainer, ['@service_container']]
         tags:
           - { name: cache.context}
     
    

    I think this is certainly the wrong thing to do. Instead inject the book.manager in here

  3. +++ b/core/modules/forum/tests/src/Unit/Breadcrumb/ForumBreadcrumbBuilderBaseTest.php
    @@ -17,6 +19,22 @@
    +    $cache_contexts_manager = $this->getMockBuilder('Drupal\Core\Cache\Context\CacheContextsManager')
    +      ->disableOriginalConstructor()
    +      ->getMock();
    +    $cache_contexts_manager->expects($this->any())
    +      ->method('validate_tokens');
    

    nitpick, not needed to change: If we use prophecy below, why not use it here as well

  4. +++ b/core/modules/system/src/Plugin/Block/SystemBreadcrumbBlock.php
    @@ -77,20 +77,13 @@ public function build() {
         if (!empty($breadcrumb)) {
           // $breadcrumb is expected to be an array of rendered breadcrumb links.
    -      return array(
    +      $build = [
             '#theme' => 'breadcrumb',
    -        '#links' => $breadcrumb,
    -      );
    +        '#links' => $breadcrumb->getLinks(),
    +      ];
    +      $breadcrumb->applyTo($build);
    +      return $build;
    

    Can we add a todo on Breadcrumb to implement Renderable in the future? This would make this even easier here

Wim Leers’s picture

  1. This cache context represents a specific part of the url cache context (hence it is also called url.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 the route cache context.

    Or am I missing something?

  2. This is merely fixing a bug in HEAD: class BookNavigationCacheContext extends ContainerAware is pre-existing. I asked the same back in #56.4. Without this change, the cache context fatals.
  3. I only updated those to Prophecy that were painful to do without Prophecy (the places where we mocking entities with getCache(Contexts|Tags|MaxAge)().
  4. Good call! Done.
dawehner’s picture

This is merely fixing a bug in HEAD: class BookNavigationCacheContext extends ContainerAware is pre-existing. I asked the same back in #56.4. Without this change, the cache context fatals.

Please 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.

Wim Leers’s picture

Please 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 reason MenuActiveTrailsCacheContext 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.

tim.plunkett’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Cache/Context/PathCacheContext.php
    @@ -0,0 +1,47 @@
    +class PathCacheContext extends RequestStackCacheContextBase implements CacheContextInterface {
    ...
    +    return t('Path');
    

    What about using StringTranslationTrait ?

  2. +++ b/core/modules/book/book.services.yml
    @@ -26,6 +26,8 @@ services:
    +    calls:
    +      - [setContainer, ['@service_container']]
    

    Unless we add explicit test coverage for this change, this should be moved to a follow-up.

Other than that, RTBC.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
50.13 KB
543 bytes
  1. See #157.1 & #157.2: this is consistent with all other cache contexts. Filed a follow-up issue to fix that in all cache contexts: #2550211: Remove the getLabel method from Cache Contexts.
  2. Let's see if tests fail without that change, i.e. let's try to defer that to a follow-up too. If this comes back green, we can defer it, and I'll file a follow-up. If it comes back red, we'll have to add test coverage here.

Status: Needs review » Needs work

The last submitted patch, 170: make_breadcrumb_block-2483183-170.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
53.85 KB
4.04 KB
  1. @borisson_ re-discovered/pointed out at #2550211-2: Remove the getLabel method from Cache Contexts why cache contexts use t() and not StringTranslationTrait: because the getLabel() method is static.
  2. We have our answer: lots and lots of fails, so this change was definitely necessary for this patch. So, here's test coverage.

Reverts the last interdiff and adds test coverage.

tim.plunkett’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

This addresses all of my concerns. Follow-ups were created, CR was written, RTBC!

Wim Leers’s picture

Issue summary: View changes

Updated IS to make much more sense.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 172: make_breadcrumb_block-2483183-172.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
54.42 KB
897 bytes

The 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!

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
dawehner’s picture

Please relax.

I only stated that I had the s

HAHA you haven't read my initial comment before d.o. though it would not be the best idea to post it.

+++ b/core/modules/book/book.services.yml
@@ -26,6 +26,8 @@ services:
+    calls:
+      - [setContainer, ['@service_container']]

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

tim.plunkett’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. diff --git a/core/lib/Drupal/Core/Cache/Context/PathCacheContext.php b/core/lib/Drupal/Core/Cache/Context/PathCacheContext.php
    index 8b13c5a..64a221a 100644
    --- a/core/lib/Drupal/Core/Cache/Context/PathCacheContext.php
    +++ b/core/lib/Drupal/Core/Cache/Context/PathCacheContext.php
    @@ -8,7 +8,6 @@
     namespace Drupal\Core\Cache\Context;
     
     use Drupal\Core\Cache\CacheableMetadata;
    -use Drupal\Core\StringTranslation\StringTranslationTrait;
     
     /**
      * Defines the PathCacheContext service, for "per URL path" caching.
    diff --git a/core/modules/forum/tests/src/Unit/Breadcrumb/ForumListingBreadcrumbBuilderTest.php b/core/modules/forum/tests/src/Unit/Breadcrumb/ForumListingBreadcrumbBuilderTest.php
    index 3935916..0d201ed 100644
    --- a/core/modules/forum/tests/src/Unit/Breadcrumb/ForumListingBreadcrumbBuilderTest.php
    +++ b/core/modules/forum/tests/src/Unit/Breadcrumb/ForumListingBreadcrumbBuilderTest.php
    @@ -9,7 +9,6 @@
     
     use Drupal\Core\Cache\Cache;
     use Drupal\Core\Link;
    -use Drupal\taxonomy\Entity\Term;
     use Drupal\Tests\UnitTestCase;
     use Symfony\Cmf\Component\Routing\RouteObjectInterface;
     use Symfony\Component\DependencyInjection\Container;
    

    Unused use.

  2. +++ b/core/lib/Drupal/Core/Breadcrumb/Breadcrumb.php
    @@ -0,0 +1,64 @@
    +  public function setLinks(array $links) {
    +    $this->links = $links;
    +
    +    return $this;
    +  }
    

    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.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
54.59 KB
2.1 KB
  1. Weird, I'd swear I already did that once :P Done!
  2. Done, and opened #2550729: Consider making the non-metadata bits of GeneratedUrl, GeneratedLink and FilterProcessResult only extensible, not overridable to discuss/address the other, similar cases in core.
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Breadcrumb/Breadcrumb.php
@@ -40,8 +40,15 @@ public function getLinks() {
+    if (!empty($this->links)) {
+      throw new \LogicException('Once breadcrumb links are set, only additional breadcrumb links can be added.');
+    }

Let's unit test it. (sorry)

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
55.65 KB
1.21 KB

RTBC after having discussed with Alex Pott — this is ridiculously trivial :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed bb96982 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed bb96982 on 8.0.x
    Issue #2483183 by lauriii, martin107, Wim Leers, tim.plunkett, moshe...
dawehner’s picture

Note: This issue caused a required container rebuild.

yched’s picture

Yay ! Minor leftover though :

+++ b/core/modules/system/src/Plugin/Block/SystemBreadcrumbBlock.php
@@ -77,20 +77,13 @@ public function build() {
     $breadcrumb = $this->breadcrumbManager->build($this->routeMatch);
     if (!empty($breadcrumb)) {
       // $breadcrumb is expected to be an array of rendered breadcrumb links.

- 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 ?

yched’s picture

And while we're in there :

+++ b/core/modules/system/src/Plugin/Block/SystemBreadcrumbBlock.php
@@ -77,20 +77,13 @@ public function build() {
+      $breadcrumb->applyTo($build);

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 ?

TR’s picture

So are we now just totally ignoring Change records now needed before commit ?

dawehner’s picture

@TR
In which sense? We have a chance record, see https://www.drupal.org/node/2543936

Wim Leers’s picture

#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.

yched’s picture

@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 ?)

Wim Leers’s picture

Oops, yes of course! Link fixed in my comment :)

fgm’s picture

#190 /me loves the notion of "chance records" :-)

Wim Leers’s picture

#194: :D

Status: Fixed » Closed (fixed)

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

FreMun’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Closed (fixed) » Needs work

Hi,

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:

function duneroze_preprocess_breadcrumb(&$variables){
    if(($node = \Drupal::routeMatch()->getParameter('node')) && $variables['breadcrumb']){
        $variables['breadcrumb'][] = array(
            'text' => $node->getTitle(),
            'url' => $node->URL()
            );
    }
}
catch’s picture

Status: Needs work » Closed (fixed)

You 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.

tjtj’s picture

But 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?