Support from Acquia helps fund testing for Drupal Acquia logo

Comments

herom created an issue. See original summary.

herom’s picture

Issue summary: View changes
herom’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Hm, nothing specific in SystemBreadcrumbBlock about visibility. Not sure where it is specified.

dawehner’s picture

I guess the problem is that \Drupal\system\PathBasedBreadcrumbBuilder::build might return something, even usually it should not for the frontpage.

ameymudras’s picture

Tried debugging the \Drupal\system\PathBasedBreadcrumbBuilder::build but could not find anything here. Problem seems to be somewhere else

ameymudras’s picture

Assigned: Unassigned » ameymudras
ameymudras’s picture

Assigned: ameymudras » Unassigned
Status: Active » Needs review
FileSize
642 bytes

Guys sorry for the previous comment, the issue was \Drupal\system\PathBasedBreadcrumbBuilder::build where the homepage was not correctly identified. Ive created a patch and attached

Status: Needs review » Needs work

The last submitted patch, 8: 2599404-8.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +sprint, +language-base, +Needs tests

Needs tests on its own.

The last submitted patch, 8: 2599404-8.patch, failed testing.

dawehner’s picture

+++ b/core/modules/system/src/PathBasedBreadcrumbBuilder.php
@@ -167,7 +167,8 @@ public function build(RouteMatchInterface $route_match) {
     }
-    if ($path && '/' . $path != $front) {
+    $front_page = \Drupal::service('path.matcher')->isFrontPage();
+    if (!$front_page) {

Do we need to take care about some other bits as well, like

    // Don't show a link to the front-page path.
    $front = $this->config->get('page.front');
    $exclude[$front] = TRUE;

?

It seems to be that there are multiple places where we deal with the frontpage

ameymudras’s picture

Status: Needs work » Needs review
FileSize
644 bytes

@dawehner how about removing

    // Don't show a link to the front-page path.
    $front = $this->config->get('page.front');
    $exclude[$front] = TRUE;

and only handling it at one place? I've attached a inter-diff for the same

ameymudras’s picture

Priority: Normal » Major
dawehner’s picture

I would rather suggest something like this

Status: Needs review » Needs work

The last submitted patch, 15: 2599404-15.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: -sprint

Removing from sprint so it reflects what is currently being worked on.
Unfortunately.

alexpott’s picture

Discussed with @xjm and @catch and manually tested myself. We agreed that this is a major bug because the breadcrumb should not be showing.

alexpott’s picture

Issue tags: +Triaged core major

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dagmar’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Needs review
FileSize
6.06 KB
787 bytes

Status: Needs review » Needs work

The last submitted patch, 21: breadcrumb-front-page-2599404-22.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
6.39 KB
1.06 KB

We should either just use Url or use the injected Url builder - I went for injection since the patch was going that way anyway.

Tests should pass now.

Status: Needs review » Needs work

The last submitted patch, 23: 2599404.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
6.41 KB
-    $front = $this->urlGenerator->generateFromRoute('<front>', [], TRUE)->getGeneratedUrl();
+    $front = $this->urlGenerator->generateFromRoute('<front>', [], [], TRUE)->getGeneratedUrl();

Status: Needs review » Needs work

The last submitted patch, 25: 2599404.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
6.72 KB

So that doesn't work because the front page is '/node' but the url generator makes it '/' which means it doesn't get excluded properly when the literal front page path is there such as node/add/page when '/node' is configured as the front page.

This might do it.

catch’s picture

+++ b/core/modules/system/src/PathBasedBreadcrumbBuilder.php
@@ -99,12 +109,13 @@ class PathBasedBreadcrumbBuilder implements BreadcrumbBuilderInterface {
+  public function __construct(RequestContext $context, AccessManagerInterface $access_manager, RequestMatcherInterface $router, InboundPathProcessorInterface $path_processor, ConfigFactoryInterface $config_factory, UrlGeneratorInterface $url_generator, TitleResolverInterface $title_resolver, AccountInterface $current_user, CurrentPathStack $current_path) {

Also, this is really getting silly. Might want a follow-up to revisit it.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Simon Georges’s picture

Status: Needs review » Needs work

I tried the patch and it seems it doesn't work when Drupal is installed in a sub-directory of the webservice web root, because removing front ("/") doesn't completely work. Is there something like the base_path in D7 we can use?

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

pameeela’s picture

I am hesitant to close this because I can't see where it was fixed, but I can't reproduce this in 9.0.1 and the steps are quite simple so I don't think I'm missing anything.

I tried a few ways, on two different installs, with the default /node home page and with an actual node set as the home page, and with a few languages but I'm not getting a breadcrumb on home in any language.

Home with /node - English (default)

Home with /node - French

Home with /node/1 set to home - Chinese, Traditional (default)

Home with /node/1 set to home - English

shailja179’s picture

Assigned: Unassigned » shailja179
pameeela’s picture

Status: Needs work » Closed (outdated)

I think Closed (outdated) is the right status for this where obviously it was an issue at one time, but so much has changed and it's no longer occurring, but I can't find any likely duplicates.

If anyone can provide updated steps to reproduce the issue (starting from "Install Drupal core"), document those steps in the issue summary and set the issue status back to "Active".

Thanks!

mdupont’s picture

Status: Closed (outdated) » Needs work

I tested it for the latest commit of the 9.1.x branch, then the 8.8.x branch. Cannot reproduce the original issue, the breadcrumb stays hidden on the homepage.

Steps taken:

  • Install Drupal
  • Enable the Language and Content Translation modules
  • Add a second language
  • Visit the home page in both languages, clearing caches between each visit. No breadcrumb.
  • Mark the "Article" content type as translatable, as well as the title field
  • Create an Article node in the default language
  • Translate the node in the second language, changing the title
  • Visit the home page in both languages. Still no breadcrumb, as expected.
  • Change the home page page to node/{node id} of the newly created Article
  • Visit the new home page in both languages, always clearing caches. Still no breadcrumb, as expected.
mdupont’s picture

Status: Needs work » Closed (outdated)

No idea why it changed the status to Needs Work, was not intentional.

shailja179’s picture

Assigned: shailja179 » Unassigned

Unassigning this issue as it could not be reproduced.

pameeela’s picture

Issue tags: +Bug Smash Initiative