Use title/url instead of l() for building breadcrumb

Overall I love this change and I'm sorry we didn't do it sooner. That said, some comments. None are fatal with the possible exception of the __toString() method:

1. +++ b/core/includes/theme.inc
   @@ -2579,7 +2596,7 @@ function drupal_common_theme() {
   +      'variables' => array('links' => [], 'breadcrumb' => []),
   

   Out of curiosity, why [] for the inner arrays but not outer? Seems odd.

2. +++ b/core/lib/Drupal/Core/Link.php
   @@ -0,0 +1,111 @@
   +namespace Drupal\Core;
   

   Are we sure there's no more appropriate namespace? I'm tempted to say Drupal\Core\Page as this is basically HtmlAElement, but I don't want that to become too much of a dumping ground.

3. +++ b/core/lib/Drupal/Core/Link.php
   @@ -0,0 +1,111 @@
   +class Link {
   

   Pursuant to the previous comment, should this be called LinkElement? (I'm OK with no, but that should be a conscious decision.)

4. +++ b/core/lib/Drupal/Core/Link.php
   @@ -0,0 +1,111 @@
   +   * Creates a link object from a given route_name/parameters ...
   

   Why the ellipsis at the end?

5. +++ b/core/lib/Drupal/Core/Link.php
   @@ -0,0 +1,111 @@
   +  public static function createFromRouteArray($text, $route_name, $route_parameters = array(), $options = array()) {
   

   The docblock for this method scares me, but it's the same as we're using for the code that this is replacing so I'll hold my nose for now. :-(

6. +++ b/core/lib/Drupal/Core/Link.php
   @@ -0,0 +1,111 @@
   +  /**
   +   * Returns the URL of the link.
   +   */
   +  public function getUrl() {
   

   Needs @return with type.

7. +++ b/core/lib/Drupal/Core/Link.php
   @@ -0,0 +1,111 @@
   +  public function toString() {
   +    return \Drupal::linkGenerator()->generateFromUrl($this->getText(), $this->getUrl());
   +  }
   

   *cries at the call to \Drupal inside a class*

   We really need to stop doing that. :-(

8. +++ b/core/modules/comment/src/CommentBreadcrumbBuilder.php
   @@ -53,11 +52,11 @@ public function applies(RouteMatchInterface $route_match) {
   +    $breadcrumb[] = new Link($entity->label(), $entity->urlInfo());
   

   Why use the constructor directly here instead of a factory method?

9. +++ b/core/modules/forum/tests/src/Breadcrumb/ForumBreadcrumbBuilderBaseTest.php
   @@ -115,32 +116,19 @@ public function testBuild() {
   -    $property = new \ReflectionProperty('Drupal\forum\Breadcrumb\ForumNodeBreadcrumbBuilder', 'stringTranslation');
   -    $property->setAccessible(TRUE);
   -    $property->setValue($breadcrumb_builder, $translation_manager);
   -
   -    // Add a link generator for l().
   -    $link_generator = $this->getMockBuilder('Drupal\Core\Utility\LinkGeneratorInterface')
   -      ->disableOriginalConstructor()
   -      ->getMock();
   -    $link_generator->expects($this->any())
   -      ->method('generate')
   -      ->will($this->returnArgument(0));
   -    $property = new \ReflectionProperty('Drupal\forum\Breadcrumb\ForumNodeBreadcrumbBuilder', 'linkGenerator');
   -    $property->setAccessible(TRUE);
   -    $property->setValue($breadcrumb_builder, $link_generator);
   +    $breadcrumb_builder->setStringTranslation($translation_manager);
   

   I love this hunk on principle. :-)

+++ b/core/includes/theme.inc
@@ -2615,7 +2615,7 @@ function drupal_common_theme() {
-      'variables' => array('links' => [], 'breadcrumb' => []),
+      'variables' => array('links' => array(), 'breadcrumb' => array()),

I would have gone the other direction, as I find short-array-syntax much easier to read now after working with it for a while. :-)

Because this time we have a Url object available, do you want to have a createFromUrl as well? This feels a lit pointless to be honest.

Well, as is it feels inconsistent. Sometimes you use "new", sometimes a static factory. Even if one of the static factories is trivial it feels like it would be more consistent to always get a link from a static factory. (And of course we can add more later if we find a reason to do so.) I won't block the issue on that, but it seems like a second static factory would be better DX.

Okay sure. Let's add a method on the link generator which can convert a link to a string. This makes more sense anyway.

Oh, I like.

No one in the WSCCI meeting today felt like chiming in, so I'll RTBC it. :-)

I like this, thanks @dawehner and @Crell for reviewing!

Quick review.

+++ b/core/lib/Drupal/Core/Link.php
@@ -0,0 +1,102 @@
+  /**
+   * Returns the text of the link.
+   */
...
+  /**
+   * Returns the URL of the link.
+   */

missing @return

The mixed creation of new Link object seems better to unify.
Probably better get rid of static method and use new Link($text, new Url(...)) all over

+++ b/core/includes/theme.inc
@@ -2196,7 +2196,7 @@ function template_preprocess_page(&$variables) {
     $variables['breadcrumb'] = array(
       '#theme' => 'breadcrumb',
-      '#breadcrumb' => \Drupal::service('breadcrumb')->build(\Drupal::routeMatch()),
+      '#links' => \Drupal::service('breadcrumb')->build(\Drupal::routeMatch()),

@@ -2537,6 +2537,23 @@ function template_preprocess_field_multiple_value_form(&$variables) {
+function template_preprocess_breadcrumb(&$variables) {
...
+  foreach ($links as $key => $link) {
+    $variables['breadcrumb'][$key] = array('text' => $link->getText(), 'url' => $link->getUrl()->toString());

@@ -2598,7 +2615,7 @@ function drupal_common_theme() {
-      'variables' => array('breadcrumb' => NULL),
+      'variables' => array('links' => array(), 'breadcrumb' => array()),

+++ b/core/modules/system/templates/breadcrumb.html.twig
@@ -14,7 +14,7 @@
     {% for item in breadcrumb %}
-      <li>{{ item }}</li>
+      <li><a href="{{ item.url }}">{{ item.text }}</a></li>

breadcrumb is a internal variable populated in preprocess

1. +++ b/core/includes/theme.inc
   @@ -2537,6 +2537,23 @@ function template_preprocess_field_multiple_value_form(&$variables) {
   +
   
   +++ b/core/modules/system/templates/breadcrumb.html.twig
   @@ -14,7 +14,7 @@
   -      <li>{{ item }}</li>
   +      <li><a href="{{ item.url }}">{{ item.text }}</a></li>
        {% endfor %}
   

   I don't like the idea at all, because we're turning just simplified Breadcrumb API that was prepared for themers and sitebuilders via contrib modules but not only for backend stuff. All this patch makes almost imposible to add custom class to particular item, set last item as pure text (common thing in breadcrumbs), we can't alter item in any way.

2. +++ b/core/lib/Drupal/Core/Link.php
   @@ -0,0 +1,106 @@
   +class Link {
   

   Like it, may be move to the separate patch and use it somewhere else in the core?

3. Due to HTML = TRUE in the path based breadcrumb builder it is potentially insecure

   Lets move this logic to particular Builder instance. Because breadcrumbs is list of html items (secure or not, link or divs) but not escaped by default list of links.

4. Theming support is not optimal, currently breacrumbs are just printed out

   So lets use renderable array, because current patch doesn't bring to mich space for themers

5. Altering is somehow is difficul, given that it is already a link

   Now it''s not a link yet but will definitely be and we can't do anything :)

dawehner and I discussed this during today's WSCCI meeting. Conclusion:

* Whether or not to show the current page title is NOT the responsibility of a particular builder. The builders are responsible for per-path or per-route or per-content type distinction. Showing the current page or not is a site-level decision.

* Thus, the correct place to make that decision is... in the breadcrumb block. The builders MUST NOT handle the current page, period. The block can/should be configurable to tack on the current page title for display. That's a display-level decision.

* The block currently does not, therefore there is no regression here that it still doesn't. The builders are still responsible for exactly what they should be responsible for.

* Thus, someone please file a follow-up to allow the breadcrumb block to be configured to show the current page title or not. :-) This issue is not blocked by that.

I think we're back to RTBC then.

Rerolled, just a small conflict with #2291833: Standardize taxonomy term entity route names.

Wow, this looks insanely awesome. Couple of small things, and then this is good to go.

1. +++ b/core/lib/Drupal/Core/Link.php
   @@ -0,0 +1,106 @@
   +   * Creates a link object from a given route name and parameters.
   ...
   +  public static function createFromRouteArray($text, $route_name, $route_parameters = array(), $options = array()) {
   

   Can we shorten to just createFromRoute? It's confusing because neither $text nor $route_name are actually arrays, and we can just type-hint route_parameters / options to arrays to indicate what data type they expect (so let's do that).

2. +++ b/core/lib/Drupal/Core/Link.php
   @@ -0,0 +1,106 @@
   +  public function getText() {
   ...
   +  public function setText($text) {
   ...
   +  public function getUrl() {
   

   Why getText/setText, but only getUrl?

3. +++ b/core/modules/book/src/BookBreadcrumbBuilder.php
   @@ -10,7 +10,7 @@
   -use Drupal\Core\Routing\LinkGeneratorTrait;
   +use Drupal\Core\Link;
   

   Yay!

4. +++ b/core/modules/comment/src/CommentBreadcrumbBuilder.php
   @@ -53,11 +52,11 @@ public function applies(RouteMatchInterface $route_match) {
   -    $breadcrumb[] = \Drupal::linkGenerator()->generateFromUrl($entity->label(), $entity->urlInfo());
   +    $breadcrumb[] = new Link($entity->label(), $entity->urlInfo());
   

   Yay!!!!

         '#theme' => 'breadcrumb',
-        '#breadcrumb' => $breadcrumb,
+        '#links' => $breadcrumb,
       );
     }
   }
diff --git a/core/modules/system/templates/breadcrumb.html.twig b/core/modules/system/templates/breadcrumb.html.twig
index 8a4f438..f6a1705 100644
--- a/core/modules/system/templates/breadcrumb.html.twig
+++ b/core/modules/system/templates/breadcrumb.html.twig
@@ -14,7 +14,13 @@
     <h2 id="system-breadcrumb" class="visually-hidden">{{ 'Breadcrumb'|t }}</h2>
     <ol>
     {% for item in breadcrumb %}
-      <li>{{ item }}</li>
+      <li>
+        {% if item.url %}
+          <a href="{{ item.url }}">{{ item.text }}</a>
+        {% else %}
+          {{ item.text }}
+        {% endif %}
+      </li>
     {% endfor %}

if we change #breadcrumbs to #links, shouldn't we change {% for item in breadcrumb %} to {% for item in links %} ???

Oh never mind "function template_preprocess_breadcrumb(&$variables)"...

Changes per #26. I don't know if there was a reason setUrl was excluded but the parallelism makes sense to me so...

Interdiff only looks big because it's a lot of renaming, courtesy PHPStorm.

Looks good!

Great. Committed and pushed to 8.x. Thanks!

+++ b/core/includes/theme.inc
@@ -2305,6 +2305,23 @@ function template_preprocess_field_multiple_value_form(&$variables) {
+function template_preprocess_breadcrumb(&$variables) {
...
+  $links = $variables['links'];

@@ -2366,7 +2383,7 @@ function drupal_common_theme() {
-      'variables' => array('breadcrumb' => NULL),
+      'variables' => array('links' => array(), 'breadcrumb' => array()),

+++ b/core/modules/system/src/Plugin/Block/SystemBreadcrumbBlock.php
--- a/core/modules/system/templates/breadcrumb.html.twig
+++ b/core/modules/system/templates/breadcrumb.html.twig

+++ b/core/modules/system/templates/breadcrumb.html.twig
@@ -14,7 +14,13 @@
-      <li>{{ item }}</li>
+      <li>

it was fixed but commited a wrong - there's no breadcrumb argument

"What happened with all the committ messages?" Sorry? I don't quite parse that. :)

Committed and pushed #33 to 8.x as well.

@dawehner I noticed that too. It's a Safari-specific Dreditor bug. See https://github.com/dreditor/dreditor/issues/226, I will try to fix it this week.

Unfortunately, this broke the suggestion hook_system_breadcrumb_alter makes. No idea why it would FORCE you to have every array value to be a Drupal\Core\Link object.

Also, why is breadcrumb.html.twig completely bypassing the LinkGenerator, and therefore ignoring any options passed to a link?

See #2345801: template_preprocess_breadcrumb() assumes all breadcrumbs are \Drupal\Core\Link objects