PathBasedBreadcrumbBuilder needs to be able to exclude pointless paths

Created on 24 February 2023, over 1 year ago
Updated 1 March 2023, over 1 year ago

Problem/Motivation

IN \Drupal\system\PathBasedBreadcrumbBuilder we have the following code

    // /user is just a redirect, so skip it.
    // @todo Find a better way to deal with /user.
    $exclude['/user'] = TRUE;

Steps to reproduce

Visit batch/something - this will error until πŸ› Method _batch_current_set is called without include core/includes/batch.inc Fixed fixed and once that is it will 404 - but the breadcrumb will contain a link with no title.

Proposed resolution

I think if a route returns an empty title it should not be added to the $links array in \Drupal\system\PathBasedBreadcrumbBuilder::build

But also we could consider trying to fix the @todo and add a route option that this checks for to exclude a route from breadcrumbs.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

πŸ› Bug report
Status

Needs work

Version

10.1 ✨

Component
SystemΒ  β†’

Last updated 2 days ago

No maintainer
Created by

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Live updates comments and jobs are added and updated live.
  • Needs change record

    A change record needs to be drafted before an issue is committed. Note: Change records used to be called change notifications.

Sign in to follow issues

Comments & Activities

  • Issue created by @alexpott
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ
              $title = $this->titleResolver->getTitle($route_request, $route_match->getRouteObject());
              if (!isset($title)) {
                // Fallback to using the raw path component as the title if the
                // route is missing a _title or _title_callback attribute.
                $title = str_replace(['-', '_'], ' ', Unicode::ucfirst(end($path_elements)));
              }
    

    Well this fallback is suppose to prevent empty titles but it's not.

  • Status changed to Needs review over 1 year ago
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    Something like this would make the fallback work as expected.

  • Status changed to Needs work over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Locally ran testEmptyStringStaticTitle without the fix and it passes. Shouldn't this fail? If not would this be out of scope as it's not testing the bug?

  • Status changed to Needs review over 1 year ago
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    @smustgrave yeah that's because of how the code currently works. The test that fails is \Drupal\Tests\Core\Controller\TitleResolverTest::testDynamicTitle(). I added \Drupal\Tests\Core\Controller\TitleResolverTest::testEmptyStringStaticTitle() to show that currently the way dynamic and static title behave is inconsistent.

    I've added yet another test to prove consistent behaviour - testing a route with absolutely no title - ie. no callback or _title.

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    So if we do steps to reproduce on πŸ› [PP-1] Empty breadcrumb at node/add and node/add/{content_type} when frontpage view is enabled Postponed you get a breadcrumb of Home >> Node - it creates a title of Node based on the path. So this code...

              $title = $this->titleResolver->getTitle($route_request, $route_match->getRouteObject());
              if (!isset($title)) {
                // Fallback to using the raw path component as the title if the
                // route is missing a _title or _title_callback attribute.
                $title = str_replace(['-', '_'], ' ', Unicode::ucfirst(end($path_elements)));
              }
    

    Is working as expected. BUT for me this code is pretty interesting if you consider multilingual sites. Paths in Drupal especially the admin interface will be English so I'm not convinced that this behaviour is correct. Another option would be to omit pages without titles from breadcrumbs.

  • πŸ‡«πŸ‡·France andypost
    +++ b/core/lib/Drupal/Core/Controller/TitleResolver.php
    @@ -76,6 +77,12 @@ public function getTitle(Request $request, Route $route) {
           // Fall back to a static string from the route.
           $route_title = $this->t($title, $args, $options);
    ...
    +    if ($route_title === '' || ($route_title instanceof TranslatableMarkup && $route_title->getUntranslatedString() === '')) {
    +      return NULL;
    

    Maybe better to check for instance of before converting value to string value object?

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    @andypost === does not convert anything - see https://3v4l.org/cWOn9

  • πŸ‡«πŸ‡·France andypost

    Thanks πŸ‘ then only #6 a question, maybe follow-up to discuss what to do with empty title for breadcrumbs, for example instead of omitting we can display elepsis character to make it navigable

  • Status changed to RTBC over 1 year ago
  • πŸ‡¬πŸ‡§United Kingdom AaronMcHale Edinburgh, Scotland

    Yeah I also find it interesting that "Batch" and "Node" start appearing with the relevant patch(s) applied. Agree with comment #6, and I guess we're only now seeing the true result of what that part of the Breadcrumb Builder ends up outputting, which is technically better than a weird looking empty breadcrumb part.

    Technically the patch works as expected, so happy to RTBC this.

    But as noted we should revisit that part of the Breadcrumb Builder to determine if that is actually the behaviour we want.

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    Discussed with @catch and @larowlan - there is consensus that rather than using the path we should skip the link - like we do for items that a user does not have access too.

  • Status changed to Needs work over 1 year ago
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    For #11

  • Status changed to Needs review over 1 year ago
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    Here's an implementation that skips paths with no titles. We'll need a change to update this.

  • Status changed to Needs work over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Here's a screenshot of visiting /batch/11

    Breadcrumb is not rendering empty crumb, example Home > [blank]

    Moving to NW for the change record.

Production build 0.69.0 2024