When the page menu item appears as the leading breadcrumb, ticking the option Hide the breadcrumb if the breadcrumb only contains the link to the front page doesn't remove the duplicate, because the route name $url_object->getRouteName() is compared literally to <front> which never matches.

I have code in this sandbox module that hides the breadcrumb properly in this case, whether the front page is a node or a view, by comparing the route and parameters of the front page Url object to those of the breadcrumb under consideration for removal:

$front_page = $site_config->get('page.front');
$front_url = Url::fromUri("internal:$front_page");

I have a patch ready to build & post, but please consider checking in this patch first since it affects exactly the same section of code.

CommentFileSizeAuthor
#7 menu_breadcrumb-duplicate-front-2754521-7.patch2.81 KBrphair
#6 menu_breadcrumb-duplicate-front-2754521-6.patch2.85 KBrphair
#2 menu_breadcrumb-duplicate-front-2754521-2.patch2.24 KBrphair
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rphair created an issue. See original summary.

rphair’s picture

rphair CreditAttribution: rphair as a volunteer commented
FileSize
menu_breadcrumb-duplicate-front-2754521-2.patch2.24 KB

Implemented fix suggested above. Avoids adding extra Home breadcrumb if the first crumb's route name & parameters are exact match of those of the page stored in site config. Needed to update caching rules, to reflect this but also more generally:

  • Site config is therefore a cacheable dependency.
  • This module's own config is also a cacheable dependency, since option changes could invalidate all breadcrumbs.
  • The url.path cache context, perhaps inherited from PathBasedBreadcrumbBuilder, doesn't make sense here since menu position is independent of path: changed it to route.menu_active_trails:$menu_name.
rphair’s picture

rphair CreditAttribution: rphair as a volunteer commented
Assigned: rphair » Unassigned
rphair’s picture

rphair CreditAttribution: rphair as a volunteer commented
Related issues: +#2754437: add Taxonomy Attachment feature

This patch absorbed into more significant patch (2754437).

acbramley’s picture

acbramley CreditAttribution: acbramley at Department of Justice & Community Safety, Victoria commented 
  1. +++ b/src/MenuBasedBreadcrumbBuilder.php
@@ -87,7 +87,10 @@ class MenuBasedBreadcrumbBuilder implements BreadcrumbBuilderInterface {
+    // Changing module options, or <front>, could invalidate any breadcrumb:
+    $site_config = $this->configFactory->get('system.site');
+    $breadcrumb->addCacheableDependency($this->config);
+    $breadcrumb->addCacheableDependency($site_config);

    Definitely a good idea to cache for config. 

  2. +++ b/src/MenuBasedBreadcrumbBuilder.php
@@ -108,6 +111,7 @@ class MenuBasedBreadcrumbBuilder implements BreadcrumbBuilderInterface {
+      $breadcrumb->addCacheContexts(['route.menu_active_trails:' . $menu_name]);

    What does this mean? That it'll cache per active trail on a route for a menu? 

  3. +++ b/src/MenuBasedBreadcrumbBuilder.php
@@ -135,7 +138,19 @@ class MenuBasedBreadcrumbBuilder implements BreadcrumbBuilderInterface {
+      // Since we're not removing the Home link, we must avoid duplicating it
+      // when the <front> page path is already in leading breadcrumb position.
+      if (!empty($links)) {
+        $front_page = $site_config->get('page.front');
+        $front_url = Url::fromUri("internal:$front_page");
+        $first_url = $links[0]->getUrl();
+        if (($front_url->getRouteName() === $first_url->getRouteName()) &&
+          ($front_url->getRouteParameters() === $first_url->getRouteParameters())) {
+          $links[0] = $home;
+        } else {
+          array_unshift($links, $home);
+        }
+      }

    This issue is related to a config setting, but I don't see any checking on that config setting in this logic. This also seems quite verbose for what it's doing?

    Why would we replace the first breadcrumb link if we detect that it's the same as the front page?

rphair’s picture

rphair CreditAttribution: rphair as a volunteer commented
FileSize
menu_breadcrumb-duplicate-front-2754521-6.patch2.85 KB

2) What does this mean? That it'll cache per active trail on a route for a menu?

My understanding from source in class MenuActiveTrailsCacheContext is that it makes the cached breadcrumb for the current page expire when the page's menu trail changes. Therefore it adds that cache dependency after confirming that the current page is being breadcrumbed on that particular menu.

3) Why would we replace the first breadcrumb link if we detect that it's the same as the front page?

That's what the original code was written to do: avoid adding the front page to the breadcrumb trail as it's being built, so it can be added back in later as the site home page if that's what the option settings indicate. The first part of that process is line 117-119:

if ($url_object->getRouteName() != "<front>") {
  $links[] = Link::fromTextAndUrl($text, $url_object);
}

which purports to only add a breadcrumb if it's not the front page. This happens regardless of any option setting. The problem: I believe we have never seen this happen since the condition above is never true. If I am wrong about this please, for my own understanding, let me know under what conditions getRouteName() will ever return <front>.

In the meantime: that's why that conditional check, and the code it contains, have been removed... the real comparison with the front page is in the second part of the code, where there's no point doing it unless the relevant config option is set.

This issue is related to a config setting, but I don't see any checking on that config setting in this logic.

It checks for that config setting (remove_home) later: line 128 of the original code, where the "replacement" happens. Let's say for instance, I don't have the "remove home" option set. In the original (flawed) version, a breadcrumb trail starting with the site home page as a menu entry will be built like this:

  1. (lines 117-119) Thinking I am comparing the lead breadcrumb to <front> before adding it, I add the front page as the leading breadcrumb, linked to the home page node (e.g., /node/1).
  2. (lines 128-134) I add the <front> page explicitly as another breadcrumb, linked to the site home page. Therefore the first two breadcrumb links point to the same page.

That's how it relates to the config setting. The existing code of this module removes the home page breadcrumb and adds it back... but since the first part isn't done properly, you end up with a duplicate whenever that config option is unset.

This also seems quite verbose for what it's doing?

It was written that way to clearly expose the logic, and also to keep variable names that were present in the original code (e.g., $text). Thanks for the feedback & please look at this rephrasing of the first part, in which I've collapsed any variable that's only used once.

In the second part, it seems like it should be easier to compare the two Url objects, but I saw no way of doing this other than strict equality of the route & parameters and nothing else. For instance, the menu-routed home page and the site home page are different in their options property and maybe more, so you can't simply test the objects themselves for equality.

rphair’s picture

rphair CreditAttribution: rphair as a volunteer commented
FileSize
menu_breadcrumb-duplicate-front-2754521-7.patch2.81 KB

grrr, forgot to remove kint()...

rphair’s picture

rphair CreditAttribution: rphair at COSD commented
Status: Needs review » Fixed

Patch referred to above is encompassed by version 8.x-1.0-beta1. The coding & control flow queries in comment #5 have been dealt with in a couple of rewrites that line up the boxcars of code differently in the build() function and tighten up the syntax a bit more.

In the process of dealing with the ambiguities about the <front> page another option called Add "Home" link was added in the beta version, to cover all possible cases of the front page might also be the first item in the breadcrumb trail.

Status: Fixed » Closed (fixed)

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