Layout builder assumes all breadcrumb links are routable

Oh, that's a really good point! Good find.

Tentatively tagging for novices. Please find me on Drupal Slack if you need guidance!

I am currently trying to do a first time contributation at Drupalcon Amsterdam 2019, and will have a look at this.

Checked to see if the URL has a Drupal route first, before calling getRouteName() on it.

#6 looks good to me, it uses Url::isRouted which returns bool and Indicates if this Url has a Drupal route.
@breakfull , can you provide me steps how can I test it ?

@Dinesh18 Updated the issue's description with 'Steps to reproduce'

Small fix to patch in #6, incorporating the suggestion in #11.

Alternative solution/code.

NW for tests

Adding some test coverage to demonstrate the reported bug

Thanks! A couple small nits, and then I think this is ready.

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -336,7 +336,12 @@ function layout_builder_system_breadcrumb_alter(Breadcrumb &$breadcrumb, RouteMa
   +      if ($link->getUrl()->isRouted()) {
   +        // Check if URL is routable by Drupal. Because
   +        // external URLs do not have an internal route name.
   +        return $link->getUrl()->getRouteName() !== "entity.entity_view_display.$entity_type_id.default";
   +      }
   +      return TRUE;
   

   Can this be swapped?
   Adding if (!isRouted()) { return TRUE; } as a guard condition makes more sense to me. Returning TRUE at the end reads confusingly to me.

2. +++ b/core/modules/layout_builder/tests/modules/layout_builder_test/layout_builder_test.module
   @@ -106,3 +110,23 @@ function layout_builder_entity_form_display_alter(EntityFormDisplayInterface $fo
   +  $external_link = $link = Link::fromTextAndUrl('External link', Url::fromUri('http://www.example.com'));
   +  $breadcrumb->addLink($external_link);
   

   No need for a local variable

3. +++ b/core/modules/layout_builder/tests/modules/layout_builder_test/layout_builder_test.module
   @@ -106,3 +110,23 @@ function layout_builder_entity_form_display_alter(EntityFormDisplayInterface $fo
   +function layout_builder_test_module_implements_alter(&$implementations, $hook) {
   +  if ($hook === 'system_breadcrumb_alter') {
   +    $group = $implementations['layout_builder_test'];
   +    $implementations = [
   +      'layout_builder_test' => $group,
   +    ] + $implementations;
   +  }
   +}
   

   What is this moving to be after? Please add that as an inline comment.

Thanks for the review!

Addressing the pointers from #19

What is this moving to be after? Please add that as an inline comment.

We need to move our hook_system_breadcrumb_alter implementation before layout_builder_system_breadcrumb_alter. Should we be more explicit about this? Currently we are moving before all the implementations

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -336,6 +336,11 @@ function layout_builder_system_breadcrumb_alter(Breadcrumb &$breadcrumb, RouteMa
   +        // Check if URL is routable by Drupal. Because
   +        // external URLs do not have an internal route name.
   

   I don't know that this comment is valuable, and I'm tempted to remove it. If you think it's worth keeping, then it should be outside the if() and should be one sentence. But to me "Check if the URL has a route before checking the route name" isn't helpful.

2. +++ b/core/modules/layout_builder/tests/modules/layout_builder_test/layout_builder_test.module
   @@ -106,3 +110,24 @@ function layout_builder_entity_form_display_alter(EntityFormDisplayInterface $fo
   +    // Move our hook_system_breadcrumb_alter() implementation to run before
   +    // layout_builder_system_breadcrumb_alter().
   

   This is perfect, thanks!

Seems reasonable to get rid of it. We have several similar instances across core

Finally found this patch, we totally need to fix this... we just made a sandbox module called cms_breadcrumbs for leading external breadcrumbs, and we need an external link in our breadcrumbs

Lets get this patch in asap, what is left to do?

I will be testing this patch on D8.9.x

Great fix, tested on D8.9.x , same patch applies (with fuzz and some .orig files leftover, but applies cleanly)

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!

Thanks @catch and everyone above, team work!