template_preprocess_breadcrumb() assumes all breadcrumbs are \Drupal\Core\Link objects

+1, this should at least fix the problem and allow the API to work as it says it should.

This seems like it's papering over the inconsistency rather than fixing it. Why not amend the preprocessor to take an array of link objects only instead of the mixed approach, which creates inconsistency and complexity?

Also, now that we have a link generator available from Twig, this seems pretty related:

#2342745: Allow Twig link function to pass in HTML attributes

Still needs tests, and I don't think #6 has been addressed.

Reroll, working on #6 and tests

I'd rather we fix the inconsistency rather than working around it.

template_preprocess_breadcrumb() assumes all links are Link objects, while hook_system_breadcrumb_alter() tells you that you can use strings, and breadcrumb.html.twig supports it.

Well, I think the alter documentation should be adapted, value objects are a good thing to use here.

They are especially helpful as we potentially could cary along the cacheability metadata.

To be clear, there are usecases to add for example the title of the current page onto the breadcrumb. This is something just done on the theme level, its no logic involved in that,
so a preprocess function should be able to add a string there. This means the template doesn't change.

So in short, this means the template_preprocess_breadcrumb() should look like this .

function template_preprocess_breadcrumb(&$variables) {
  $variables['breadcrumb'] = array();
  /** @var \Drupal\Core\Link $link */
  foreach ($variables['links'] as $key => $link) {
    $is_object = $link instanceof Link; 
    $variables['breadcrumb'][$key] = [
      'text' => $is_object ? $link->getText() : $link, 
      'url' => $is_object ? $link->getUrl()->toString() : NULL,
    ];
  }
}

It could be like this, maybe : the menu_test code generates a breadcrumb with both link and plain strings, which are preprocessed normally.

Minor nit:

+++ b/core/modules/system/templates/breadcrumb.html.twig
@@ -17,7 +17,7 @@
-        {% else %}
+        {%  else %}

An extra space crept in here.

hook_system_breadcrumb_alter() now also deals with link objects only.

Let's just fix the docs here.

Fine, but then we should fix them properly, that makes no sense, now you're doing createFromRoute() with the return value from Drupal::l() :)

For the record, having a breadcrumb that is *not* a link is a perfectly valid use case, e.g. if you want to current page also be shown in the breadcrumb. I did that now with the route: Link::createFromRoute($title, '<none>');

I agree and have a use case in d7 where my client had a mega menu and the headings need to be in the menu aka <nolink> from the special menu items project.

Home link > section title > sub section link > current page title

For the record, having a breadcrumb that is *not* a link is a perfectly valid use case, e.g. if you want to current page also be shown in the breadcrumb. I did that now with the route: Link::createFromRoute($title, '');

When we talked about that on some other issue we came to the conclusion that no link is part of the template level, not part of the breadcrumb level for itself.
So its a different domain where you need just the current page title, which can be implemented purely on the theme level.

Addresses #23

Fine, but then we should fix them properly, that makes no sense, now you're doing createFromRoute() with the return value from Drupal::l() :)

Ha, good point!

Committed 0b5b1f1 and pushed to 8.0.x. Thanks!