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.
Comment | File | Size | Author |
---|---|---|---|
#154 | interdiff.txt | 711 bytes | andypost |
#154 | 1947536-breadcrumbs-154.patch | 27.28 KB | andypost |
#152 | interdiff.txt | 2.33 KB | andypost |
#152 | 1947536-breadcrumbs-152.patch | 27.29 KB | andypost |
#148 | interdiff.txt | 1.31 KB | andypost |
Comments
Comment #1
larowlanHere's the service and drop of drupal_[s,g]et_breadcrumb
Comment #2
amateescu CreditAttribution: amateescu commentedIf 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.
Comment #3
larowlanYeah agreed, thought the same thing as I was writing it
Comment #5
andypost@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++
Comment #6
podarokdue to
this one needs change notification after commit
Comment #6.0
podarokUpdated issue summary.
Comment #6.1
podarokUpdated issue summary.
Comment #7
donquixote CreditAttribution: donquixote commentedyes, agree.
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 wasdrupal_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.
Comment #8
dawehnerOh 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?
Comment #9
larowlanSo 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
Comment #10
podaroknitpick
arroy
Comment #11
andypostSuppose better to implement this as renderable
s/arroy/array
When you appeding, better to point the side (start or end of array)
Comment #12
larowlanAddresses #10 and #11
Comment #12.0
larowlancode style
Comment #13
larowlanUpdated issue summary
Comment #14
dawehnerI'm wondering whether we should use UnitTestCase here, when menu_get_active_breadcrumb() is actually not runnable in the test.
Comment #16
msonnabaum CreditAttribution: msonnabaum commentedI 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.
Comment #17
msonnabaum CreditAttribution: msonnabaum commentedComment #18
andypostIt 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
Comment #19
larowlanremoved mention of converting to block as that is handled in #507488: Convert page elements (local tasks, actions) into blocks
Updated remaining tasks
Comment #19.0
larowlanUpdated issue summary.
Comment #20
larowlanAdds @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.
Comment #22
andypost#20: breadcrumbs-1947536.20.patch queued for re-testing.
Comment #23
larowlanGreen 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
Comment #24
andypostPatch 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 usetheme_item_list()
because holding a html-pieces is wrong here.As side note: maybe breadcrumbs better relates to request then page level
prefix is not needed for Drupal
Comment #25
amateescu CreditAttribution: amateescu commentedIt *is* needed if you're in a namespaced file, so the code above is correct.
Comment #26
larowlanIs that in scope here?
Comment #27
donquixote CreditAttribution: donquixote commentedQuick 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.
Comment #28
Crell CreditAttribution: Crell commentedAgreed 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.
Comment #29
donquixote CreditAttribution: donquixote commented#28 (Crell)
Sounds fine. But I'd like to add a thought.
We have two situations to take care of:
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.
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?
Comment #30
donquixote CreditAttribution: donquixote commentedProposed 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.
Comment #31
Crell CreditAttribution: Crell commented@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.)
Comment #32
donquixote CreditAttribution: donquixote commentedWhat 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.
Comment #34
Crell CreditAttribution: Crell commentedI 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.
Comment #35
donquixote CreditAttribution: donquixote commentedYes.
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.
Comment #36
bojanz CreditAttribution: bojanz commentedI 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.
Comment #37
Crell CreditAttribution: Crell commentedGreat, so let's top talking and get to writing. :-) Who wants it tackle it?
Comment #38
donquixote CreditAttribution: donquixote commentedSome 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".
Comment #39
Crell CreditAttribution: Crell commented1) 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.)
Comment #40
Damien Tournoud CreditAttribution: Damien Tournoud commentedI 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.
Comment #41
donquixote CreditAttribution: donquixote commentedI will try to find some time for this this week. If anyone thinks to be faster than that, feel free to reassign :)
Comment #42
msmithcti CreditAttribution: msmithcti commentedDon'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.
Comment #43
donquixote CreditAttribution: donquixote commentedThat's a good start, thanks!
Comment #44
msmithcti CreditAttribution: msmithcti commentedGreat! 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.
Comment #45
donquixote CreditAttribution: donquixote commentedAnd 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 :)
Comment #46
donquixote CreditAttribution: donquixote commented@splatio: so *now* we are toe-stepping :)
I am going to have a look how different we are.
Comment #47
jibranI think according to #1939660: Use YAML as the primary means for service registration we have to change the above to menu_link.services.yml
Comment #48
donquixote CreditAttribution: donquixote commented@splatio: i am doing some merging
Comment #49
donquixote CreditAttribution: donquixote commentedCompetition 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.
Comment #50
donquixote CreditAttribution: donquixote commented@splatio, #44 compared to #42 and #45:
+++ b/core/core.services.yml
I trust you on this one :)
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.
We should respect the signature of BreadcrumbBuilderInterface::build() ! It always takes a request object.
And we should not fire it more than once.
Comment #51
donquixote CreditAttribution: donquixote commentedShould we try to get this in, and then do the rest in a follow-up patch?
It would make the patches more readable..
Comment #53
donquixote CreditAttribution: donquixote commented#49: drupal-8.x-breadcrumb-as-service-1947536-49-favouring-donquixote.patch queued for re-testing.
Comment #54
donquixote CreditAttribution: donquixote commented#49: drupal-8.x-breadcrumb-as-service-1947536-49-favouring-splatio.patch queued for re-testing.
Comment #55
donquixote CreditAttribution: donquixote commented#45: drupal-8.x-breadcrumb-as-service-1947536-45.patch queued for re-testing.
Comment #56
msmithcti CreditAttribution: msmithcti commentedGood 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! :)
Comment #57
donquixote CreditAttribution: donquixote commentedThe 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)
Comment #58
donquixote CreditAttribution: donquixote commentedWe 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().
Comment #59
donquixote CreditAttribution: donquixote commentedAnother 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().
Comment #60
donquixote CreditAttribution: donquixote commentedDoes this make the request a constructor argument? Why?
Comment #61
donquixote CreditAttribution: donquixote commentedHow can we suppress the block title? I get a title that says "System Breadcrumb".
Comment #62
donquixote CreditAttribution: donquixote commentedAnother 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.Comment #63
donquixote CreditAttribution: donquixote commentedBranch on WSCCI: http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/8.x-...
Comment #64
donquixote CreditAttribution: donquixote commentedObviously this needs a follow-up as soon as this one finishes:
#1863816: Allow plugins to have services injected
Comment #65
donquixote CreditAttribution: donquixote commentedThe 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.
Comment #66
donquixote CreditAttribution: donquixote commentedCurrently 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 :)
Comment #67
tim.plunkettPlacing 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.
Comment #68
donquixote CreditAttribution: donquixote commentedtimplunkett, 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
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.
Comment #69
donquixote CreditAttribution: donquixote commentedEclipseGc 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.
Comment #70
Crell CreditAttribution: Crell commentedAs discussed, this should change to just array|NULL, and specify the significance of an empty array.
Why not use empty array for needs-resort? (I forget off hand what path processors do.)
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.
Do we need to merge, or just overwrite?
This should be done in a YAML file now.
And donquixote is correct, the request should not be a constructor dependency.
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."
Comment #71
Crell CreditAttribution: Crell commentedNo, 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.
Comment #72
donquixote CreditAttribution: donquixote commented@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)
Comment #73
donquixote CreditAttribution: donquixote commented@Crell (#70)
(assuming all comments are refering to #62)
What is the render result of an empty render array?
Empty array means there are no builders.
NULL means the sort never happened.
Ooops. I forgot to remove that krsort(). There is already getSortedBuilders() which does the job.
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.
Absolutely.
Fine.
Comment #74
donquixote CreditAttribution: donquixote commentedDuh, the patch #72 contains some junk (core/lib/Drupal/Core/Breadcrumb/BreadcrumbPluginManager.php)
Comment #76
Crell CreditAttribution: Crell commentedYes, that was re #62.
Comment #77
donquixote CreditAttribution: donquixote commentedOne with DIC, one with plugins.
Comment #78
donquixote CreditAttribution: donquixote commented@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)
Comment #79
donquixote CreditAttribution: donquixote commented#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.
Comment #81
donquixote CreditAttribution: donquixote commentedThe 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()
Either we have to change the test to allow more than one breadcrumb.
Or we have to remove the other breadcrumb.
Comment #82
donquixote CreditAttribution: donquixote commentedAdding 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.
Comment #83
EclipseGc CreditAttribution: EclipseGc commenteddidn't test.
Comment #84
donquixote CreditAttribution: donquixote commentedSome rough changes to the test to make it pass with multiple breadcrumbs.
Comment #85
Crell CreditAttribution: Crell commentedIs 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.)
Comment #86
donquixote CreditAttribution: donquixote commented#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.)
Comment #87
andypostThis one needs better doc-block to show example and point to implementations
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
Comment #87.0
andypostUpdated issue summary.
Comment #88
andypostProbably 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
Comment #89
donquixote CreditAttribution: donquixote commentedDo you guys agree with the overall architectural direction this is taking?
Comment #90
msmithcti CreditAttribution: msmithcti commentedI'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:
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.
I like that this is set to NULL for recalculation. Semantically that seems better.
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.
Comment #91
andypostDirection is great, please add a link to sandbox to issue summary
Once plugins way is right following #1863816: Allow plugins to have services injected
Comment #92
Crell CreditAttribution: Crell commentedI'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.
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.
Comment #93
donquixote CreditAttribution: donquixote commented@splatio (#90)
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 agree. Plugins don't offer much of a benefit here, and they also don't seem to simplify anything.
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.
Comment #94
Damien Tournoud CreditAttribution: Damien Tournoud commentedAs I explained above, I think this is short-sighted:
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.
Comment #95
donquixote CreditAttribution: donquixote commentedI 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.
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/%.
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.)
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?
Comment #96
Damien Tournoud CreditAttribution: Damien Tournoud commentedNo, 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*:
Using the
Request
object here doesn't have the right semantic at all.The question we are asking is:
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.
Comment #97
tim.plunkettWell, with our upcasting, we have a Request and Route pair. The upcasted objects are in $request->attributes->all()
Comment #98
Damien Tournoud CreditAttribution: Damien Tournoud commentedOk, so the signature of
->build()
should be$route, $attributes
.Comment #99
donquixote CreditAttribution: donquixote commented@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.
Comment #100
donquixote CreditAttribution: donquixote commented@Damien,
I did a var_dump() of $request, $request->attributes, and $request->attributes->all(), to get a better idea what you suggest.
Conclusion:
The SystemBreadcrumbBlock::build() would become:
Comment #101
donquixote CreditAttribution: donquixote commented@Damien,
plugins vs services:
I would be interested to see a scenario you imagine where plugins (for breadcrumb building) bring a practical benefit.
Comment #102
Damien Tournoud CreditAttribution: Damien Tournoud commented@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.
Comment #103
donquixote CreditAttribution: donquixote commented@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)
Comment #104
catchAdded to #1839338: [meta] Remove drupal_set_*() drupal_add_*() in favour of #attached/response for out-of-band stuff.
Comment #105
donquixote CreditAttribution: donquixote commentedNew patch:
SystemBreadcrumbBlock::blockBuild()
instead ofSystemBreadcrumbBlock::build()
BreadcrumbBuilderInterface::build()
now receives aParameterBag $attributes
instead ofRequest $request
TODO:
Comment #106
donquixote CreditAttribution: donquixote commentedoops
Comment #107
donquixote CreditAttribution: donquixote commentedBtw, 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())
Comment #108
Damien Tournoud CreditAttribution: Damien Tournoud commentedWhat'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.Comment #109
donquixote CreditAttribution: donquixote commentedDamien (#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.
Comment #110
catchComment #111
donquixote CreditAttribution: donquixote commentedI 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)
Comment #112
Crell CreditAttribution: Crell commentedOverall this looks really good. Nothing below is a fatal problem, but should get cleaned up and then we can get this committed, I think.
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.
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.
This needs to also zero-out $sortedBuilders, so that getSortedBuilders() knows it needs to rebuild the list.
I like the error checking here. However, this should probably be \DomainException(): http://www.php.net/manual/en/class.domainexception.php
This should be injected rather than calling config() directly.
The entity manager can be injected now, so we can skip calling entity_load() and just call an injected object.
This will soon belong in the generator. I'm not sure if this particular route is there yet. More just an FYI.
This can all be just {@inheritdoc}
{@inheritdoc}
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? :-)
Comment #113
kim.pepperThis is just a re-roll of #105.
Comment #115
kim.pepperThe installer is stopping at:
I've not been involved in this issue enough (besides a re-roll) to provide any insight as to the actual cause.
Comment #116
kim.pepperI noticed the re-roll above had a merge conflict in
system.services.yml
which used arguments:['%container.namespaces%']
instead ofarguments: ['@container.namespaces']
This is another re-roll which resolves that conflict correctly.
(Still doesn't install).
Comment #118
Crell CreditAttribution: Crell commentedI 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.
Comment #119
alexpottThe block plugins have moved #1987298: Shorten directory structure and PSR-0 namespacing for plugins and have been re-factored #1927608: Remove the tight coupling between Block Plugins and Block Entities
Comment #120
aspilicious CreditAttribution: aspilicious commented/**
* AddS
Remove one newline
Should be @todo
Comment #121
andypostcleaned comments
Comment #122
Crell CreditAttribution: Crell commentedThis should just be an {@inheritdoc}
Inject the config manager so we don't need to call a function.
Inject the entity manager so that we don't need to call a function.
As above. :-)
{@inheritdoc}
Can we mark this @deprecated right off the bat?
In fact, we should then mark drupal_set_breadcrumb() deprecated, too.
{@inheritdoc}
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.
Comment #123
dawehnerJust some feedback.
Just wondering whether this is more babysitting of broken code.
Needs docs
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.
Could be just {@inheritdoc}
just wondering whether you could also use system_path
Needs docs
Can we inject dependencies into the object?
@inheritdoc
I guess the todo will stay in?
Comment #124
Crell CreditAttribution: Crell commentedThat 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.
Comment #125
donquixote CreditAttribution: donquixote commentedFrom 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.
Comment #126
andypostClean-up @inheritdoc and typo
Comment #127
Crell CreditAttribution: Crell commentedAttached 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. :-)
Comment #128
tim.plunkettshould be \ not ;
THe following lines of @todos should be indented two spaces
+1
Comment #129
donquixote CreditAttribution: donquixote commented#127
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.
Comment #130
andypostYep, so cleanup parameterBag from uses and fixes #128
Comment #131
Crell CreditAttribution: Crell commentedI'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. :-)
Comment #132
tim.plunkettDouble space after .
Builds
array|false|null in this case. Don't ask me why.
This can be re-wrapped.
Provides
I don't think we generally nest anything below @var
mismatch
@todo, no colon
Why not just do this now?
Missing {@inheritdoc}
Missing leading \
Missing docblocks
Delete this?
If TRUE?!
Comment #133
andypostSuppose
\UnexpectedValueException
should be enough hereComment #134
andypostPrevious patch was wrong
Comment #136
tim.plunkett#134: 1947536-breadcrumbs-133.patch queued for re-testing.
Comment #137
tim.plunkettOkay, 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:
So I've disabled the blocks for now, and also converted the existing bit in template_process_page().
Otherwise, I think this is done.
Comment #139
andypostonly ForumTest still broken - working on it
Comment #140
andypostnot forum tests should be fine
Comment #141
andypostAnd 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
Comment #142
tstoecklerDoesn'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.
Comment #143
andypost@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
the ordering is right: modules registers their services in container, once one of them returns array this points to builder to stop
Comment #144
tstoecklerI guess it would be nice to mention that you can pass an empty array() to suppress all breadcrumbs.
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...Comment #144.0
tstoecklerUpdated issue summary.
Comment #144.1
donquixote CreditAttribution: donquixote commentedSummary 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.
Comment #145
donquixote CreditAttribution: donquixote commentedI updated the issue summary.
I hope it is a balanced representation of the current direction we are taking.
Comment #146
Crell CreditAttribution: Crell commented#143: 1947536-breadcrumbs-143.patch queued for re-testing.
Comment #148
andypostMerged HEAD and addressed #144
Comment #149
Crell CreditAttribution: Crell commentedLet's get a move on then. :-)
Comment #150
tstoecklerWell now $builder is actually an array of builders :-), but I haven't thought of any better suggestions either, so...
Comment #151
alexpottMinor nit but the docs seem to be wrong...
This is not sorted by priority it's keyed by priotity... Also I think this should be...
No need for the repetition... same for the variable doc block for $sortedBuilders too...
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?
Comment #152
andypost@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 goodComment #154
andypostMerge after #2014215: Shift render array defaults back out onto BlockRenderController
Comment #155
Crell CreditAttribution: Crell commentedThe changes in the last 2 patches look fine to me. Back to RTBC unless the bot objects.
Comment #156
alexpottNULL 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.
Comment #157
andypostAs you see $sortedBuilders is always array and always initialized
Comment #158
andypostIn 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 statesComment #159
Crell CreditAttribution: Crell commentedAlex: 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.
Comment #160
donquixote CreditAttribution: donquixote commentedThis might feel awkward, but one option would be to name the protected variable as "$sortedBuildersOrNull".
Comment #161
Crell CreditAttribution: Crell commenteddon: No offense, but that's a terrible idea. :-) We're OK here, or at least as OK as we are everywhere else in core.
Comment #162
alexpott#154: 1947536-breadcrumbs-154.patch queued for re-testing.
Comment #164
tim.plunkett#154: 1947536-breadcrumbs-154.patch queued for re-testing.
Comment #166
Crell CreditAttribution: Crell commentedBot goof.
Comment #167
alexpottCommitted 4d08d57 and pushed to 8.x. Thanks!
Comment #168
Crell CreditAttribution: Crell commentedWIN!
This definitely needs a change notice...
Comment #169
jibranThis issue needs followups I can create if someone agrees to me.
This one.
This one.
And this one.
Comment #170
andypostAgreed! and follow-up to convert other dsb()...
EDIT first there should be change notice
Comment #171
Crell CreditAttribution: Crell commentedChange noticed added: https://drupal.org/node/2026025
Yes, let's open up a follow ups.
Comment #172
jibranFor
drupal_set_breadcrumb()
created #2026075: [meta] Remove drupal_set_breadcrumb and LegacyBreadcrumbBuilder andFor
menu_get_active_breadcrumb()
#2026077: Rewrite menu_get_active_breadcrumb()And #2026081: Clean up ForumBreadcrumbBuilder::build() for
Comment #173
jibranI haven't created issue for
taxonomy_term_page
because it is converted to views in #1857256: Convert the taxonomy listing and feed at /taxonomy/term/%term to Views and will be fixed in #2026073: Convert drupal_set_breadcrumb/ViewExecutable::getBreadcrumb() to breadcrumb builder service in ViewExecutable.Comment #174.0
(not verified) CreditAttribution: commentedRemove a "-" in issue summary.