Sub-task of #1889790: [Meta] Allow modules to register links for menus, breadcrumbs and tabs (if not with hook_menu)

Problem/Motivation

Breadcrumbs use a static pattern but should be updated to a service to get the functions out of the global scope.
Breadcrumbs are a sticking point in the panel-ish controller conversion.

Proposed resolution

Create a service to manage breadcrumbs

Remaining tasks

Create the class and register the service
Investigate more about how the system will actually be used.
- on the theme / display level
- modules that provide breadcrumb builder plugins. for this we should investigate the various drupal_set_breadcrumb() that already exist in core.

User interface changes

Breadcrumbs becomes a block

API changes: Introduce BreadcrumbBuilder

drupal_get_breadcrumb() and drupal_set_breadcrumb() will be deprecated or removed.

Instead, we will have a "breadcrumb" service, which can build a breadcrumb for any given request.

The service operates with a series of breadcrumb builder objects, representing the different modules or components of the site that provide breadcrumbs for specific pages - e.g. menu, taxonomy, etc.

The main breadcrumb service will iterate over the registered builder objects, until one of them provides a breadcrumb.

It was discussed whether those builder objects should be services or plugins. For now the consensus is that they should be services, identified by a tag in services.yml.

D8 contrib

Drupal 7 contrib has some interesting advanced solutions for breadcrumb building, that outshine what we are doing here.
In this discussion were mentioned:

  • Path breadcrumbs, mentioned in
    #5 (andypost).
  • Crumbs, mentioned in #7 (donquixote), and #36 (bojanz). This shares the basic ideas of this core architecture, but takes it to a new level:
    - Not the entire breadcrumb, but each segment, is individually negotiated by plugins.
    - Plugins can be sorted in a fine-grained manner.
    - The breadcrumb is built leaf-to-root in a hierarchical way, allowing to combine segments/joins provided by different plugins that do not know about each other.

It would have been an option to put some of this into core, but
- this would have been a lot of work, and we don't have enough time before freeze.
- It would prevent contrib to further improve those systems.

Instead, what we focus on here is:
- Architectural clean-up of the D7 core breadcrumb system.
- Keep it simple, leave advanced use cases to contrib.
- Roll out the red carpet for contrib modules: Allow contrib modules to either provide specific breadcrumb builders, or replace the entire breadcrumb service.

Original, but deprecated part of the issue summary

API changes (deprecated)

drupal_get_breadcrumb and drupal_set_breadcrumb
would become

Drupal::service('breadcrumb')->get()

and

Drupal::service('breadcrumb')->set()

respectively.

Add

Drupal::service('breadcrumb')->append()

method to make it easier to work with.

CommentFileSizeAuthor
#154 interdiff.txt711 bytesandypost
#154 1947536-breadcrumbs-154.patch27.28 KBandypost
#152 interdiff.txt2.33 KBandypost
#152 1947536-breadcrumbs-152.patch27.29 KBandypost
#148 interdiff.txt1.31 KBandypost
#148 1947536-breadcrumbs-146.patch27.32 KBandypost
#143 interdiff.txt4.82 KBandypost
#143 1947536-breadcrumbs-143.patch27.29 KBandypost
#141 interdiff.txt1.31 KBandypost
#141 1947536-breadcrumbs-141.patch25.93 KBandypost
#141 1947536-breadcrumbs-debug.txt2 KBandypost
#141 drop-forum-breadcrumbs.txt2.2 KBandypost
#141 1947536-breadcrumbs-141-no_forum.patch28.13 KBandypost
#140 interdiff.txt693 bytesandypost
#140 1947536-breadcrumbs-140.patch25.84 KBandypost
#139 interdiff.txt1.26 KBandypost
#137 seven.png30.97 KBtim.plunkett
#137 bartik.png16.76 KBtim.plunkett
#137 breadcrumb-1947536-137.patch25.78 KBtim.plunkett
#137 interdiff.txt2.88 KBtim.plunkett
#134 1947536-breadcrumbs-133.patch24.59 KBandypost
#133 interdiff.txt9.21 KBandypost
#133 content-type-111715-184.patch139.9 KBandypost
#130 interdiff.txt3.22 KBandypost
#130 1947536-breadcrumbs-130.patch20.71 KBandypost
#127 1947536-breadcrumbs.patch20.97 KBCrell
#127 interdiff.txt8.79 KBCrell
#126 interdiff.txt4.09 KBandypost
#126 1947536-breadcrumbs-126.patch19.26 KBandypost
#121 interdiff.txt4.93 KBandypost
#121 1947536-breadcrumbs-121.patch20.55 KBandypost
#119 116-119-interdiff.patch4.96 KBalexpott
#119 1947536-breadcrumb-service-119.patch20.54 KBalexpott
#116 1947536-breadcrumb-service-116.patch20.4 KBkim.pepper
#113 1947536-breadcrumb-service-113.patch20.4 KBkim.pepper
#105 d8-breadcrumb-as-service-1947536-105-vs-84.interdiff.txt11.16 KBdonquixote
#105 d8-breadcrumb-as-service-1947536-105.patch20.4 KBdonquixote
#82 drupal-8.x-breadcrumb-as-service-1947536-82-forum.patch3.64 KBdonquixote
#82 drupal-8.x-breadcrumb-as-service-1947536-82-forum-only.patch3.64 KBdonquixote
#84 drupal-8.x-breadcrumb-as-service-1947536-84-testing.patch19.57 KBdonquixote
#84 drupal-8.x-breadcrumb-as-service-1947536-84-testing-diff.patch2.71 KBdonquixote
#77 drupal-8.x-breadcrumb-as-service-1947536-75-plugins.patch12.56 KBdonquixote
#77 drupal-8.x-breadcrumb-as-service-1947536-75-plugins.interdiff.txt15.3 KBdonquixote
#77 drupal-8.x-breadcrumb-as-service-1947536-75.patch13.23 KBdonquixote
#72 drupal-8.x-breadcrumb-as-service-1947536-72.patch10.87 KBdonquixote
#72 drupal-8.x-breadcrumb-as-service-1947536-72.interdiff.txt8.73 KBdonquixote
#62 drupal-8.x-breadcrumb-as-service-1947536-62.patch13.38 KBdonquixote
#62 drupal-8.x-breadcrumb-as-service-1947536-62-vs-49c.interdiff.txt8.38 KBdonquixote
#49 drupal-8.x-breadcrumb-as-service-1947536-49-favouring-splatio.patch11.8 KBdonquixote
#49 drupal-8.x-breadcrumb-as-service-1947536-49-splatio-vs-donquixote.interdiff.txt6.44 KBdonquixote
#49 drupal-8.x-breadcrumb-as-service-1947536-49-favouring-donquixote.patch9.44 KBdonquixote
#45 drupal-8.x-breadcrumb-as-service-1947536-45-vs-42.interdiff.txt3.82 KBdonquixote
#45 drupal-8.x-breadcrumb-as-service-1947536-45.patch9.6 KBdonquixote
#44 convert-breadcrumbs-service-1947536-44-do-not-test.patch9.97 KBmsmithcti
#42 convert-breadcrumbs-service-1947536-42-do-not-test.patch7.33 KBmsmithcti
#20 breadcrumbs-1947536.20.interdiff.txt815 byteslarowlan
#20 breadcrumbs-1947536.20.patch14.07 KBlarowlan
#12 breadcrumbs-1947536.12.interdiff.txt2.56 KBlarowlan
#12 breadcrumbs-1947536.12.patch14 KBlarowlan
#9 breadcrumbs-1947536.9.interdiff.txt7.68 KBlarowlan
#9 breadcrumbs-1947536.9.patch13.71 KBlarowlan
#1 breadcrumbs-1947536.1.patch10.61 KBlarowlan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Status: Active » Needs review
FileSize
10.61 KB

Here's the service and drop of drupal_[s,g]et_breadcrumb

amateescu’s picture

+++ b/core/lib/Drupal/Core/Menu/Breadcrumbs.php
@@ -0,0 +1,76 @@
+  public function __construct() {
+    $this->breadcrumbs = menu_get_active_breadcrumb();
+  }

If the breadcrumb is just a bunch of menu links, I wonder why this class (service) is offered by 'Core' instead of the menu_link module.

larowlan’s picture

Yeah agreed, thought the same thing as I was writing it

andypost’s picture

Status: Needs review » Needs work

@amateescu I disagree that each crumb a menu_link. They just a PluginBag of "render-link" pointing to "router-item"

Also there's a big question about access for each "crumb"

Last weeks I active working with breadcrumbs and I think the Path breadcrumbs is the most sane solution for now.

@larowlan++

podarok’s picture

Issue tags: +Needs change record

due to

+++ b/core/includes/common.incundefined
@@ -257,36 +257,6 @@ function drupal_get_profile() {
-function drupal_set_breadcrumb($breadcrumb = NULL) {
+++ b/core/includes/common.incundefined
@@ -257,36 +257,6 @@ function drupal_get_profile() {
-function drupal_get_breadcrumb() {

this one needs change notification after commit

podarok’s picture

Issue summary: View changes

Updated issue summary.

podarok’s picture

Issue summary: View changes

Updated issue summary.

donquixote’s picture

Create a service to manage breadcrumbs
Create a block for breadcrumbs that has the service injected.
Place this block in the appropriate region in standard install profile.

yes, agree.

Last weeks I active working with breadcrumbs and I think the Path breadcrumbs is the most sane solution for now.

Did you have a look at Crumbs?
I mention this, because it implies a different way to look at the problem: Starting with the current page path, it finds the "parent", the parent of the parent, etc, and ends with the front page. Each step can be provided by a different plugin. (menu, taxonomy, etc). Plugins can be given priorities.

One implication is that you can calculate breadcrumbs independent of the page you are on.

The suggested "API changes" of e.g. Drupal::service('breadcrumb')->append() (what previously was drupal_set_breadcrumb()) would be obsolete, because they are designed to be called from the page callback (that's what I understand).

I don't know if it is realistic to consider this pattern for D8 Core.
The current D7 version relies heavily on the path being the one and only identifier for each breadcrumb element. This would have to be rethought to reflect the new routing system in D8.

It would be reasonable to discuss this in a separate issue.
But, as the API changes of e.g. Drupal::service('breadcrumb')->append() have been proposed in this issue, I mention it here first.

This being said, it might be ok to move forward with the suggested API changes as a 1:1 equivalent to the old system, and postpone any Crumbs-related ideas to a second step.

dawehner’s picture

This being said, it might be ok to move forward with the suggested API changes as a 1:1 equivalent to the old system, and postpone any Crumbs-related ideas to a second step.

Oh yes. Once something is in, refactor it, is way easier, especially when we have already test coverage.

@larowlan
If we add a new function /append/insert we should add test coverage. Just wondering whether insert() is actually that useful, as you most probably don't have a clue about the position in the links?

larowlan’s picture

So this
a) gets tests passing
b) adds an interface so the service can be swapped out by something in contrib like those suggested by @andypost and @donquixote
c) adds unit testing
d) removes the insert method
e) Moves it into menu_link module because thats where it fits better

podarok’s picture

nitpick

+++ b/core/modules/menu_link/lib/Drupal/menu_link/BreadcrumbInterface.phpundefined
@@ -0,0 +1,44 @@
+   * Appends a new crumb or an arroy of crumbs to the breadcrumbs.

arroy

andypost’s picture

Status: Needs work » Needs review
+++ b/core/includes/theme.inc
@@ -2878,7 +2878,7 @@ function template_process_page(&$variables) {
-    $variables['breadcrumb'] = theme('breadcrumb', array('breadcrumb' => drupal_get_breadcrumb()));
+    $variables['breadcrumb'] = theme('breadcrumb', array('breadcrumb' => Drupal::service('breadcrumb')->get()));

Suppose better to implement this as renderable

$variables['breadcrumb'] = array(
'#theme' => 'breadcrumb',
'#breadcrumb' => [service get]
);
+++ b/core/modules/menu_link/lib/Drupal/menu_link/BreadcrumbInterface.php
@@ -0,0 +1,44 @@
+   * Appends a new crumb or an arroy of crumbs to the breadcrumbs.
+   *
+   * @param string|array $breadcrumbs
+   *   The breadcrumb(s)
+   *
+   * @return array
+   *   The current breadcrumbs.
+   */
+  public function append($breadcrumbs);

s/arroy/array

When you appeding, better to point the side (start or end of array)

larowlan’s picture

larowlan’s picture

Issue summary: View changes

code style

larowlan’s picture

Updated issue summary

dawehner’s picture

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Tests/BreadcrumbsUnitTest.phpundefined
@@ -0,0 +1,80 @@
+class BreadcrumbsUnitTest extends UnitTestCase {

I'm wondering whether we should use UnitTestCase here, when menu_get_active_breadcrumb() is actually not runnable in the test.

msonnabaum’s picture

I think using UnitTestCase here is fine since it's only called if the class is instantiated with no args. If that could be passed in in the factory however, I think that'd be better.

Also, why isn't MenuLinkFormController getting the breadcrumb service injected? Would be nice if it wasn't calling out to Drupal::service.

msonnabaum’s picture

Status: Needs review » Needs work
andypost’s picture

+++ b/core/includes/theme.incundefined
@@ -2878,7 +2878,10 @@ function template_process_page(&$variables) {
-    $variables['breadcrumb'] = theme('breadcrumb', array('breadcrumb' => drupal_get_breadcrumb()));
+    $variables['breadcrumb'] = array(
+      '#theme' => 'breadcrumb',
+      '#breadcrumb' => Drupal::service('breadcrumb')->get(),
+    );

It seems that it's too late here for render array.

And please add @todo here pointing to issue #507488: Convert page elements (local tasks, actions) into blocks

larowlan’s picture

Title: Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service, convert breadcrumbs output to a block. » Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service

removed mention of converting to block as that is handled in #507488: Convert page elements (local tasks, actions) into blocks
Updated remaining tasks

larowlan’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

Status: Needs work » Needs review
FileSize
14.07 KB
815 bytes

Adds @todo as requested by @andypost
Resets to using theme() instead of render array in process callback as identified by @andypost.
@msonnabaum we're not yet able to inject dependencies into EntityFormControllers
@msonnobaum can you clarify what you mean by 'If that could be passed in in the factory however, I think that'd be better.' Will try and catch you on irc about it.

andypost’s picture

#20: breadcrumbs-1947536.20.patch queued for re-testing.

larowlan’s picture

Green again.
Spoke with @msonnobaum about this in irc - he's happy with the unit test case for the new functionality, (the existing functionality like menu_get_active_breadcrumb() is subject to simpletest tests already)
Perhaps that should go in a follow up - to move that function to a service/this service?
Or should that be tackled here too?
Injecting dependencies into controllers is covered in #1909418: Allow Entity Controllers to have their dependencies injected

andypost’s picture

Status: Needs review » Needs work

Patch needs re-roll for \Drupal
Also I dislike the menuLink dependency because no menu link code used to theme breadcrumbs (the l() function is used to make links for breadcrumbs array.
So next logical step here to change theme_breadcrumb() to use theme_item_list() because holding a html-pieces is wrong here.

As side note: maybe breadcrumbs better relates to request then page level

+++ b/core/modules/forum/forum.moduleundefined
@@ -266,7 +266,7 @@ function forum_node_view(EntityInterface $node, EntityDisplay $display, $view_mo
+      Drupal::service('breadcrumb')->set($breadcrumb);

+++ b/core/modules/forum/forum.pages.incundefined
@@ -42,7 +42,7 @@ function forum_page($forum_term = NULL) {
+  Drupal::service('breadcrumb')->set($breadcrumb);

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -31,7 +31,7 @@ public function form(array $form, array &$form_state, EntityInterface $menu_link
+      \Drupal::service('breadcrumb')->set($breadcrumb);

+++ b/core/modules/node/node.api.phpundefined
@@ -1338,7 +1338,7 @@ function hook_view(\Drupal\Core\Entity\EntityInterface $node, \Drupal\entity\Plu
+    Drupal::service('breadcrumb')->set($breadcrumb);

+++ b/core/modules/taxonomy/taxonomy.pages.incundefined
@@ -32,7 +32,7 @@ function taxonomy_term_page(Term $term) {
+  Drupal::service('breadcrumb')->set($breadcrumb);

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.phpundefined
@@ -1706,10 +1707,11 @@ public function getBreadcrumb($set = FALSE) {
+        $breadcrumbs = \Drupal::service('breadcrumb');

prefix is not needed for Drupal

amateescu’s picture

prefix is not needed for Drupal

It *is* needed if you're in a namespaced file, so the code above is correct.

larowlan’s picture

So next logical step here to change theme_breadcrumb() to use theme_item_list() because holding a html-pieces is wrong here.

Is that in scope here?

donquixote’s picture

Quick note on the API:
(I realize #7 may be too much for this issue, this one is going to be smaller)

Instead of setting the breadcrumb directly with Drupal::service('breadcrumb')->set($breadcrumb);,
I would rather let a module replace the service for calculating the breadcrumb.

The benefit:
- Breadcrumb won't be calculated twice.
- Breadcrumb won't be calculated when not needed (e.g. when the theme does not want one).

I am not familiar enough with the DI - does it already provide a generic way to replace a service? Or do we need to provide a separate mechanic for that?

------------

If we go this route, would it make sense to split the service into
- a breadcrumb cache, to provide the calculated breadcrumb to the public, and request its calculation the first time it is requested.
- a breadcrumb source, plugged into the breadcrumb cache, to actually calculate the breadcrumb - this is what a module would replace.

Crell’s picture

Agreed with donquixote. Services really should be immutable. They should not be used as a surrogate for a global variable.

What we want here is a block that can, based on DB state and the request, generate a breadcrumb. The logic for that calculation is a service. That service should be swappable per-site. We should *not* be allowing arbitrary code to set a global breadcrumb state; that way lies the madness we're trying to get out of. :-)

There's already a way to replace services in the DIC via Compiler Passes, so we don't need a new mechanism for that. (That's one of the reasons for pushing so much to the DIC: It makes it much easier to swap things out.)

The two services don describes map roughly to how path alias caching works now: An implementation class, and then a caching class that uses the same interface and wraps it in the DIC. That's a stable model we can and should use here.

donquixote’s picture

#28 (Crell)
Sounds fine. But I'd like to add a thought.

We have two situations to take care of:

  1. A module wants to reinvent how breadcrumbs are calculated site-wide.
    Example: Crumbs, Hansel breadcrumbs, Path breadcrumb, etc.
    For these it would make sense to replace the service instead of setting a breadcrumb, as in #27 / #28.
  2. A module wants to define a breadcrumb for a specific page / path.
    Example: Organic Groups, Taxonomy, Forum, etc.
    These are not really interested in replacing the breadcrumb calculation service site-wide.

In the current system, the two can easily conflict with each other.

E.g. in Crumbs I actually had to develop plugins to replace what was otherwise done with drupal_set_breadcrumb(). There are plugins dedicated to forum, taxonomy, organic groups, etc, because Crumbs ignores the result of drupal_set_breadcrumb() (for a good reason).

Now as said before I think it is asked too much to have Crumbs in core.
However, how can we solve situation (2), without getting in the way of contrib breadcrumb modules, and without the drupal_set_breadcrumb() hell, and without ever calculating a breadcrumb that we discard later?

donquixote’s picture

Proposed solution:
A page callback (*) can create a response (*), and this response can contain a hint about breadcrumb-building.
This hint might not be a complete breadcrumb, just a piece of information that can be consumed by the breadcrumb-building service. The "hint" could also be a callback or service by itself. To be discussed.

Then the breadcrumb-building service can consume this hint to work it into the breadcrumb.

(*) I might be misrepresenting how D8 works, but I think the idea is still valid.

Note:
This is actually not a big help for Crumbs in particular, because Crumbs wants to be able to calculate breadcrumbs for arbitrary paths, not just the current page.

Crell’s picture

@larowlan and I discussed this issue in IRC a few days ago. What we came up with was a BreadcrumbManager of sorts that allows multiple breadcrumb services to be registered, with weight. When we go to render a breadcrumb, each breadcrumb service in turn can decide to do so or ignore it, passing it on to the next until one of them decides it knows how to set a breadcrumb. Core can ship with a single low-priority service that uses the menu tree, as is the default now. Forum and OG can add ones with higher weight that would trigger first in the appropriate circumstance. Oh, you want to just take over the site entirely? Register your own breadcrumb service, disable all the others in the Container, and you have complete control. (Or ignore them and just handle all paths.) That gives us both approach 1 and approach 2 from #29, as well as full injection, idempotency, the ability to render the block that has breadcrumbs in it in complete isolation, and puppies.

I think a good approach here would be to model on the PathProcessor interfaces; We have a BreadcrubDefinerInterface (or something like that), which all the individual services implement. Then BreadcrumbManager implements it too, plus an addDefiner() method that takes priority. Wire the manager up in the DIC, use a tag to register the others on it, and we're in business.

don, larowlan, does that make sense to you? Someone want to try writing it? :-) (Building the new one should be easy; the only hard part would be ripping out the old one.)

donquixote’s picture

What you describe sounds like a little version of Crumbs :)

I am going to describe the differences, not to argue for anything, but to make the discussion more interesting.
- Priorities: In Crumbs, you have keys to sort more fine-grained than plugin object level, e.g. "menu.hierarchy.navigation", with wildcards like menu.*, menu.hierarchy.*. The plugin level would be menu.hierarchy.
- Priorities can be managed in a configuration form. Not sure if you are planning that too?
- Crumbs plugins only return the title and parent of one specific breadcrumb item. The rest is done recursively up to the root. This allows a breadcrumb composed of more than one plugin, e.g. Taxonomy + OG. It also reduces redundancy.
- Breadcrumb trails can be determined no matter which page you are on atm. Is this the same as your proposed solution?

As you can imagine, I am hopelessly sold on my own module :)
But I don't really know how I would see this integrated in D8. So all I do here is point out the differences.

Crell’s picture

I see no reason why you couldn't implement that as a breadcrumb definer thingie yourself, with all the sub-plugging (via the Plugin API), configuration forms, etc. you want. But then you could also have it apply only to certain paths and leave the others intact if you were so inclined, or take over entirely; as long as you stick to the same interface, it would work cleanly with everything else in Core with no hackery.

donquixote’s picture

Yes.

The big picture would be:
- a core breadcrumbs API as in #31. Modules like og can provide services to calculate complete breadcrumbs for specific paths.
- a contrib crumbs-8.x that builds on top of this core API. Modules like og can provide plugins to calculate the direct parent of a breadcrumb item, for specific paths.
(- possibly other contrib breadcrumb modules that might provide their own APIs on top of the core API)

So, a contrib module now needs to decide out whether it wants to work with the core API or a contrib API (e.g. Crumbs) or both.
And a contrib breadcrumb API module like Crumbs needs to decide whether it wants to override all breadcrumbs of the core API, or leave some of them intact.

This introduces some redundancy, but it is still acceptable.
And it leaves the big magic in contrib, where it can evolve at a faster pace than it would in core.

----------

If we all agree on this direction, we can move on and iron out the details of #31. E.g. I still don't know if you need to be on the respective page to calculate the breadcrumb, or if you can do that from anywhere.

bojanz’s picture

I find Crumbs the only solution worth mentioning in contrib, due to its approach (calculating the parent of a parent until done, plugins with different weights).

Being able to swap the default breadcrumb service instead of having to override a breadcrumb that was already generated would already be a huge win.
I agree that #31 is a step in a crumbs-like direction and it is already more than I expected Core would consider doing.
I also agree that it would be great to try and tackle the "generate breadcrumb parent by parent" way of generating breadcrumbs, I believe that it is a fundamental concept and really makes the whole process more consistent.

Crell’s picture

Great, so let's top talking and get to writing. :-) Who wants it tackle it?

donquixote’s picture

Some questions to decide:
- Can we calculate a breadcrumb from anywhere, for an arbitrary path? or just on the page itself? What do we start with? A route + arguments?
- Should breadcrumbs have their own module in core? How should it be named? I think it should not be part of menu_links.
- How can we register a service/plugin only for a specific path?
- Who defines the weights/priorities of services? The user?
- How can we minimize wasted operations? E.g. if we have 10 services and the 10th wins, we'd like to avoid running the other 9 of them.

Proposed plan:
We actually make this a new and fresh module that is totally unrelated of existing breadcrumb code in core.
This means, we can leave all the existing stuff alive, and rip it out in a separate patch. This will also make it easier to review those patches.

One big challenge will be to keep this simple and not try to make native core breadcrumbs in D8 better than they are in D7.
The main goal is to roll out the red carpet for D8 contrib. Unless we actually go for "Crumbs in core".

Crell’s picture

1) The breadcrumb calculator should get a request passed to it, and return what it thinks the breadcrumbs should be, or NULL to let someone else decide. Whether that is the current request or a faked request is then not its job to care about.

2) I'm inclined to say the core service goes in /core/lib, and the Menu Links-based implementation goes in the menu links module.

3&5) A calculator (we need to decide what to call this thing) can decide itself to ignore certain paths. That's the point. It's probably too complicated for now to have them only fire on certain path patterns, and not worth the effort unless we find that there really is a huge number of them. If so, the model used by the Access Checker system is probably best. (On route dumping, compute per-route which breadcrumb calculators will care and save it on the route, then have the breadcrumb manager call just those that are registered. But since the most common one will be on nodes, that's probably not going to buy us much.)

4) Priorities would be set in the DIC when wiring up the calculator, so decided by the module dev. We're doing that in plenty of places now, and it's fine. It's also overridable by another module via compiler pass.

Agreed entirely with the "build the new one first" approach in #38; that's what we've been doing in most places. Also agreed with a guideline of "make core clean enough that contrib can do the cool stuff".

Don, want to take a shot at it? :-) (I'm in IRC right now if you want to talk further more efficiently than here.)

Damien Tournoud’s picture

1) The breadcrumb calculator should get a request passed to it, and return what it thinks the breadcrumbs should be, or NULL to let someone else decide. Whether that is the current request or a faked request is then not its job to care about.

I don't buy that. The breadcrumb need to deal with an higher level concept then a Request. A Route and an array of route parameters will probably make more sense, and that's going to be way easier to fake.

donquixote’s picture

Assigned: larowlan » donquixote

I will try to find some time for this this week. If anyone thinks to be faster than that, feel free to reassign :)

msmithcti’s picture

Don't want to step on your toes @donquixote but thought it was worth posting the patch I'd got already. This only provides some basic wiring as described in #31 and #39, there isn't actually any code in the breadcrumb builder build() methods.

donquixote’s picture

That's a good start, thanks!

msmithcti’s picture

Great! Here's a little more then. This patch now also provides and block that renders the breadcrumbs, BreadcrumbManager has been fleshed out more and I've copied the old implementation of the breadcrumbs from menu_get_active_breadcrumb().

Not going to get chance to do anything else for the next few days so leaving this assigned to you @donquixote.

donquixote’s picture

And here we go, a patch, following up on the nice work by splatio.

This is far from finished, but I want to set another starting point for further work.

Left to do:
- Actually move the implementation from menu_get_active_breadcrumb() into the MenuLinkBreadcrumbBuilder service.
- Discuss identifiers of classes, methods, services.
- Improve doc comments.
- Write tests.

And thanks to people on IRC for helping out :)

donquixote’s picture

@splatio: so *now* we are toe-stepping :)
I am going to have a look how different we are.

jibran’s picture

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkBundle.phpundefined
@@ -0,0 +1,30 @@
+  /**
+   * Implements \Symfony\Component\HttpKernel\Bundle\BundleInterface::build().
+   */
+  public function build(ContainerBuilder $container) {
+    // Register the breadcrumb builder from the menu_link module.
+    $container->register('menu_link_breadcrumb', 'Drupal\menu_link\MenuLinkBreadcrumbBuilder')
+      ->addArgument(new Reference('request'))
+      ->addTag('breadcrumb_builder', array('priority' => 200));
+  }

I think according to #1939660: Use YAML as the primary means for service registration we have to change the above to menu_link.services.yml

donquixote’s picture

@splatio: i am doing some merging

donquixote’s picture

Competition is spicy!

I did two differently balanced merges.

The "favouring-splatio" might be a bit misleading, it is still my own footprint in there..

EDIT:
Indeed the getSortedBuilders() is not splatio's implementation! If you want to see that, look in #44.

donquixote’s picture

Status: Needs work » Needs review

@splatio, #44 compared to #42 and #45:

+++ b/core/core.services.yml

I trust you on this one :)

+  protected function getBuilders() {
+    if (empty($this->sortedBuilders)) {
+      $this->sortedBuilders = $this->sortBuilders();
+    }
+
+    return $this->sortedBuilders;

I think having a "lazy" krsort() that does not fire more often than necessary is absolutely justified. The main use case being if contrib adds dozens of builders, and/or if we want to generate many breadcrumbs in a bulk operation.

Having the getBuilders() and sortBuilders() as two separate methods is a matter of taste. I somehow prefer having both in one method, it doesn't look too long to me. But I don't have a strong opinion.

Resetting $this->sortedBuilders to NULL instead of array() seems the more robust option to me.

+    foreach ($this->getBuilders() as $builder) {
+      if (!is_null($builder->build($request))) {
+        return $builder->build();

We should respect the signature of BreadcrumbBuilderInterface::build() ! It always takes a request object.
And we should not fire it more than once.

donquixote’s picture

Status: Needs work » Needs review

Should we try to get this in, and then do the rest in a follow-up patch?
It would make the patches more readable..

donquixote’s picture

donquixote’s picture

donquixote’s picture

msmithcti’s picture

Good job on the two merges! I prefer the BreadcrumbBuilder implementation in the "favouring-spatio" patch (but then I would say that ;). All valid points in #50. For the getSortedBuilders I took my lead from PathProcessorManager which has the separate methods, I also have no real opinion either way, copying a pattern used elsewhere in core is probably a good thing though.

Lets see what someone without a vested interest has to say though! :)

donquixote’s picture

+class MenuLinkBreadcrumbBuilder implements BreadcrumbBuilderInterface {
[..]
+  public function build(Request $request) {
+    // TODO: Move the implementation out of menu.inc, into this method.
+    $breadcrumb = menu_get_active_breadcrumb();
+    return $breadcrumb;
+  }

The rationale here was that we want to rewrite this anyway, so copying it does not add much value atm.

I'd say we wait with the rewrite until the future of menu links is more clear?
(menus as entities, menu links as entities)

donquixote’s picture

+    foreach ($this->getSortedBuilders() as $builder) {
+      $breadcrumb = $builder->build($request);
+      if (isset($breadcrumb) && is_array($breadcrumb)) {
+        return $breadcrumb;
+      }
+    }

We should change this one.
If one of the builders returns FALSE, we should still return it, meaning that there is no breadcrumb on this page. So the check should only be isset().

donquixote’s picture

Another TODO:
Find occurences of drupal_set_breadcrumb() in modules, and figure out how the new system can handle them.
And/or create a "legacy" builder that takes the value from drupal_set_breadcrumb().

donquixote’s picture

+services:
+  menu_link.breadcrumb:
+    class: Drupal\menu_link\MenuLinkBreadcrumbBuilder
+    arguments: ['@request']
+    tags:
+      - { name: breadcrumb_builder, priority: 1000 }

Does this make the request a constructor argument? Why?

donquixote’s picture

How can we suppress the block title? I get a title that says "System Breadcrumb".

donquixote’s picture

Another patch and interdiff.

Some decisions about the options in #49.
- Yes, we want lazy sorting of builders in the BreadcrumbManager.
- No, we don't want to copy the implementation details of menu_get_active_breadcrumb(). We will rewrite that anyway, so for now we should aim for a light-weight patch.
- Yes, we want to "use Drupal;" to avoid the backslash on "\Drupal::service(..)"

New stuff:
- Return value of each builder is now a render array. Whether that is a good idea, can be discussed.
- Return value FALSE means to suppress the breadcrumb.
- Return value NULL means to let other builders decide.
- Priority of MenuLinkBreadcrumbBuilder changed to 0. Other builders can set a negative weight if they really think they should have lower priority. To be discussed.
- Added LegacyBreadcrumbBuilder to read the value of drupal_set_breadcrumb().
- Removed the arguments: ['@request'] in menu_link.services.yml. Request is for build(), not for the constructor.

donquixote’s picture

donquixote’s picture

Obviously this needs a follow-up as soon as this one finishes:
#1863816: Allow plugins to have services injected

donquixote’s picture

The BreadcrumbBuilder services are meant to be agnostic about the "current request" global state. That is, they should care only about the request specified as an argument to the build() method, not about whether that is the current request or another.

Both LegacyBreadcrumbBuilder and MenuLinkBreadcrumbBuilder violate this principle in that they ignore the $request argument, and instead look into global state.

We have to accept this for the time being.

donquixote’s picture

Currently the SystemBreadcrumbBlock makes a default title, which says "System Breadcrumb". Site builders need to uncheck that on every site they build.
It appears in D8 we have no easy way to set an empty title for a block.

This needs to be fixed in a separate issue, then we can follow up. Until then, we should leave it.

Unless timplunkett can come up with a solution :)

tim.plunkett’s picture

Placing your own breadcrumbs block is not something you'll often do.

For now, I followed core's convention and provided one for each theme in standard: http://drupalcode.org/sandbox/Crell/1260830.git/commitdiff/dac6bb8

We may want to look into doing this automatically in hook_themes_enabled() or something.

donquixote’s picture

timplunkett, Crell and donquixote on IRC discussed about the return value of BreadcrumbBuilderInterface::build().

One of the suggestions was to return empty array() instead of FALSE, in case a builder thinks there should not be a breadcrumb on this page.

Imo this is a semantic issue:
Do you say "there is no bottle of wine left", or do you say "the bottle is empty" ?
In the end you want to remove the bottle, you don't want empty bottles left standing around on the table.
To be further discussed.

The empty array() does not feel entirely wrong, so I could live with that instead of FALSE.

It was also suggested to make the returned breadcrumb an object. E.g. we would have

$breadcrumb = Drupal::service('breadcrumb')->build($request);
if ($breadcrumb->isEmpty()) {
  $items = $breadcrumb->getItems();
}

Consensus was, we should postpone that until we know more about how the system will actually be used.
- on the theme / display level
- modules that provide breadcrumb builder plugins. for this we should investigate the various drupal_set_breadcrumb() that already exist in core.

donquixote’s picture

EclipseGc on IRC suggested that BreadcrumbBuilder services should be plugins instead.

So it would be
Drupal\menu_link\Plugin\system\breadcrumb\MenuLinkBreadcrumbBuilder

Did sound promising on IRC.

Crell’s picture

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbBuilderInterface.php
@@ -0,0 +1,29 @@
+   * @return array|FALSE|NULL
+   *   A render array for the breacrumbs, or
+   *   FALSE, to suppress breadcrumbs on this page, or
+   *   NULL, to let other builders decide.

As discussed, this should change to just array|NULL, and specify the significance of an empty array.

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php
@@ -0,0 +1,94 @@
+  /**
+   * Holds the array of breadcrumb builders sorted by priority.
+   *
+   * @var array|NULL
+   *   An array of breadcrumb builders sorted by priorities, or
+   *   NULL, if the array needs to be re-calculated.

Why not use empty array for needs-resort? (I forget off hand what path processors do.)

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php
@@ -0,0 +1,94 @@
+    $this->builders[$priority][] = $builder;
+    // The internal $this->builders array always needs to be correctly sorted.
+    // Builders with high priority are first in the array.
+    krsort($this->builders);
+  }

I don't think we need to resort on add. Just sort $builders when we go to sort the whole thing. What we do need to here, though, is set $sortedBuilders to array() so that they get resorted on next request.

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php
@@ -0,0 +1,94 @@
+      // Merge the nested $this->builders array into $this->sortedBuilders.
+      $this->sortedBuilders = array();
+      foreach ($this->builders as $builders) {
+        $this->sortedBuilders = array_merge($this->sortedBuilders, $builders);

Do we need to merge, or just overwrite?

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkBundle.php
@@ -0,0 +1,30 @@
+/**
+ * Bundle class for menu_link services.
+ */
+class MenuLinkBundle extends Bundle {
+
+  /**
+   * Implements \Symfony\Component\HttpKernel\Bundle\BundleInterface::build().
+   */
+  public function build(ContainerBuilder $container) {
+    // Register the breadcrumb builder from the menu_link module.
+    $container->register('menu_link_breadcrumb', 'Drupal\menu_link\MenuLinkBreadcrumbBuilder')
+      ->addArgument(new Reference('request'))
+      ->addTag('breadcrumb_builder', array('priority' => 200));
+  }

This should be done in a YAML file now.

And donquixote is correct, the request should not be a constructor dependency.

+++ b/core/modules/system/lib/Drupal/system/LegacyBreadcrumbBuilder.php
@@ -0,0 +1,46 @@
+/**
+ * Class to define the legacy breadcrumb builder.
+ *
+ * This breadcrumb builder implements legacy support for the
+ * drupal_set_breadcrumb() mechanic. It may be removed some day.
+ */
+class LegacyBreadcrumbBuilder implements BreadcrumbBuilderInterface {

Good idea. But the comment shouldn't say "some day". We will need to kill this pre-launch. Instead, just say "Remove this once drupal_set_breadcrumb() has been eliminated."

Crell’s picture

No, the model here would not benefit from plugins. It's not UI configurable, and we're not spawning new instances of them. This is just straight up DI.

donquixote’s picture

@Crell,
I will get back to you later.
Though I want to say, some of the issues you bring up have already been mentioned earlier..

I worked on a plugin-based solution that I will post just to show how that would look like (and to reward myself for spending time on this)

donquixote’s picture

@Crell (#70)
(assuming all comments are refering to #62)

As discussed, this should change to just array|NULL, and specify the significance of an empty array.

What is the render result of an empty render array?

Why not use empty array for needs-resort? (I forget off hand what path processors do.)

Empty array means there are no builders.
NULL means the sort never happened.

I don't think we need to resort on add. Just sort $builders when we go to sort the whole thing. What we do need to here, though, is set $sortedBuilders to array() so that they get resorted on next request.

Ooops. I forgot to remove that krsort(). There is already getSortedBuilders() which does the job.

Do we need to merge, or just overwrite?

In the end we want the array to contain all builders, in the correct order. And we don't care about preserving the keys.
So array_merge() is the correct way to do that.

.. MenuLinkBundle ..
This should be done in a YAML file now.

Absolutely.

Good idea. But the comment shouldn't say "some day". We will need to kill this pre-launch. Instead, just say "Remove this once drupal_set_breadcrumb() has been eliminated."

Fine.

donquixote’s picture

Duh, the patch #72 contains some junk (core/lib/Drupal/Core/Breadcrumb/BreadcrumbPluginManager.php)

Crell’s picture

Status: Needs review » Needs work

Yes, that was re #62.

donquixote’s picture

donquixote’s picture

@Crell (#71), myself (#77)

Plugins vs services:
To summarize the benefits of each, as I understand them:

Services:
- easy way to have only one instance per service and keep that during the request
- easy to inject dependencies with DIC
- configured in *.services.yml
- using tags to have it be recognized as a breadcrumb builder, needs a compiler pass.
- performance benefit from DIC compilation?

Plugins:
- annotations instead of *.services.yml -> can all be in one class
- no DIC for plugins (yet)
- configurations can be managed by a unified system. This probably makes sense if all breadcrumb builders have a similar structure of configuration. which is not the case. (currently we don't plan any config, but if we get some they could be totally arbitrary for each plugin)

donquixote’s picture

#77 (*-plugins) patch notes.

I am being bold there.
Instead of extending PluginManagerBase, I let the breadcrumb manager use an instance of a "generic" plugin manager.
Seems cleaner to me, since now the main service (BreadcrumbBuilder) has no ancient magic powers from PluginManagerBase that would be visible to the outside world.

donquixote’s picture

Status: Needs review » Needs work

The fail is because two breadcrumbs show up on the page. One in a block, one in the theme.

See
Drupal\system\Tests\Menu::assertBreadcrumb()
Drupal\system\Tests\Menu::getParts()

    $parts = $this->getParts();
    $pass = TRUE;
    foreach ($trail as $path => $title) {
      $url = url($path);
      $part = array_shift($parts);
      $pass = ($pass && $part['href'] === $url && $part['text'] === check_plain($title));
    }
    // No parts must be left, or an expected "Home" will always pass.
    $pass = ($pass && empty($parts));
[..]

  protected function getParts() {
    $parts = array();
    $elements = $this->xpath('//nav[@class="breadcrumb"]/ol/li/a');
[..]

Either we have to change the test to allow more than one breadcrumb.
Or we have to remove the other breadcrumb.

donquixote’s picture

Adding a forum plugin.
This will still fail, because of the duplicate breadcrumb.

EDIT: I'm saying plugin all the time.
It is just a service.

EDIT: And it's both the same patch. Duh.

EclipseGc’s picture

Status: Needs work » Needs review

didn't test.

donquixote’s picture

Some rough changes to the test to make it pass with multiple breadcrumbs.

Crell’s picture

Status: Needs work » Needs review

Is the second patch in #84 supposed to be an interdiff? I'm not clear on what it is...

(You may also want to use shorter file names to avoid breaking the page layout; just issue nid-breadcrumbs-as-service should be sufficient.)

donquixote’s picture

#84 is a patch which applies both to 8.x and the "modified" 8.x with the previous stuff applied.
Unfortunately I did the wrong upload in #82 so now it is rather a standalone thing.

(For patch names: I also like to keep things sorted on my own machine. Without the project name and version prefix, I would be even worse.)

andypost’s picture

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.phpundefined
@@ -0,0 +1,120 @@
+ * Breadcrumb manager.
+ *
+ * Holds an array of path processor objects and uses them to sequentially
+ * process a path, in order of processor priority.
+ */
+class BreadcrumbManager implements BreadcrumbBuilderInterface {

This one needs better doc-block to show example and point to implementations

+++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Breadcrumb/LegacyBreadcrumbBuilder.phpundefined
@@ -0,0 +1,56 @@
+ * Class to define the legacy breadcrumb builder.
...
+ * This breadcrumb builder implements legacy support for the
+ * drupal_set_breadcrumb() mechanic.
+ * Remove this once drupal_set_breadcrumb() has been eliminated.
+ *
+ * @Plugin(
+ *   id = "system_legacy_breadcrumb_builder",
+ *   admin_label = @Translation("Menu link"),
+ *   module = "menu_link",
+ *   weight = 500
+ * )
+ */
+class LegacyBreadcrumbBuilder implements BreadcrumbBuilderInterface {

+++ b/core/modules/system/lib/Drupal/system/Plugin/block/block/SystemBreadcrumbBlock.phpundefined
@@ -0,0 +1,42 @@
+class SystemBreadcrumbBlock extends BlockBase {

Suppose this needs follow-up to get rid of and point to SystemBreadcrumbBlock that accepts "injected Request"
So mark this one as @depricated and link to d.o/issue

andypost’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

we should postpone that until we know more about how the system will actually be used.
- on the theme / display level
- modules that provide breadcrumb builder plugins. for this we should investigate the various drupal_set_breadcrumb() that already exist in core.

Probably better to file new META issue to discuss
Also summary needs update to figure new plugin implementation and state

EDIT Related #1909418: Allow Entity Controllers to have their dependencies injected

donquixote’s picture

Do you guys agree with the overall architectural direction this is taking?

msmithcti’s picture

I'm a little confused whether a decision has been made yet regarding service vs plugin? I'd agree with what Crell said in #71 and that these shouldn't really be candidates for plugins. Assuming #84 is the patch to review:

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbBuilderInterface.phpundefined
@@ -0,0 +1,29 @@
+  /**
+   * Build the breadcrumb.
+   *
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   The HttpRequest object representing the current request.
+   *
+   * @return array|FALSE|NULL
+   *   A render array for the breacrumbs, or
+   *   FALSE, to suppress breadcrumbs on this page, or
+   *   NULL, to let other builders decide.
+   */
+  public function build(Request $request);

I don't think this should return false and should just return an empty array instead. That allows anything calling the breadcrumb service to just loop through the array if needed instead of checking if its false first. I'm fairly sure theme_breadcrumb() will handle an empty array fine.

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.phpundefined
@@ -0,0 +1,91 @@
+  /**
+   * Holds the array of breadcrumb builders sorted by priority.
+   *
+   * @var array|NULL
+   *   An array of breadcrumb builders sorted by priorities, or
+   *   NULL, if the array needs to be re-calculated.
+   */
+  protected $sortedBuilders;

I like that this is set to NULL for recalculation. Semantically that seems better.

+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.phpundefined
@@ -0,0 +1,101 @@
+    if (!empty($breadcrumb)) {
+      return array(
+        '#theme' => 'breadcrumb',
+        '#breadcrumb' => $breadcrumb,
+      );

I think this is too early to be returning a render array. We shouldn't assume that what ever is calling the breadcrumb service will even render the breadcrumbs.

andypost’s picture

Direction is great, please add a link to sandbox to issue summary
Once plugins way is right following #1863816: Allow plugins to have services injected

Crell’s picture

I'm very much against using plugins here, as it's unnecessary. This is just Plain Old PHP Objects.

The overall direction though I think is solid, and we should push forward with it with all haste.

+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.php
@@ -0,0 +1,101 @@
+    if ($request->attributes->has('drupal_menu_item')) {
+      $item = $request->attributes->get('drupal_menu_item');
+      switch ($item['path']) {
+
+        case 'node/%':
+          $node = $item['map'][1];
+          // Load the object in case of missing wildcard loaders.
+          $node = is_object($node) ? $node : node_load($node);
+          if (_forum_node_check_node_type($node)) {
+            $breadcrumb = $this->forumPostBreadcrumb($node);
+          }
+          break;
+
+        case 'forum/%':
+          $term = $item['map'][1];
+          // Load the object in case of missing wildcard loaders.
+          $term = is_object($term) ? $term : forum_forum_load($term);
+          $breadcrumb = $this->forumTermBreadcrumb($term);
+          break;
+      }
+    }

Note this will only work on legacy routes, not on new-style routes. drupal_menu_item is only present on legacy routes.

For new routes, you would need to look at the route object on the request to get the path pattern that was used. Of course, I don't know off hand if those two routes have been converted yet... :-) I'm pretty sure forum has not.

donquixote’s picture

@splatio (#90)

I think this is too early to be returning a render array. We shouldn't assume that what ever is calling the breadcrumb service will even render the breadcrumbs.

I think I agree.
On IRC there was some discussion, it was said we need a render array "at the source" (as opposed to just in the block plugin output). Though it was not really clear to me what the "source" means.
One use case mentioned was that we might want to request the breadcrumb via AJAX. But still I don't see why this means that BreadcrumbManager or BreadcrumbBuilder need to return a render array. We can always provide a wrapper, whenever we want a render array.

The other thought:
drupal_get_breadcrumb() used to return an array of rendered links. This is ok, but not very practical if you want to do any further processing. This is an invitation for regular expressions or DOMDocument html parsing to alter those links.. I've done it myself in D7, and it always felt wrong.

Should we instead return a structured link array? How would that look like?
Or should we leave it as-is for easier conversion, and consider a change in a follow-up?
For my taste: Leave it with an array or rendered html snippets for now, unless someone has a really convincing alternative.

@Crell (#92)

I'm very much against using plugins here, as it's unnecessary.

I agree. Plugins don't offer much of a benefit here, and they also don't seem to simplify anything.

Note this will only work on legacy routes, not on new-style routes. drupal_menu_item is only present on legacy routes.

Absolutely.
We should not wait until all routes are converted to the new system. Ideally, we make one proof-of-concept implementation with the old system (forum being a good example), and another proof-of-concept based on the new system (I need to look for one). Then we should get it in, and then we should add other implementations in a follow-up.

Damien Tournoud’s picture

Status: Needs review » Needs work

As I explained above, I think this is short-sighted:

+  public function build(Request $request) {

Breadcrumbs are hierarchical by nature. Most of the case, you want to build the breadcrumb for a particular page based on the breadcrumb of its parent.

The forum post breadcrumb in the current patch is a typical example of this: it needs to be built based on the breadcrumb of its parent (the forum), which is itself a taxonomy term and should be built as we build any taxonomy term breadcrumb.

So you want an API that makes it easy to get the breadcrumb of a particular "thing". This "thing" is not a request, obviously. The closest concept we have is the pair (Route, array of route parameters) that we get out of the routing. For a node page, that's conceptually (NodeRoute, [Node object]), ie. exactly what we used to call a "translated path".

Also, about Plugins vs. Services: at the end of the day, you *always* end up wanting this kind of stuff to be discoverable and configurable via a UI. Making everything a plugin is forward thinking.

donquixote’s picture

Breadcrumbs are hierarchical by nature. Most of the case, you want to build the breadcrumb for a particular page based on the breadcrumb of its parent.

I assume you know where I come from, and that I should be the first to agree. Ideally I would have a Crumbs in core, which would do just that. But this is one step too far for D8 at this point. So atm I want something that is simple and contrib-friendly.

The forum post breadcrumb in the current patch is a typical example of this: it needs to be built based on the breadcrumb of its parent (the forum), which is itself a taxonomy term and should be built as we build any taxonomy term breadcrumb.

It is a taxonomy term, but it does not use the usual taxonomy term route. At least in the current way the forum is built, a forum taxonomy term can be viewed both at forum/% and at taxonomy/term/%. And for this breadcrumb we want forum/%.

So you want an API that makes it easy to get the breadcrumb of a particular "thing". This "thing" is not a request, obviously. The closest concept we have is the pair (Route, array of route parameters) that we get out of the routing. For a node page, that's conceptually (NodeRoute, [Node object]), ie. exactly what we used to call a "translated path".

I am not an expert about the Request object. But isn't Request just a wrapper around the route + parameters, adding some useful derived meta information? The equivalent to menu_get_item() ?
We talked about this on IRC a while ago, and it seems like Request is the only thing we have atm to fill this role. If that is not sufficient, we need something else.
(Crumbs does something similar in D7: It calls menu_get_item() in each step, so that "parent finder" plugins don't have to resolve all the wildcard loaders themselves.)

Also, about Plugins vs. Services: at the end of the day, you *always* end up wanting this kind of stuff to be discoverable and configurable via a UI. Making everything a plugin is forward thinking.

I don't really see how not having plugins would prevent us from having a UI one or another way.
What exactly do you imagine to be configurable? The weights?

Damien Tournoud’s picture

I am not an expert about the Request object. But isn't Request just a wrapper around the route + parameters, adding some useful derived meta information? The equivalent to menu_get_item() ?
We talked about this on IRC a while ago, and it seems like Request is the only thing we have atm to fill this role. If that is not sufficient, we need something else.

No, a Request is what you get from the client. It encapsulates the requested path and method, the headers and some environment-related variables. It is extended along the way with some additional metadata, but that's irrelevant here.

The question we are asking the breadcrumb builder is *not*:

Please, build me a breadcrumb for a GET request for /node/1 coming from IP 8.8.8.8, and a browser identifying itself as NCSA_Mosaic/2.0 (Windows 3.1)

Using the Request object here doesn't have the right semantic at all.

The question we are asking is:

Please, build me a breadcrumb for the full node page for the node with NID 1.

This is best represented by an up-casted route. This comes from the CMF router as (from memory) a pair (Route class, Array of route parameters). This is what we should take as an input.

tim.plunkett’s picture

Well, with our upcasting, we have a Request and Route pair. The upcasted objects are in $request->attributes->all()

Damien Tournoud’s picture

Well, with our upcasting, we have a Request and Route pair. The upcasted objects are in $request->attributes->all()

Ok, so the signature of ->build() should be $route, $attributes.

donquixote’s picture

@Damien
I agree, the client information is not relevant for the breadcrumb.
I also support passing around exactly the information that is needed, and not more.

The route + parameters should be sufficient, but it would be convenient to have the "wildcard loaders" already resolved, maybe also the access checked, so that not every breadcrumb plugin/builder has to do this by itself.
(although they might still have to do that considering other modules can temper with the routes)

EDIT: I think I get it now. I will give it a try.

donquixote’s picture

@Damien,
I did a var_dump() of $request, $request->attributes, and $request->attributes->all(), to get a better idea what you suggest.
Conclusion:

  • I agree with you, $request has too much information that we don't need. $request->attributes is the right direction.
  • Should we pass around $request->attributes, or rather $request->attributes->all()? One is a parameter bag, the other is an array. I'd say we pass the parameter bag. This bag wouldn't exist if it didn't provide some advantages..
    • I silently wonder if it would not be desirable to have a dedicated class+object for this? We don't have that atm, so we don't.
    • And if we don't have a dedicated class for this, then at least a more meaningful name than "$attributes" ?
  • The $route parameter seems redundant. It is already included in $request->attributes. So I suggest to do without that.

The SystemBreadcrumbBlock::build() would become:

    // TODO: Replace this with DI
    $builder = Drupal::service('breadcrumb');
    $request = Drupal::service('request');
    $breadcrumb = $builder->build($request->attributes);
    if (!empty($breadcrumb)) {
      // $breadcrumb is expected to be a render array.
      return array(
        '#theme' => 'breadcrumb',
        '#breadcrumb' => $breadcrumb,
      );
    }
donquixote’s picture

@Damien,
plugins vs services:
I would be interested to see a scenario you imagine where plugins (for breadcrumb building) bring a practical benefit.

Damien Tournoud’s picture

@donquixote: This direction sounds good.

About plugins vs services: plugins provide discoverability and extensible meta-data. They also can provide (optionally) the architecture for settings and admin forms. I think they are a good match for mostly anything.

donquixote’s picture

@Damien,
services with tags seem about equally practical as plugins with discovery. And they have a cleaner way of dependency injection (even after the patch from #1863816: Allow plugins to have services injected).

Settings and admin forms: I suppose the real advantage is if there is a strong similarity between different plugins and their settings?
(which is not going to be the case for breadcrumb builders, I'm afraid)

catch’s picture

donquixote’s picture

New patch:

  1. SystemBreadcrumbBlock::blockBuild() instead of SystemBreadcrumbBlock::build()
  2. BreadcrumbBuilderInterface::build() now receives a ParameterBag $attributes instead of Request $request
  3. BreadcrumbBuilderInterface::build() now returns an array of rendered links instead of a render array. The block still returns a render array.
  4. BreadcrumbBuilderInterface::build() now returns an empty array() instead of FALSE, to indicate an empty breadcrumb / no breadcrumb.
  5. BreadcrumbManager::build() now does some sanity checks on the breadcrumbs returned by registered builders. We never know what contrib will throw at us!
  6. Docblock comments changed accordingly.

TODO:

      $breadcrumb = $builder->build($attributes);
      [..]
        $class = get_class($builder);
        // TODO: Dedicated exception class.
        throw new \Exception("Invalid breadcrumb returned by $class::build().");
donquixote’s picture

Status: Needs work » Needs review

oops

donquixote’s picture

Btw, we do currently not have any module that would need a dedicated breadcrumb builder on routes that use the new routing system. The new routes are all covered by MenuLinkBreadcrumbBuilder.

(All breadcrumbs that need dedicated builders currently use drupal_set_breadcrumb())

Damien Tournoud’s picture

3. BreadcrumbBuilderInterface::build() now returns an array of rendered links instead of a render array. The block still returns a render array.

What's the reasoning for this? I would rather avoid it, because l() depends on the request (to build the full URL and add the "active" class), and I would rather not have to inject it in there. Unrendered links are in addition easier to alter, which is always desirable.

donquixote’s picture

Damien (#108)
I agree with your concern.
The reason is
- This is what D7 does (drupal_get_breadcrumb(), theme_breadcrumb(), etc).
- I did not investigate how unrendered links look like in D8.
- I don't want to break LegacyBreadcrumbBuilder
- I don't want to rewrite menu_get_active_breadcrumb() and theme_breadcrumb().

We should change that when more of the other work is done.

catch’s picture

Issue tags: +D8 cacheability
donquixote’s picture

I will be of limited availability the next weeks.
If someone wants to pick this up and re-assign, go ahead.

(doesn't mean i am totally out of the game, just want to let you know)

Crell’s picture

Status: Needs review » Needs work

Overall this looks really good. Nothing below is a fatal problem, but should get cleaned up and then we can get this committed, I think.

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbBuilderInterface.php
@@ -0,0 +1,29 @@
+   * @param \Symfony\Component\HttpFoundation\ParameterBag $attributes
+   *   Attributes representing the current page.

If we're going to pass in the attributes rather than the whole request, don't couple it to HttpFoundation at all. Use $request->attributes->all() to get back a plain old PHP array instead.

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbBuilderInterface.php
@@ -0,0 +1,29 @@
+   * @return array|FALSE|NULL
+   *   A render array for the breacrumbs, or
+   *   FALSE, to suppress breadcrumbs on this page, or
+   *   NULL, to let other builders decide.

I believe we decided to return an empty array to suppress breadcrumbs, didn't we? At least that's what the implementations below are doing.

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php
@@ -0,0 +1,107 @@
+  public function addBuilder(BreadcrumbBuilderInterface $builder, $priority) {
+    $this->builders[$priority][] = $builder;
+  }

This needs to also zero-out $sortedBuilders, so that getSortedBuilders() knows it needs to rebuild the list.

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php
@@ -0,0 +1,107 @@
+      else {
+        $class = get_class($builder);
+        // TODO: Dedicated exception class.
+        throw new \Exception("Invalid breadcrumb returned by $class::build().");
+      }

I like the error checking here. However, this should probably be \DomainException(): http://www.php.net/manual/en/class.domainexception.php

+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.php
@@ -0,0 +1,98 @@
+    $config = config('forum.settings');

This should be injected rather than calling config() directly.

+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.php
@@ -0,0 +1,98 @@
+    $vocabulary = entity_load('taxonomy_vocabulary', $config->get('vocabulary'));

The entity manager can be injected now, so we can skip calling entity_load() and just call an injected object.

+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.php
@@ -0,0 +1,98 @@
+        $breadcrumb[] = l($parent->label(), 'forum/' . $parent->tid);

This will soon belong in the generator. I'm not sure if this particular route is there yet. More just an FYI.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkBreadcrumbBuilder.php
@@ -0,0 +1,42 @@
+  /**
+   * Implements \Drupal\Breadcrumb\BreadcrumbBuilderInterface::build().
+   *
+   * @param \Symfony\Component\HttpFoundation\ParameterBag $attributes
+   *   Attributes representing the current page.
+   *
+   * @return array|NULL
+   *   An array of rendered breadcrumb links, or
+   *   an empty array(), to suppress breadcrumbs on this page, or
+   *   NULL, to let other builders decide.
+   */
+  public function build(ParameterBag $attributes) {

This can all be just {@inheritdoc}

+++ b/core/modules/system/lib/Drupal/system/Plugin/block/block/SystemBreadcrumbBlock.php
@@ -0,0 +1,45 @@
+  /**
+   * Implements \Drupal\block\BlockBase::blockBuild().
+   *
+   * @return array|NULL
+   *   A render array to display the breadcrumbs for the current page, or
+   *   NULL, if no breadcrumb block should be displayed.
+   */
+  public function blockBuild() {

{@inheritdoc}

+++ b/core/modules/system/lib/Drupal/system/Plugin/block/block/SystemBreadcrumbBlock.php
@@ -0,0 +1,45 @@
+    $breadcrumb_manager = Drupal::service('breadcrumb');

Drupal is a global class so should be referenced as \Drupal, rather than use'd. That said, can we still not inject into plugins? :-(

Thanks, donquixote! Anyone else want to drive this home while he's gone? :-)

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
20.4 KB

This is just a re-roll of #105.

kim.pepper’s picture

Status: Needs review » Needs work

The installer is stopping at:

Completed 39 of 40 - 98%
Installed Tour module.Tour module.

I've not been involved in this issue enough (besides a re-roll) to provide any insight as to the actual cause.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
20.4 KB

I noticed the re-roll above had a merge conflict in system.services.yml which used arguments: ['%container.namespaces%'] instead of arguments: ['@container.namespaces']

This is another re-roll which resolves that conflict correctly.

(Still doesn't install).

Crell’s picture

Status: Needs review » Needs work

I did a little poking here. When installing the standard profile, it always hangs on one of the late modules. Minimal profile doesn't do that, though. Not sure why.

alexpott’s picture

aspilicious’s picture

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.phpundefined
@@ -0,0 +1,107 @@
+  /*
+   * Add another breadcrumb builder.

/**
* AddS

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterBreadcrumbBuilderPass.phpundefined
@@ -0,0 +1,30 @@
+class RegisterBreadcrumbBuilderPass implements CompilerPassInterface {
+
+
+  public function process(ContainerBuilder $container) {

Remove one newline

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkBreadcrumbBuilder.phpundefined
@@ -0,0 +1,42 @@
+    // TODO: Rewrite the implementation.

Should be @todo

andypost’s picture

cleaned comments

Crell’s picture

Status: Needs review » Needs work
+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.php
@@ -0,0 +1,99 @@
+class ForumBreadcrumbBuilder implements BreadcrumbBuilderInterface {
+
+  /**
+   * Implements \Drupal\Breadcrumb\BreadcrumbBuilderInterface::build().
+   *
+   * @param \Symfony\Component\HttpFoundation\ParameterBag $attributes
+   *   Attributes representing the current page.
+   *
+   * @return array|NULL
+   *   An array of rendered breadcrumb links, or
+   *   an empty array(), to suppress breadcrumbs on this page, or
+   *   NULL, to let other builders decide.

This should just be an {@inheritdoc}

+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.php
@@ -0,0 +1,99 @@
+    $config = config('forum.settings');

Inject the config manager so we don't need to call a function.

+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.php
@@ -0,0 +1,99 @@
+    $vocabulary = entity_load('taxonomy_vocabulary', $config->get('vocabulary'));

Inject the entity manager so that we don't need to call a function.

+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.php
@@ -0,0 +1,99 @@
+    $config = config('forum.settings');
+    $vocabulary = entity_load('taxonomy_vocabulary', $config->get('vocabulary'));

As above. :-)

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkBreadcrumbBuilder.php
@@ -0,0 +1,43 @@
+class MenuLinkBreadcrumbBuilder implements BreadcrumbBuilderInterface {
+
+  /**
+   * Implements \Drupal\Breadcrumb\BreadcrumbBuilderInterface::build().
+   *
+   * @param \Symfony\Component\HttpFoundation\ParameterBag $attributes
+   *   Attributes representing the current page.
+   *
+   * @return array|NULL
+   *   An array of rendered breadcrumb links, or
+   *   an empty array(), to suppress breadcrumbs on this page, or
+   *   NULL, to let other builders decide.

{@inheritdoc}

+++ b/core/modules/system/lib/Drupal/system/LegacyBreadcrumbBuilder.php
@@ -0,0 +1,46 @@
+/**
+ * Class to define the legacy breadcrumb builder.
+ *
+ * This breadcrumb builder implements legacy support for the
+ * drupal_set_breadcrumb() mechanic.
+ * Remove this once drupal_set_breadcrumb() has been eliminated.
+ */
+class LegacyBreadcrumbBuilder implements BreadcrumbBuilderInterface {

Can we mark this @deprecated right off the bat?

In fact, we should then mark drupal_set_breadcrumb() deprecated, too.

+++ b/core/modules/system/lib/Drupal/system/LegacyBreadcrumbBuilder.php
@@ -0,0 +1,46 @@
+  /**
+   * Implements \Drupal\Breadcrumb\BreadcrumbBuilderInterface::build().
+   *
+   * @param \Symfony\Component\HttpFoundation\ParameterBag $attributes
+   *   Attributes representing the current page.
+   *
+   * @return array|NULL
+   *   An array of rendered breadcrumb links, or
+   *   an empty array(), to suppress breadcrumbs on this page, or
+   *   NULL, to let other builders decide.

{@inheritdoc}

+++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBreadcrumbBlock.php
@@ -0,0 +1,45 @@
+    $breadcrumb_manager = \Drupal::service('breadcrumb');
+    $request = \Drupal::service('request');

Can't we inject these now?

And shouldn't the request be available via a method parameter or something? Not sure. I'll ask Kris.

dawehner’s picture

Status: Needs work » Needs review

Just some feedback.

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.phpundefined
@@ -0,0 +1,108 @@
+      else {
+        $class = get_class($builder);
+        // TODO: Dedicated exception class.
+        throw new \Exception("Invalid breadcrumb returned by $class::build().");
+      }

Just wondering whether this is more babysitting of broken code.

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterBreadcrumbBuilderPass.phpundefined
@@ -0,0 +1,30 @@
+  public function process(ContainerBuilder $container) {

Needs docs

+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.phpundefined
@@ -0,0 +1,99 @@
+class ForumBreadcrumbBuilder implements BreadcrumbBuilderInterface {

In general I'm wondering whether it's okay to load all forum breadcrumb builders on each request. It seems to be better to not instanstiate all the time bu t use the applies pattern similar to access checkers. This would require operations while compiling the routes.

+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.phpundefined
@@ -0,0 +1,99 @@
+  /**
+   * Implements \Drupal\Breadcrumb\BreadcrumbBuilderInterface::build().
+   *
+   * @param \Symfony\Component\HttpFoundation\ParameterBag $attributes
+   *   Attributes representing the current page.
+   *
+   * @return array|NULL
+   *   An array of rendered breadcrumb links, or
+   *   an empty array(), to suppress breadcrumbs on this page, or
+   *   NULL, to let other builders decide.

Could be just {@inheritdoc}

+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.phpundefined
@@ -0,0 +1,99 @@
+    if ($attributes->has('drupal_menu_item')) {
+      $item = $attributes->get('drupal_menu_item');
+      switch ($item['path']) {

just wondering whether you could also use system_path

+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.phpundefined
@@ -0,0 +1,99 @@
+   * Builds the breadcrumb for a forum post page.
...
+  protected function forumPostBreadcrumb($node) {

Needs docs

+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.phpundefined
@@ -0,0 +1,99 @@
+    $config = config('forum.settings');
+    $vocabulary = entity_load('taxonomy_vocabulary', $config->get('vocabulary'));
...
+    if ($parents = taxonomy_term_load_parents_all($node->forum_tid)) {
...
+    $config = config('forum.settings');
+    $vocabulary = entity_load('taxonomy_vocabulary', $config->get('vocabulary'));

Can we inject dependencies into the object?

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkBreadcrumbBuilder.phpundefined
@@ -0,0 +1,43 @@
+  /**
+   * Implements \Drupal\Breadcrumb\BreadcrumbBuilderInterface::build().
+   *
+   * @param \Symfony\Component\HttpFoundation\ParameterBag $attributes
+   *   Attributes representing the current page.
+   *
+   * @return array|NULL
+   *   An array of rendered breadcrumb links, or
+   *   an empty array(), to suppress breadcrumbs on this page, or
+   *   NULL, to let other builders decide.

@inheritdoc

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkBreadcrumbBuilder.phpundefined
@@ -0,0 +1,43 @@
+    // @todo Rewrite the implementation.

I guess the todo will stay in?

Crell’s picture

Just wondering whether this is more babysitting of broken code.

That sort of babysitting is a good thing, because it makes debugging easier. The bug that Alex tracked down above was the block API having changed, but the installer having zero ability to tell us that. He had to just sort of "know" that the API changed a bit. That's bad. :-)

"Don't babysit broken code" is another Drupalism that we should stamp out. It's the wrong approach. We're not doing that anymore.

As for access check-style conditional application, I'm inclined to punt on that for now. If benchmarking tells us we need it, we can add it later.

donquixote’s picture

In general I'm wondering whether it's okay to load all forum breadcrumb builders on each request. It seems to be better to not instanstiate all the time bu t use the applies pattern similar to access checkers. This would require operations while compiling the routes.

As for access check-style conditional application, I'm inclined to punt on that for now. If benchmarking tells us we need it, we can add it later.

From my experience, this would not get us very far.
Most of the requests on a typical site (at least on D7) are some kind of node or another entity. And most of the plugins are for nodes or entities from a specific bundle, or having specific fields. Knowing that from the route alone is quite hard, or requires almost as much information as the breadcrumb builder itself.

What might actually help us is this: #1973618: DIC: Lazy instantiation of service dependencies (ProxyManager for "proxy services")
(elsewhere this is called "proxy objects")
But then I would not proxy the breadcrumb builders themselves, but some of their dependencies.

EDIT: I am too bold with my statement.
I think we actually can get some benefit from pre-filtering based on the routes, saving us both the instantiation and the conditional code of some builders on some requests. We should not expect too much, but the benefit will be measurable.

andypost’s picture

Clean-up @inheritdoc and typo

Crell’s picture

FileSize
8.79 KB
20.97 KB

Attached patch:

- Fixes more docs.
- Injects everything we can reasonably inject right now.
- Switches to passing an array of attributes rather than the ParameterBag, thus decoupling the breadcrumb system from HttpFoundation.
- Marks as @deprecated that which should be deprecated.
- Adds a @todo to fix forum breadcrumbs, as right now that code only works with the old routing system. As those routes are converted to the new router that code will need to be updated, but we can't do that now without random guessing.

Let's see how much I broke. :-)

tim.plunkett’s picture

+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.phpundefined
@@ -7,21 +7,51 @@
+   * @var ;Drupal\Core\Config\Config

should be \ not ;

+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.phpundefined
@@ -7,21 +7,51 @@
+    // @todo This only works for legacy routes. Once node/% and forum/% are
+    // converted to the new router this code will need to be updated.

THe following lines of @todos should be indented two spaces

+++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBreadcrumbBlock.phpundefined
@@ -28,7 +28,7 @@ class SystemBreadcrumbBlock extends BlockBase {
-    $breadcrumb = $breadcrumb_manager->build($request->attributes);
+    $breadcrumb = $breadcrumb_manager->build($request->attributes->all());

+1

donquixote’s picture

#127

Switches to passing an array of attributes rather than the ParameterBag, thus decoupling the breadcrumb system from HttpFoundation.

The reason I went with ParameterBag was that I saw it as a generic convenience wrapper around array(). If you look at it, you can wonder why it is a HttpFoundation specifity and not a generic Sf2 core feature.
Anyway, drop it.

----------------

This aside:
There has been a discussion between Damien and me in #108 / #109, why we operate with rendered links instead of some kind of array to define the link.
I think I have said all there is to say from my side, but I would still like to see a 3rd opinion.

andypost’s picture

Yep, so cleanup parameterBag from uses and fixes #128

Crell’s picture

I'd love to change the data structure we're returning, but I think that's a follow-up. Let's get the basic structure in first.

I think #130 addresses everything that's left in #128, so... are we done? Someone RTBC this. :-)

tim.plunkett’s picture

+++ b/core/includes/common.incundefined
@@ -283,6 +283,11 @@ function drupal_get_profile() {
+ * @deprecated This will be removed in 8.0.  Instead, register a new breadcrumb

@@ -295,6 +300,11 @@ function drupal_set_breadcrumb($breadcrumb = NULL) {
+ * @deprecated This will be removed in 8.0.  Instead, register a new breadcrumb

+++ b/core/modules/system/lib/Drupal/system/LegacyBreadcrumbBuilder.phpundefined
@@ -0,0 +1,42 @@
+ * @deprecated This will be removed in 8.0.  Instead, register a new breadcrumb

Double space after .

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbBuilderInterface.phpundefined
@@ -0,0 +1,28 @@
+   * Build the breadcrumb.

Builds

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbBuilderInterface.phpundefined
@@ -0,0 +1,28 @@
+   * @return array|FALSE|NULL

array|false|null in this case. Don't ask me why.

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbBuilderInterface.phpundefined
@@ -0,0 +1,28 @@
+   *   A render array for the breadcrumbs, or
+   *   FALSE, to suppress breadcrumbs on this page, or
+   *   NULL, to let other builders decide.

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.phpundefined
@@ -0,0 +1,98 @@
+   *   An array of breadcrumb builders sorted by priorities, or
+   *   NULL, if the array needs to be re-calculated.
...
+    // Call the build method of registered breadcrumb builders,
+    // until one of them returns something other than NULL.

This can be re-wrapped.

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.phpundefined
@@ -0,0 +1,98 @@
+ * Breadcrumb manager.

Provides

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.phpundefined
@@ -0,0 +1,98 @@
+   *   An array of breadcrumb builders sorted by priorities.
...
+   *   An array of breadcrumb builders sorted by priorities, or
+   *   NULL, if the array needs to be re-calculated.

I don't think we generally nest anything below @var

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.phpundefined
@@ -0,0 +1,98 @@
+   * @param \Drupal\Core\Breadcrumb\BreadcrumbBuilder $builder
...
+  public function addBuilder(BreadcrumbBuilderInterface $builder, $priority) {

mismatch

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.phpundefined
@@ -0,0 +1,98 @@
+        // TODO: Dedicated exception class.

@todo, no colon

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.phpundefined
@@ -0,0 +1,98 @@
+        throw new \Exception("Invalid breadcrumb returned by $class::build().");

Why not just do this now?

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterBreadcrumbBuilderPass.phpundefined
@@ -0,0 +1,30 @@
+  public function process(ContainerBuilder $container) {

Missing {@inheritdoc}

+++ b/core/modules/system/lib/Drupal/system/LegacyBreadcrumbBuilder.phpundefined
@@ -0,0 +1,42 @@
+ * @see Drupal\Core\Breadcrumb\BreadcrumbBuilderInterface

Missing leading \

+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/MenuTestBase.phpundefined
@@ -34,13 +34,30 @@ protected function assertBreadcrumb($goto, array $trail, $page_title = NULL, arr
+  protected function assertBreadcrumbParts($trail) {

@@ -49,14 +66,11 @@ protected function assertBreadcrumb($goto, array $trail, $page_title = NULL, arr
+  protected function assertMenuActiveTrail($tree, $last_active) {

Missing docblocks

+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/MenuTestBase.phpundefined
@@ -49,14 +66,11 @@ protected function assertBreadcrumb($goto, array $trail, $page_title = NULL, arr
+    // Indentation to reduce git diff.

Delete this?

+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/MenuTestBase.phpundefined
@@ -49,14 +66,11 @@ protected function assertBreadcrumb($goto, array $trail, $page_title = NULL, arr
+    if (TRUE) {

If TRUE?!

andypost’s picture

// TODO: Dedicated exception class.

Suppose \UnexpectedValueException should be enough here

andypost’s picture

Previous patch was wrong

tim.plunkett’s picture

#134: 1947536-breadcrumbs-133.patch queued for re-testing.

tim.plunkett’s picture

FileSize
2.88 KB
25.78 KB
16.76 KB
30.97 KB

Okay, so this works great, except we have no where to put the new breadcrumb block.
That should be left to #507488: Convert page elements (local tasks, actions) into blocks to handle.

Otherwise, it looks like this:
seven.png
bartik.png

So I've disabled the blocks for now, and also converted the existing bit in template_process_page().

Otherwise, I think this is done.

andypost’s picture

Status: Needs review » Needs work
FileSize
1.26 KB

only ForumTest still broken - working on it

andypost’s picture

Status: Needs work » Needs review
FileSize
25.84 KB
693 bytes

not forum tests should be fine

andypost’s picture

And another couple of small nitpicks

Also added debug to test new crumbs on forum pages and patch to remove old implementation for forum node and term

EDIT

Debug:
'node/%'
in forum_node_view() (line 273 of core/modules/forum/forum.module).
Debug:
'builder node/%'
in Drupal\forum\ForumBreadcrumbBuilder->build() (line 57 of core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.php).
tstoeckler’s picture

Status: Needs review » Needs work
+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkBreadcrumbBuilder.php
@@ -0,0 +1,34 @@
+    elseif (FALSE === $breadcrumb) {
+      // A value of FALSE indicates that no breadcrumb should be displayed.
+      // BreadcrumbBuilderInterface dictates to return array() in this case.
+      return array();

+++ b/core/modules/system/lib/Drupal/system/LegacyBreadcrumbBuilder.php
@@ -0,0 +1,42 @@
+    elseif (FALSE === $breadcrumb) {
+      // A value of FALSE indicates that no breadcrumb should be displayed.
+      // BreadcrumbBuilderInterface dictates to return array() in this case.
+      return array();

Doesn't the breadcrumb manager contain the same check? It seems per the interface, we should simply return NULL here, not an empty array, in order for the other builders to be applied.
I don't know how that currently works. Either way, the comment should be updated.

andypost’s picture

@tstoeckler++ Good point, once any of services returns array (no matter empty or not) manager will return chain immediately

Currently menu_get_active_breadcrumb() always returns array so executes last.
Legacy builder for other (5) drupal_set_breadcrumbs()
And then highest priority for module specific builders

+++ b/core/modules/forum/forum.services.ymlundefined
@@ -0,0 +1,6 @@
+  forum.breadcrumb:
...
+      - { name: breadcrumb_builder, priority: 1001 }

+++ b/core/modules/menu_link/menu_link.services.ymlundefined
@@ -0,0 +1,5 @@
+  menu_link.breadcrumb:
...
+      - { name: breadcrumb_builder, priority: 0 }

+++ b/core/modules/system/system.services.ymlundefined
@@ -6,3 +6,7 @@ services:
+  system.breadcrumb.legacy:
...
+      - {name: breadcrumb_builder, priority: 500}

the ordering is right: modules registers their services in container, once one of them returns array this points to builder to stop

tstoeckler’s picture

Status: Needs work » Needs review
+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbBuilderInterface.php
@@ -0,0 +1,26 @@
+   *   A render array for the breadcrumbs or NULL to let other builders decide.

I guess it would be nice to mention that you can pass an empty array() to suppress all breadcrumbs.

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php
@@ -0,0 +1,91 @@
+      foreach ($this->builders as $builders) {

The code is completely correct, but I'm wondering if we can't find better variable names. One is used to seeing things like foreach ($this->builders as $builder) but here both are called $builders. Hmm...

tstoeckler’s picture

Issue summary: View changes

Updated issue summary.

donquixote’s picture

Issue summary: View changes

Summary was horribly outdated:
- Breadcrumb service works as a builder, no more global set() and get().
- Different modules can provide breadcrumb builders.
- Explain strategy of core vs contrib.

donquixote’s picture

I updated the issue summary.
I hope it is a balanced representation of the current direction we are taking.

Crell’s picture

andypost’s picture

Merged HEAD and addressed #144

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Let's get a move on then. :-)

tstoeckler’s picture

Well now $builder is actually an array of builders :-), but I haven't thought of any better suggestions either, so...

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Minor nit but the docs seem to be wrong...

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.phpundefined
@@ -0,0 +1,91 @@
+  /**
+   * Holds the array of breadcrumb builders, sorted by priority.
+   *
+   * @var array
+   *   An array of breadcrumb builders sorted by priorities.
+   */
+  protected $builders = array();

This is not sorted by priority it's keyed by priotity... Also I think this should be...

  /**
   * Holds arrays of breadcrumb builders, keyed by priority.
   *
   * @var array
   */
  protected $builders = array();

No need for the repetition... same for the variable doc block for $sortedBuilders too...

+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.phpundefined
@@ -0,0 +1,91 @@
+   *   An array of breadcrumb builders sorted by priorities, or
+   *   NULL, if the array needs to be re-calculated.

It's never re-calculated... it's only ever constructed once...

Also there is a danger of a PHP warning here if there are no builders since the default value of sorterBuilders is NULL but it's used in a foreach loop. Perhaps something like what's below is better?

class BreadcrumbManager implements BreadcrumbBuilderInterface {

  /**
   * Holds the array of breadcrumb builders, keyed by priority.
   *
   * @var array
   */
  protected $builders = array();

  /**
   * Holds the array of breadcrumb builders sorted by priority.
   *
   * @var array
   */
  protected $sortedBuilders = array();

  /**
   * Adds another breadcrumb builder.
   *
   * @param \Drupal\Core\Breadcrumb\BreadcrumbBuilderInterface $builder
   *   The breadcrumb builder to add.
   * @param int $priority
   *   Priority of the breadcrumb builder.
   */
  public function addBuilder(BreadcrumbBuilderInterface $builder, $priority) {
    $this->builders[$priority][] = $builder;
    // Force the builders to be re-sorted.
    $this->sortedBuilders = array();
  }

  /**
   * {@inheritdoc}
   */
  public function build(array $attributes) {
    // Call the build method of registered breadcrumb builders,
    // until one of them returns array.
    foreach ($this->getSortedBuilders() as $builder) {
      $breadcrumb = $builder->build($attributes);
      if (!isset($breadcrumb)) {
        // The builder returned NULL, so we continue with the other builders.
        continue;
      }
      elseif (is_array($breadcrumb)) {
        // The builder returned an array of breadcrumb links.
        return $breadcrumb;
      }
      else {
        throw new \UnexpectedValueException(format_string('Invalid breadcrumb returned by !class::build().', array('!class' => get_class($builder))));
      }
    }

    // Fall back to an empty breadcrumb.
    return array();
  }

  /**
   * Returns the sorted array of breadcrumb builders.
   *
   * @return array
   *   An array of breadcrumb builder objects.
   */
  protected function getSortedBuilders() {
    if (empty($this->sortedBuilders) && !empty($this->builders)) {
      // Sort the builders according to priority.
      krsort($this->builders);
      // Merge the nested $this->builders array into $this->sortedBuilders.
      $this->sortedBuilders = array();
      foreach ($this->builders as $builder) {
        $this->sortedBuilders = array_merge($this->sortedBuilders, $builder);
      }
    }
    return $this->sortedBuilders;
  }

}
andypost’s picture

Status: Needs work » Needs review
FileSize
27.29 KB
2.33 KB

@alexpott I think NULL makes sense here so changes docs.

Also $builders returned back, because this is a array and I dont think $priority => $builders_array is good

andypost’s picture

Crell’s picture

Status: Needs review » Reviewed & tested by the community

The changes in the last 2 patches look fine to me. Back to RTBC unless the bot objects.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

NULL can not make sense here because we use the result in a foreach loop without checking if it's an array...

Yes we should always have a builder because the system module has one but this is just smelly to me.

andypost’s picture

Status: Needs work » Needs review
+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.phpundefined
@@ -0,0 +1,92 @@
+  public function build(array $attributes) {
...
+    foreach ($this->getSortedBuilders() as $builder) {
...
+  protected function getSortedBuilders() {
+    if (!isset($this->sortedBuilders)) {
...
+      $this->sortedBuilders = array();

As you see $sortedBuilders is always array and always initialized

andypost’s picture

Status: Needs review » Reviewed & tested by the community

In case we define default value to empty array it makes getSortedBuilders() to run everytime it's called, so this function will require some 'flag/reset' to differentiate init/empty/build states

Crell’s picture

Alex: Andy is correct. As long as you always access sortedBuilders through getSortedBuilders(), which this class does, the foreach() is safe. This is a common pattern we're doing in a bunch of places now. If you don't feel it's robust enough that should be a follow-up to address inbound path processors, outbound path processors, param converters, breadcrumbs, and a few other services I am sure I' forgetting. We're good here.

donquixote’s picture

This might feel awkward, but one option would be to name the protected variable as "$sortedBuildersOrNull".

class BreadcrumbBuilder {
  protected $sortedBuildersOrNull;
  function dontDoThis() {
    // The variable name indicates that you are not supposed to do this.
    foreach ($this->sortedBuildersOrNull as $builder) {}
  }
  function doThisInstead() {
    // The method name indicates that this is safe
    foreach ($this->getSortedBuilders() as $builder) {}
  }
}
Crell’s picture

don: No offense, but that's a terrible idea. :-) We're OK here, or at least as OK as we are everywhere else in core.

alexpott’s picture

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review

#154: 1947536-breadcrumbs-154.patch queued for re-testing.

Crell’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +MenuSystemRevamp, +WSCCI, +Needs change record, +Blocks-Layouts, +D8 cacheability

Bot goof.

alexpott’s picture

Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

Committed 4d08d57 and pushed to 8.x. Thanks!

Crell’s picture

Title: Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service » Change notice: Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service

WIN!

This definitely needs a change notice...

jibran’s picture

Issue tags: +Needs followup

This issue needs followups I can create if someone agrees to me.

+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.phpundefined
@@ -0,0 +1,121 @@
+          $node = $item['map'][1];
...
+          $term = $item['map'][1];

This one.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkBreadcrumbBuilder.phpundefined
@@ -0,0 +1,26 @@
+    return menu_get_active_breadcrumb();

This one.

+++ b/core/modules/system/lib/Drupal/system/LegacyBreadcrumbBuilder.phpundefined
@@ -0,0 +1,37 @@
+ * Remove this once drupal_set_breadcrumb() has been eliminated.

And this one.

andypost’s picture

Agreed! and follow-up to convert other dsb()...
EDIT first there should be change notice

Crell’s picture

Title: Change notice: Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service » Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record

Change noticed added: https://drupal.org/node/2026025

Yes, let's open up a follow ups.

jibran’s picture

jibran’s picture

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Remove a "-" in issue summary.