Updated: Comment #89
Problem/Motivation
We want to remove the {menu_router} table but the fallback breadcrumb implementation relies on menu_get_active_breadcrumb() which relies on menu_get_active_trail() which in turn relies on the old {menu_router}. In addition, the coupling of the breadcrumb to the {menu_links] table makes it very difficult to refactor links to properly interate with the new routing system.
Proposed resolution
Implement a path based breadcrumb hierarchy instead ala Breadcrumbs by path
Remaining tasks
Reviews
Decide on what to do with tests for particular breadcrumbs
Profiling/Optimisation
User interface changes
Some breadcrumbs will appear differently.
API changes
menu_get_active_breadcrumb() is removed
PathBasedBreadcrumbBuilder replaces MenuLinkBreadcrumbBuilder as the default builder.
BookBreadcrumbBuilder is added to support book module hierarchy
hook_menu_breadcrumb_alter() is replaced by hook_system_breadcrumb_alter(), and is invoked from the BreadcrumbManager
menu_item_route_access() is not be used, instead the Drupal\Core\Access\AccessManager is used.
- Removed a test which tests breadcrumbs on local tasks. This test is now pointless because we have a pure path based building, which will work no matter we have just a path or local tasks with paths.
Related Issues
#1983534: [META] WSCCI Home Stretch
#2076085: Resolve the need for a 'title callback' using the route
Comment | File | Size | Author |
---|---|---|---|
#146 | breadcrumb-2070651-146.patch | 55.75 KB | pwolanin |
#146 | 2070651-142-146.increment.txt | 1.45 KB | pwolanin |
#142 | breadcrumb-2070651-142.patch | 55.6 KB | dawehner |
#142 | interdiff.txt | 5.06 KB | dawehner |
#138 | breadcrumb-2070651-138.patch | 55.18 KB | pwolanin |
Comments
Comment #1
larowlanLets see how many tests rely on breadcrumbs as they currently stand.
This will need profiling/optimisation too.
Updated issue summary to indicate so and added note that menu_item_route_access is deprecated, replaced by menu_link.access service.
Look ma - no {menu_router} - the breadcrumbs below are generated without menu_get_active_breadcrumb() :)
Comment #2
larowlanAnd this one has them in the correct order :)
Comment #3
larowlanThis one catches the not found exception to prevent it bubbling when no such route exists.
And fixes issue with wrong property (urlGenerator)
Comment #5
tim.plunkettNice start! This is the cause of most of the notices, hopefully we'll see real fails now.
Comment #7
tim.plunkettHere are some fixes, including restoring a version of the old breadcrumb just for blocks.
Comment #8
larowlanYep thanks on the notices, dawned on me during the night. Also we need to check access in the legacy bit too.
Comment #9
tim.plunkettI figured I'd get as far as I could while you were asleep :)
Comment #11
tim.plunkettOkay just fixing my own bug, now its just up to deciding what from BreadcrumbTest to delete.
Comment #12
tim.plunkettOne bug I noticed during manual testing:
Go to admin/structure/views/view/frontpage/edit
See the breadcrumb is "Home > Administrator > Structure > Views"
Click "Views"
Expected:
Goes to admin/structure/views
Actual:
Goes to admin/structure/views/view, which is a 404
Comment #14
larowlanso this makes BookBreadcrumbBuilder not rely on legacy stuff so we can remove menu_get_active_breadcrumb() (which I have).
There are fails in BreadcrumbTest, particularly for routes with params that still use hook_menu() eg admin/structure/taxonomy/manage/%/* won't have the breadcrumb relating to the vocabulary, as taxonomy_term_overview is not yet a controller. So here comes the big question. Do we remove those tests until such time or do we postpone this on other wscci conversions?
Would we consider removing those tests with a follow up to add them back that is postponed on each wscci conversion? Or do we note that they need to go back in for/with each distinct conversion?
Comment #15
larowlanOr, we can drop the else back one level, and just let legacy stuff use menu_get_item() instead of loading a menu_link and let it take care of itself.
Time. Distance. Lunch. things fall into place
Comment #17
larowlanBook breadcrumb relies on menu_link.access service ... so upgrade tests fail, I guess that should be a core or system module service?
Comment #18
larowlanMoves menu_link.access to a core service
Comment #19
jibranWhat is the difference between #2026075: [meta] Remove drupal_set_breadcrumb and LegacyBreadcrumbBuilder and this issue?
/me hides.
I think we have to mark some issues duplicate. :)
Comment #20
larowlanHmm, I couldn't find that one and the discussion yesterday was 'remove menu based breadcrumbs from core'
That issue is about removing the legacy (drupal_set_breadcrumb, based on globals), this one is about removing reliance on menu_route from MenuBreadcrumbBuilder (and menu_get_active_breadcrumb).
They are different I think.
Comment #22
jibranThen #2026077: Rewrite menu_get_active_breadcrumb() is definitely duplicate.
Comment #23
jibranNW as per #21. Marked #2061931: Remove drupal_set_breadcrumb and LegacyBreadcrumbBuilder in menu_link module. and #2061937: Remove drupal_set_breadcrumb and LegacyBreadcrumbBuilder in includes as duplicate.
Postpone on this #2026071: Convert drupal_set_breadcrumb to breadcrumb builder service in MenuLinkFormController::form() and #2061933: Remove drupal_set_breadcrumb and LegacyBreadcrumbBuilder in system module.
Comment #24
larowlanOk. this should be down to just active-trail and BreadcrumbTest fails.
i.e. The point where we need to make a decision about what to remove.
Comment #25
larowlanComment #26
pwolanin CreditAttribution: pwolanin commentedI like the approach, but I think the access check should be using this recently added service: #2046737: Add a method to the AccessManager that only needs a route name and parameters
Comment #27
larowlanFixes #26 however the upcast/downcasting seems clumsy.
Not sure if MenuBreadcrumbBulder is the best place for those methods.
Comment #29
larowlanupcasting didn't work for dynamic stuff, eg entity_page_label, will refactor after work.
Comment #30
larowlanAs promised
Comment #32
larowlan10 Fails, all breadcrumbs/menu-active-trail related.
What next - kill those tests?
Comment #33
pwolanin CreditAttribution: pwolanin commentedI you have the request with upcast attributes, why pass around $map interanlly?
We should be removing before D8 release all the numeric-placeholder-based code anyhow?
Comment #34
larowlanI don't have the request but can add it as a dependency
Comment #35
pwolanin CreditAttribution: pwolanin commentedI meant the request that's created as part of the access checking
Comment #36
pwolanin CreditAttribution: pwolanin commentedI think the key change is removing the reliance on {menu_links} - the router part will go away in any case and is just used for access checking.
Comment #37
dawehnerIt would be cool to use entity.manager.
Let's somehow document why a certain priority is chosen.
It feels wrong to rely on drupal_menu_item itself, as it is a old menu_router concept.
We need some documentation what is going on. Potentially we maybe should also multi-load the routes.
So we are doing this to be upload to get the upcasted values? Why are we not asking the upcasting directly instead. A little bit of documentation might help.
This feels to be like a awful amount of work to just get $route->compile()->getVariables()
Let's use entity.manager
Comment #38
pwolanin CreditAttribution: pwolanin commentedugh, this is still very much a work in progress, but wanting to show the direction I think we're headed in based on IRC discussion w/ larowlan and timplunkett.
Comment #39
pwolanin CreditAttribution: pwolanin commentedComment #40
pwolanin CreditAttribution: pwolanin commentedok, this is mostly working thanks to help from Crell and dawehner to figure out how to use the path processor and router.
Using the already loaded legacy router item keeps that code very short happily.
Comment #41
dawehnerBetter use RouteObjectInterface::ROUTE_NAME
I would vote to create a different breadcrumb service just taking over that specific route.
Should we just move this !$title logic to the next if, as the old menu items, could have the same problem?
Urgs, there is some trailing whitespace hidden.
Comment #43
pwolanin CreditAttribution: pwolanin commented@dawehner - the added menu link edit link is part of the reason I was suggesting chaining. Otherwise we have to maybe inherit this one?
This patch fixes the other issues, but we have to look into the test failures.
Comment #44
pwolanin CreditAttribution: pwolanin commentedbah
Comment #45
pwolanin CreditAttribution: pwolanin commentedI shouldn't have gone back to adding the parameters - that's causing most of the test fails. Since they are not used or needed, let's just pass an empty array as above. Also missing a ";" (haste makes waste)
Comment #47
pwolanin CreditAttribution: pwolanin commentedtoo aggressive in pruning the constructor...
HEAD tests are broken at the moment too...
Comment #48
pwolanin CreditAttribution: pwolanin commentedComment #50
pwolanin CreditAttribution: pwolanin commentedOdd, I just rebased and didn't see any conflicts. Trying again.
Comment #52
larowlanhopefully moving in the right direction
Comment #54
pwolanin CreditAttribution: pwolanin commentedSeems worse? where is system.site.page config going to be used?
Comment #55
larowlanBlocked on #2076435: Enabling forum on Standard install profile from the UI causes an Exception which seems to occur irregularly
Comment #56
larowlanThanks to @berdir, patch above had rogue empty field yml file from another branch
Comment #58
larowlanShould keep moving in right direction.
Comment #60
jibranGreat work @larowlan and @pwolanin only 25 fails remaining.
Comment #61
larowlanStill getting a fatal, so actual fails might be more (we went from 10 to 25 because we get further now).
Comment #62
pwolanin CreditAttribution: pwolanin commentedpatch didn't apply, but I was able to re-base. Here's a re-roll of #58
Comment #63
pwolanin CreditAttribution: pwolanin commentedThis fixes an infinite loop + timeout so we can start to see the real fails.
Comment #65
pwolanin CreditAttribution: pwolanin commentedI think having this complete is also blocked on #2076085: Resolve the need for a 'title callback' using the route
Comment #65.0
pwolanin CreditAttribution: pwolanin commentedUpdated issue summary.
Comment #66
donquixote CreditAttribution: donquixote commentedIn IRC I expressed my feeling that I miss the word "builder" in the issue title and summary..
so here it is, a new title.
Note: With the modular infrastructure for breadcrumb builders, it would actually be possible to do these two things separately - in separate issues or in separate patches/commits. Just saying.
Comment #67
pwolanin CreditAttribution: pwolanin commentedkills or modified outdated tests, plus adds a rule to skip 'user' in the breadcrumb (since it's just a redirect now).
Also adds a _title to the route for admin/config so it looks right.
Comment #68
donquixote CreditAttribution: donquixote commentedThis should say "Constructs the BookBreadcrumbBuilder" :)
Comment #69
donquixote CreditAttribution: donquixote commentedJust wondering, if we already have a BookBreadrumbBuilder based on menu links without having to refer to nasty legacy code..
then why don't we do the same with MenuLinkBreadcrumbBuilder, and e.g. allow the user to enable or disable it, or enable it only for specific menus?
BookBreadcrumbBuilder could then be a subclass of that..
The other thing:
I really think it would be a good idea to do this piecemeal. Have one patch to introduce the new path-based builder. Then have another one or two or even three to introduce book and remove menu_link.
This is for supposedly easier reviews, and a nicer git history.
This does not have to slow us down: We can upload a series of patches + interdiff, so at any time we already have the full series to review. I've done this elsewhere, seemed like a good idea to me.
Comment #71
pwolanin CreditAttribution: pwolanin commentedTheBookBreadcrumBuilder *does* refer to nasty legacy code and will need to be updated as links change. However, its use of them is much more constrained than the main code.
We must get rid of the existing one if we want to modernize the menu link handling, so I don't see any benefit to a 2-phase approach when this patch is getting very close.
Comment #72
pwolanin CreditAttribution: pwolanin commentedAdds an alter hook.
Comment #73
tim.plunkettSince we only get two extra params here, do we want to combine $attributes and $successful_builder into a $context array, just in case? Or continue passing $attributes, but have $context = array('builder' => $successful_builder); as the 4th param?
If we choose to not pass $builder in a $context array, then we should typehint it.
Comment #74
pwolanin CreditAttribution: pwolanin commentedFixes the hook and implements it in menu_link modules, but requires this fix: #2080841: l() method on \Drupal needs to be static
Comment #76
pwolanin CreditAttribution: pwolanin commentedAll the tests should pass now, though there are some that we should remove since they are now invalid since they are testing menu link based behavior and they just happen to pass.
Comment #76.0
pwolanin CreditAttribution: pwolanin commentedUpdated issue summary.
Comment #77
dawehnerNitpick: missing "\" at the beginning.
We can now use the link generator. For translatability we should use $this->t() all the time and just write a method which does what we need.
I just realized that we can't call t() at the current yaml defined titles ...
I try to understand how image styles has to do with breadcrumbs. Is this change really needed? I reverted that for now.
We should better use RouteObjectInterface::ROUTE_NAME
Missing end of line.
Nitpick: missing empty line.
As before we need a t() function.
Comment #79
jibranA non technical review.
Please add issue link.
#2047619: Add a link generator service for route-based links is fixed.
:)
config object is not in the patch anymore. It moved out :D
I think we need a bitwise operator here.
incomplete @todo.
We can remove this @todo.
no need of space before in.
I think we can shift "only the" to first comment line.
I think this should be more descriptive. An Array of html breadcrumb links. Returned by build method. Something like that.
same here it should mention something about $request as well.
Comment #80
jibranX-post with @dawehner and testbot. My review is for #76.
Comment #81
dawehnerDone
Fixed
Fixed in #78
Good catch!
Do you have a congrete suggestion?
Fixed.
Please have a look whether this works for you now.
According to my IDE this would be 81 chars.
Good points!
What do you think about the two interdiffs?
Comment #82
jibranI think from my previous review you missed 8th point.
@todo and in should be align.
About
($menu_item['type'] & MENU_LINKS_TO_PARENT)
I think we should add a comment. By looking at the code it seems to me, at first, it is a typo and we missed&&
but @pwolanin told me on IRCconst MENU_LINKS_TO_PARENT = 0x0008;
entity.manager now.
This should mention something about html as well because it contains html.
I think we can drop coming.
This should be align
Comment #84
dawehnerComment #85
jibranThank you @dawehner for fixing #82.
Sorry we have to revert this change. @tim.plunkett pointed me to the multi-line @todo standard http://paste2.org/hsgkh2gj
More then 80 chars.
I like this thank you for adding this. :)
I think you missed it.
May includes the following key:
Comment #87
dawehnerWell, this is still called and I think attributes are actually more complex to understand than the first variable.
So I finally understand what is going on. On 403/404 pages the exception controller create a new subrequest. Request::create()
takes the Uri to request, though we always ignored the base url.
I am not sure whether this is the 100% right fix, but at least it works.
Comment #88
pwolanin CreditAttribution: pwolanin commentedUpdated the summary a bit.
I don't think we need to call it blocked, given the state of the patch - but we'll need to integrate title callback depending on which patch goes in first. I'd rather say this is blocking progress on route conversion and menu link conversion.
Comment #88.0
pwolanin CreditAttribution: pwolanin commentedadd
Comment #88.1
pwolanin CreditAttribution: pwolanin commentedUpdated issue summary.
Comment #89
pwolanin CreditAttribution: pwolanin commentedSeems like drupal_set_breadcrumb() is deprecated but still supported for 8.x, so updating the summary
Comment #89.0
pwolanin CreditAttribution: pwolanin commentedUpdated issue summary.
Comment #89.1
pwolanin CreditAttribution: pwolanin commentedUpdated issue summary.
Comment #90
pwolanin CreditAttribution: pwolanin commentedSome minor cleanup of code comments and an unused variable.
Comment #91
amateescu CreditAttribution: amateescu commentedThe stuff related to the new routing system are a bit beyond me, but here's what I found on a visual inspection of the patch:
The comment said that 'only_active_trail' was used internally for breadcrumbs. If that's no longer the case, don't we have to remove some more code from menu_tree_page_data()?
Actually constructs a BookBreadcrumbBuilder.
Where is 'Manage form display'?
Comment #92
dawehnerComment #93
larowlanBlank line
Do we need a todo regarding _title_callback?
We have a link generator now.
Anyway we can use the new duplicate helper here? I don't think we can, but worth asking.
This @todo can go now, we've got 'Configuration' in the _title (and the test has been reverted).
This will need a todo to remove once _title_callback is allowed in routing.yml
linke? typo
> 80 chars
extra line? or is that expected with @endcode?
Comment #94
dawehnerThe link generator though just works upon route names/parameters and not on paths... so we could distinct between a route and a page callback here as well, but this would make it harder to read for now.
What about waiting until we have removed the old router system?
Good idea!
You are right, see https://drupal.org/node/1354#code
Comment #95
pwolanin CreditAttribution: pwolanin commentedOrder of arguments is wrong.
Comment #96
pwolanin CreditAttribution: pwolanin commentedAdjusting the code comment around l(). Given the internals of the UrlGenerator, this is better for now until we can convert as described.
Also, changing the RequestHelper so it empties the new request attributes. I was seeing object unexpectedly carried over from the parent request.
Comment #97
dawehnerMaybe we should document on the documentation of the function?
Comment #98
pwolanin CreditAttribution: pwolanin commentedOk, removed the attributes param all together and document that the attributes are emptied. But we have to put back _account until https://drupal.org/node/2073531 is resolved.
This also translates the route _title, which was a missing step before.
Comment #100
pwolanin CreditAttribution: pwolanin commentedfixes the notices - I don't know why the fails are coming and going.
Comment #102
dawehnerI could not figure out anything so here is just a rerole.
Especially the language failures feels like the changes to the cloned request object leaks into the main request.
Comment #104
pwolanin CreditAttribution: pwolanin commentedSo, I think we don't full understand the side effects of using duplicate() on the request, at least I'm at an immediate loss to explain why it's throwing off the language handling when we clear the attributes.
Per discussion with dawehner going back to use Request::create() since that's working and we can revisit an optimization later.
Also bumping to major since this blocks various menu link and router conversions.
Comment #105
pwolanin CreditAttribution: pwolanin commentedThis change resolves a fatal error on PathLanguageTest, but there is still a DB exception in the debug output.
might be a core bug.
Comment #106
dawehnerHere is a quick fix
Comment #107
dawehnerPeter suggested a better fix.
Comment #108
pwolanin CreditAttribution: pwolanin commentedThat's fine, though we don't even need the else
Comment #109
pwolanin CreditAttribution: pwolanin commentedbumping to critical per catch since it's blocking other menu link conversions.
Comment #110
ParisLiakos CreditAttribution: ParisLiakos commentedLooks mostly awesome
check against MenuLinkInterface?
why dont we use the linkGenetor here?
Drupal:t() ?
there is no such thing afaik.
just t()
Finally i think PathBasedBreadcrumbBuilder could really use some unit tests
Comment #111
dawehnerWell, we could, but we should better wait until we just use route names, as otherwise the code would be too complicated ...
Feel free to add a unit test, but it is too late for now :)
Comment #113
dawehnerThis was a stupid mistake.
Comment #114
pwolanin CreditAttribution: pwolanin commentedI think with the last couple rounds of review this is pretty well polished.
Comment #115
ParisLiakos CreditAttribution: ParisLiakos commentedit is either $this->t() or t(). and a forgotten comment
Comment #116
pwolanin CreditAttribution: pwolanin commentedoops - good catch - that was left from the earlier t() problems.
Comment #117
catchDidn't do an in-depth review of the whole patch yet, but in general this looks good. I wouldn't expect this to be too much of a performance difference either way from using the active trail, especially when a path doesn't have too many parts, but it'd be interesting to see what it looks like in the profiler - because it's in the critical path of most pages and in case there's any surprises.
Few minor things:
Please reverse the if/else so that converted routes get checked first.
This could duplicate the request now rather than creating it from scratch: https://drupal.org/node/2086077.
Why is the ucfirst/strtolower necessary?
Comment #118
pwolanin CreditAttribution: pwolanin commented@catch - ucfirst/strtolower was to try to bring this in line with the capitalization a hand-entered title would have. Possibly ucfirst would be sufficient, or we could just skip it.
We were trying to use the request duplicate helper, but I think we need to revisit that a bit - there are request attributes that are carried over from the current request and it seemed as though leaving OR removing them had unexpected side effects in patches above.
Comment #119
catchI think we could skip that with the titles - ucfirst can be done in the theme if it's desired.
If the new request helper isn't working with this, I'd be OK with a @todo + follow-up - this is blocking other issues.
Comment #120
dawehnerComment #122
pwolanin CreditAttribution: pwolanin commentedroute name changed from menu_link_edit to menu.link_edit
Comment #124
dawehnerThis was easier to fix as thought.
Comment #126
pwolanin CreditAttribution: pwolanin commentedOops, the actual route for the link is menu.menu_edit, not menu.link_edit.
Comment #128
dawehner#126: breadcrumb-2070651-126.patch queued for re-testing.
Comment #129
jibranretagging.
Comment #130
dawehner#126: breadcrumb-2070651-126.patch queued for re-testing.
Comment #132
dawehnerNow that node/{node} is converted we need the title callback to work, oh wait, this issue is blocked on that one here.
Comment #133
mcrittenden CreditAttribution: mcrittenden commentedCouldn't tell if dawehner forgot to set to CNR or if that was intentional, so let's let testbot decide.
Comment #134
dawehnerThat was not intentional.
Comment #136
ParisLiakos CreditAttribution: ParisLiakos commentedso, lets reroll this one
Comment #138
pwolanin CreditAttribution: pwolanin commentedThis adds a title callback for the node view so the breadcrumb is correct on the edit page.
Comment #139
dawehnerWhat about adding a follow up to write a generic entity based one? The title resolver already instantiates a new object, so it is possible to do that.
Can we do that already?
So we can also skip that part?
Another place
Comment #140
pwolanin CreditAttribution: pwolanin commentedre: #2 - I think we still want that as a fallback (using the raw path string).
Comment #142
dawehnerWorked a bit on the book breadcrumb manager to make it using all the hot new shit.
The BreadcrumbTest still had a menu link based breadcrumb in there.
Comment #143
ParisLiakos CreditAttribution: ParisLiakos commentedyay!
Comment #144
catchThis reads a bit ambivalent to me. Aliased path could refer to either the 'path that has been aliased' or the 'aliased path'. And 'original' could be the request path as well I think. Just say 'system path'?
#2076085: Resolve the need for a 'title callback' using the route was committed - can we resolve this @todo?
Comment #145
pwolanin CreditAttribution: pwolanin commented@catch - let me make a quick re-roll. This is using the path as it comes into the request - i.e. it IS potentially an alias, and not the system path.
Comment #146
pwolanin CreditAttribution: pwolanin commentedThe latter @todo comment is misleading - we should keep this fallback for the cases still where there is no title on the route.
This patch just changes the comments.
Comment #147
catchRight of course it is because we're doing the inbound processing but I'd forgotten that when I went back to the wording of the comment :(
RTBC again.
Comment #148
alexpottCommitted 00c06fc and pushed to 8.x. Thanks!
Comment #149
pwolanin CreditAttribution: pwolanin commented1st pass change notice: https://drupal.org/node/2098323
Comment #150
pwolanin CreditAttribution: pwolanin commentedfixing tags
Comment #151
aspilicious CreditAttribution: aspilicious commentedDon't set it to fixed if the change notice isn't reviewed. I see that menu_get_active_breadcrumb() is removed.
But how do I get the current breadcrumb?
Comment #152
mcrittenden CreditAttribution: mcrittenden commentedI suppose the change notice needs work to answer the question in #151, so changing status.
Comment #153
dawehnerI am sorry but the new breadcrumb api changed that already, see https://drupal.org/node/2026025/revisions/view/2808661/2855137
Comment #154
plachComment #155
jibranCreated #2099095: Create BreadcrumbBuilderBase and remove the boilerplate code from BreadcrumbBuilders as followup.
Comment #157
xjmComment #157.0
xjmUpdated issue summary.