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.

#1983534: [META] WSCCI Home Stretch
#2076085: Resolve the need for a 'title callback' using the route

CommentFileSizeAuthor
#146 breadcrumb-2070651-146.patch55.75 KBpwolanin
#146 2070651-142-146.increment.txt1.45 KBpwolanin
#142 breadcrumb-2070651-142.patch55.6 KBdawehner
#142 interdiff.txt5.06 KBdawehner
#138 breadcrumb-2070651-138.patch55.18 KBpwolanin
#138 2070651-136-138.increment.txt6.48 KBpwolanin
#136 breadcrumb-2070651-136.patch52.4 KBParisLiakos
#132 breadcrumb-2070651-132.patch52.39 KBdawehner
#126 breadcrumb-2070651-126.patch53.74 KBpwolanin
#126 2070651-124-126.increment.txt714 bytespwolanin
#124 breadcrumb-2070651-124.patch53.74 KBdawehner
#124 interdiff.txt714 bytesdawehner
#122 breadcrumb-2070651-122.patch53.74 KBpwolanin
#122 2070651-120-122.increment.txt1.28 KBpwolanin
#120 breadcrumb-2070651-120.patch53.74 KBdawehner
#120 interdiff.txt2.52 KBdawehner
#115 breadcrumb-2070651-115.patch53.69 KBParisLiakos
#115 interdiff.txt1.4 KBParisLiakos
#113 breadcrumb-2070651-113.patch53.76 KBdawehner
#113 interdiff.txt473 bytesdawehner
#111 breadcrumb-2070651-111.patch53.62 KBdawehner
#111 interdiff.txt1.69 KBdawehner
#107 breadcrumb-2070651-107.patch53.72 KBdawehner
#107 interdiff.txt944 bytesdawehner
#106 breadcrumb-2070651-105.patch53.73 KBdawehner
#106 interdiff.txt989 bytesdawehner
#105 breadcrumb-2070651-105.patch53.68 KBpwolanin
#105 2070651-104-105.increment.txt2.64 KBpwolanin
#104 breadcrumb-2070651-104.patch53.4 KBpwolanin
#104 2070651-102-104.increment.txt3.17 KBpwolanin
#102 breadcrumb-2070651-102.patch55.69 KBdawehner
#100 breadcrumb-2070651-100.patch55.69 KBpwolanin
#100 2070651-98-100.increment.txt931 bytespwolanin
#98 breadcrumb-2070651-98.patch55.69 KBpwolanin
#98 2070651-96-98.increment.txt3.7 KBpwolanin
#96 breadcrumb-2070651-96.patch54.08 KBpwolanin
#96 2070651-95-96.increment.txt1.73 KBpwolanin
#95 breadcrumb-2070651-95.patch53.2 KBpwolanin
#95 2070651-94-95.increment.txt861 bytespwolanin
#94 breadcrumb-2070651-94.patch53.2 KBdawehner
#94 interdiff.txt5.36 KBdawehner
#92 breadcrumb-2070651-92.patch53 KBdawehner
#92 interdiff.txt2.46 KBdawehner
#90 breadcrumb-2070651-90.patch52.58 KBpwolanin
#90 2070651-87-90.increment.txt1.78 KBpwolanin
#87 breadcrumb-2070651-87.patch52.61 KBdawehner
#87 interdiff.txt3.42 KBdawehner
#84 breadcrumb-2070651-84.patch50.95 KBdawehner
#84 interdiff.txt40.03 KBdawehner
#81 breadcrumb-2070651-81.patch46.55 KBdawehner
#81 interdiff.txt4.53 KBdawehner
#77 breadcrumbs-by-path-2070651-76.patch46.58 KBdawehner
#77 interdiff.txt13.76 KBdawehner
#76 breadcrumbs-by-path-2070651-76.patch46.88 KBpwolanin
#76 2070651-74-76.increment.txt975 bytespwolanin
#74 breadcrumbs-by-path-2070651-74.patch46.78 KBpwolanin
#74 2070651-72-74.increment.txt4.66 KBpwolanin
#72 breadcrumbs-by-path-2070651-72.patch46.16 KBpwolanin
#72 2070651-67-72.increment.txt8.29 KBpwolanin
#67 breadcrumbs-by-path-2070651-67.patch38.2 KBpwolanin
#67 2070651-63-67.increment.txt0 bytespwolanin
#63 breadcrumbs-by-path-2070651-63.patch33.37 KBpwolanin
#63 2070651-58-63.increment.txt3.97 KBpwolanin
#62 rebase-2070651.58.patch30.06 KBpwolanin
#58 breadcrumbs-by-path-2070651.58.interdiff.txt3.89 KBlarowlan
#58 breadcrumbs-by-path-2070651.58.patch30.06 KBlarowlan
#56 breadcrumbs-by-path-2070651.56.interdiff.txt179 byteslarowlan
#56 breadcrumbs-by-path-2070651.56.patch28.83 KBlarowlan
#52 breadcrumbs-by-path-2070651.52.interdiff.txt12 KBlarowlan
#52 breadcrumbs-by-path-2070651.52.patch29 KBlarowlan
#50 breadcrumbs-by-path-2070651-50.patch20.9 KBpwolanin
#48 2070651-45-47.increment.txt3.26 KBpwolanin
#47 breadcrumbs-by-path-2070651-47.patch115.88 KBpwolanin
#47 breadcrumbs-by-path-2070651-47.patch115.88 KBpwolanin
#45 breadcrumbs-by-path-2070651-45.patch20.59 KBpwolanin
#45 2070651-43-45.increment.txt1.34 KBpwolanin
#44 2070651-40-43.increment.txt1.93 KBpwolanin
#43 breadcrumbs-by-path-2070651-43.patch20.63 KBpwolanin
#43 breadcrumbs-by-path-2070651-43.patch20.63 KBpwolanin
#40 breadcrumbs-by-path-2070651-40.patch20.59 KBpwolanin
#40 2070651-38-40.increment.txt9.32 KBpwolanin
#39 2070651-30-38.increment.txt24.49 KBpwolanin
#38 breadcrumbs-by-path-2070651-38.patch23.21 KBpwolanin
#30 breadcrumbs-by-path-2070651.30.interdiff.txt5.81 KBlarowlan
#30 breadcrumbs-by-path-2070651.30.patch21.58 KBlarowlan
#27 breadcrumbs-by-path-2070651.27.interdiff.txt15.65 KBlarowlan
#27 breadcrumbs-by-path-2070651.27.patch20.81 KBlarowlan
#24 breadcrumbs-by-path-2070651.24.interdiff.txt3.35 KBlarowlan
#24 breadcrumbs-by-path-2070651.24.patch23.96 KBlarowlan
#18 breadcrumbs-by-path-2070651.18.interdiff.txt2.62 KBlarowlan
#18 breadcrumbs-by-path-2070651.18.patch23.55 KBlarowlan
#15 breadcrumbs-by-path-2070651.15.interdiff.txt1.09 KBlarowlan
#15 breadcrumbs-by-path-2070651.15.patch23.08 KBlarowlan
#14 breadcrumbs-by-path-2070651.14.interdiff.txt17.45 KBlarowlan
#14 breadcrumbs-by-path-2070651.14.patch23.1 KBlarowlan
#11 breadcrumb-2070651-10.patch13.46 KBtim.plunkett
#11 interdiff.txt990 bytestim.plunkett
#7 breadcrumb-2070651-7.patch13.54 KBtim.plunkett
#7 interdiff.txt4.07 KBtim.plunkett
#5 breadcrumb-2070651-4.patch10.08 KBtim.plunkett
#5 interdiff.txt815 bytestim.plunkett
#3 breadcrumbs-by-path-2070651.3.interdiff.txt2.47 KBlarowlan
#3 breadcrumbs-by-path-2070651.3.patch10.06 KBlarowlan
#2 Screen Shot 2013-08-21 at 6.52.02 PM.png25.36 KBlarowlan
#2 Screen Shot 2013-08-21 at 6.52.11 PM.png35 KBlarowlan
#2 breadcrumbs-by-path-2070651.2.interdiff.txt1.16 KBlarowlan
#2 breadcrumbs-by-path-2070651.2.patch9.86 KBlarowlan
#1 breadcrumbs-by-path-2070651.1.patch9.84 KBlarowlan
#1 Screen Shot 2013-08-21 at 6.41.28 PM.png48.24 KBlarowlan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Status: Active » Needs review
FileSize
48.24 KB
9.84 KB

Lets 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() :)

Screen Shot 2013-08-21 at 6.41.28 PM.png

larowlan’s picture

larowlan’s picture

This one catches the not found exception to prevent it bubbling when no such route exists.
And fixes issue with wrong property (urlGenerator)

tim.plunkett’s picture

Issue tags: +MenuSystemRevamp
FileSize
815 bytes
10.08 KB

Nice start! This is the cause of most of the notices, hopefully we'll see real fails now.

tim.plunkett’s picture

FileSize
4.07 KB
13.54 KB

Here are some fixes, including restoring a version of the old breadcrumb just for blocks.

larowlan’s picture

Yep thanks on the notices, dawned on me during the night. Also we need to check access in the legacy bit too.

tim.plunkett’s picture

I figured I'd get as far as I could while you were asleep :)

tim.plunkett’s picture

FileSize
990 bytes
13.46 KB

Okay just fixing my own bug, now its just up to deciding what from BreadcrumbTest to delete.

tim.plunkett’s picture

One 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

larowlan’s picture

so 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?

larowlan’s picture

Or, 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

larowlan’s picture

Status: Needs review » Needs work

Book breadcrumb relies on menu_link.access service ... so upgrade tests fail, I guess that should be a core or system module service?

larowlan’s picture

Status: Needs work » Needs review
FileSize
23.55 KB
2.62 KB

Moves menu_link.access to a core service

jibran’s picture

What 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. :)

larowlan’s picture

Hmm, 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.

jibran’s picture

larowlan’s picture

Ok. 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.

larowlan’s picture

Status: Needs work » Needs review
pwolanin’s picture

Status: Needs review » Needs work

I 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

larowlan’s picture

Status: Needs work » Needs review
FileSize
20.81 KB
15.65 KB

Fixes #26 however the upcast/downcasting seems clumsy.
Not sure if MenuBreadcrumbBulder is the best place for those methods.

larowlan’s picture

Status: Needs review » Needs work

upcasting didn't work for dynamic stuff, eg entity_page_label, will refactor after work.

larowlan’s picture

Status: Needs work » Needs review
FileSize
21.58 KB
5.81 KB

As promised

larowlan’s picture

Status: Needs review » Needs work

10 Fails, all breadcrumbs/menu-active-trail related.
What next - kill those tests?

pwolanin’s picture

I 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?

larowlan’s picture

I don't have the request but can add it as a dependency

pwolanin’s picture

I meant the request that's created as part of the access checking

pwolanin’s picture

Title: Remove reliance on {menu_router} for breadcrumbs, build based on path instead. » Remove reliance on {menu_links} for breadcrumbs, build based on path instead.
Issue tags: +Stalking Crell

I 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.

dawehner’s picture

  1. +++ b/core/modules/book/book.services.yml
    @@ -1,4 +1,9 @@
    +    arguments: ['@router.route_provider', '@plugin.manager.entity', '@access_manager', '@string_translation', '@router.dynamic']
    

    It would be cool to use entity.manager.

  2. +++ b/core/modules/book/book.services.yml
    @@ -1,4 +1,9 @@
    +    tags:
    +      - { name: breadcrumb_builder, priority: 701 }
    
    +++ b/core/modules/menu_link/menu_link.services.yml
    @@ -1,5 +1,6 @@
         tags:
           - { name: breadcrumb_builder, priority: 0 }
    

    Let's somehow document why a certain priority is chosen.

  3. +++ b/core/modules/book/lib/Drupal/book/BookBreadcrumbBuilder.php
    @@ -0,0 +1,70 @@
    +    if (!empty($attributes['_system_path']) && isset($attributes['_drupal_menu_item']['map'][1]->book)) {
    

    It feels wrong to rely on drupal_menu_item itself, as it is a old menu_router concept.

  4. +++ b/core/modules/book/lib/Drupal/book/BookBreadcrumbBuilder.php
    @@ -0,0 +1,70 @@
    +      $menu_links = $this->menuLinkStorage->loadMultiple($mlids);
    +      if (count($menu_links) > 0) {
    +        $depth = 1;
    +        while (!empty($book['p' . ($depth + 1)])) {
    +          if (!empty($menu_links[$book['p' . $depth]]) && ($menu_link = $menu_links[$book['p' . $depth]])) {
    +            if (!empty($menu_link->route_name)) {
    +              try {
    ...
    +                  $parameters = $this->getNamedParameters($route, $path_elements);
    +                  if ($this->accessManager->checkNamedRoute($menu_link->route_name, $parameters)) {
    +                    // @todo Replace with link generator service when
    +                    //   https://drupal.org/node/2047619 lands.
    +                    $links[] = l($menu_link->label(), $menu_link->link_path, $menu_link->options);
    +                  }
    +                }
    +              }
    

    We need some documentation what is going on. Potentially we maybe should also multi-load the routes.

  5. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkBreadcrumbBuilder.php
    @@ -15,12 +25,220 @@
    +    $request = Request::create('/' . $href);
    +    $request->attributes->set('_system_path', $href);
    +    // Attempt to match this path to provide a fully built request.
    +    try {
    +      $request->attributes->add($this->dynamicRouter->matchRequest($request));
    +    }
    +    catch (NotFoundHttpException $e) {
    +      return FALSE;
    +    }
    

    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.

  6. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkBreadcrumbBuilder.php
    @@ -15,12 +25,220 @@
    +    $path_bits = explode('/', trim($route->getPath(), '/'));
    +    foreach ($map as $index => $map_item) {
    +      $matches = array();
    +      // Search for placeholders wrapped by curly braces. For example, a path
    +      // 'foo/{bar}/baz' would return 'bar'.
    +      if (isset($path_bits[$index]) && preg_match('/{(?<placeholder>.*)}/', $path_bits[$index], $matches)) {
    

    This feels to be like a awful amount of work to just get $route->compile()->getVariables()

  7. +++ b/core/modules/menu_link/menu_link.services.yml
    @@ -1,5 +1,6 @@
    +    arguments: ['@router.route_provider', '@plugin.manager.entity', '@access_manager', '@string_translation', '@router.dynamic']
    

    Let's use entity.manager

pwolanin’s picture

ugh, 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.

pwolanin’s picture

FileSize
24.49 KB
pwolanin’s picture

Status: Needs work » Needs review
FileSize
9.32 KB
20.59 KB

ok, 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.

dawehner’s picture

+++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php
@@ -123,23 +105,32 @@ public function build(array $attributes) {
+          $route_name = $route_request->attributes->get('_route');

Better use RouteObjectInterface::ROUTE_NAME

  1. +++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php
    @@ -0,0 +1,165 @@
    +    // Custom breadcrumb behaviour for editing menu links, we append a link to
    +    // the menu in which the link is found.
    +    if (!empty($attributes['_route']) && $attributes['_route'] == 'menu_link_edit' && !empty($attributes['menu_link'])) {
    +      $menu_link = $attributes['menu_link'];
    +      if (!$menu_link->isNew()) {
    +        // Add a link to the menu admin screen.
    +        $menu = $this->menuStorage->load($menu_link->menu_name);
    +        $links[] = l($menu->label(), 'admin/structure/menu/manage/' . $menu_link->menu_name);
    +      }
    +    }
    

    I would vote to create a different breadcrumb service just taking over that specific route.

  2. +++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php
    @@ -0,0 +1,165 @@
    +          $menu_item = $route_request->attributes->get('_drupal_menu_item');
    +          $access = $menu_item['access'];
    +          $title = $menu_item['title'];
    ...
    +          $title = $route_request->attributes->get('_title');
    +          if (!$title) {
    +            $title = end($path_elements);
    +          }
    

    Should we just move this !$title logic to the next if, as the old menu items, could have the same problem?

  3. +++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php
    @@ -0,0 +1,165 @@
    +   * ¶
    

    Urgs, there is some trailing whitespace hidden.

pwolanin’s picture

Status: Needs review » Needs work
FileSize
20.63 KB
20.63 KB

@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.

pwolanin’s picture

FileSize
1.93 KB

bah

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.34 KB
20.59 KB

I 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)

pwolanin’s picture

too aggressive in pruning the constructor...

HEAD tests are broken at the moment too...

pwolanin’s picture

FileSize
3.26 KB
pwolanin’s picture

Odd, I just rebased and didn't see any conflicts. Trying again.

larowlan’s picture

hopefully moving in the right direction

pwolanin’s picture

Status: Needs review » Needs work

Seems worse? where is system.site.page config going to be used?

larowlan’s picture

larowlan’s picture

Status: Needs work » Needs review
FileSize
28.83 KB
179 bytes

Thanks to @berdir, patch above had rogue empty field yml file from another branch

larowlan’s picture

Should keep moving in right direction.

jibran’s picture

Status: Needs review » Needs work

Great work @larowlan and @pwolanin only 25 fails remaining.

larowlan’s picture

Still getting a fatal, so actual fails might be more (we went from 10 to 25 because we get further now).

pwolanin’s picture

FileSize
30.06 KB

patch didn't apply, but I was able to re-base. Here's a re-roll of #58

pwolanin’s picture

Status: Needs work » Needs review
FileSize
3.97 KB
33.37 KB

This fixes an infinite loop + timeout so we can start to see the real fails.

pwolanin’s picture

Status: Needs review » Needs work
Issue tags: +blocked

I think having this complete is also blocked on #2076085: Resolve the need for a 'title callback' using the route

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

donquixote’s picture

Title: Remove reliance on {menu_links} for breadcrumbs, build based on path instead. » Introduce a path-based breadcrumb builder, remove the one that's based on {menu_links}.

In 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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
0 bytes
38.2 KB

kills 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.

donquixote’s picture

+class BookBreadcrumbBuilder implements BreadcrumbBuilderInterface {
[..]
+  /**
+   * Constructs the MenuLinkBreadcrumbBuilder.

This should say "Constructs the BookBreadcrumbBuilder" :)

donquixote’s picture

Just 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.

pwolanin’s picture

Status: Needs review » Needs work

TheBookBreadcrumBuilder *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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
8.29 KB
46.16 KB

Adds an alter hook.

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php
    @@ -49,25 +68,30 @@ public function addBuilder(BreadcrumbBuilderInterface $builder, $priority) {
    +    $this->moduleHandler->alter('system_breadcrumb', $breadcrumb, $attributes, $successful_builder);
    

    Since 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?

  2. +++ b/core/modules/system/system.api.php
    @@ -1351,6 +1351,21 @@ function hook_module_implements_alter(&$implementations, $hook) {
    + * @param \Drupal\Core\Breadcrumb\BreadcrumbBuilderInterface $builder
    ...
    +function hook_system_breadcrumb_alter(array &$breadcrumb, array $attributes, $builder = NULL) {
    

    If we choose to not pass $builder in a $context array, then we should typehint it.

pwolanin’s picture

Fixes the hook and implements it in menu_link modules, but requires this fix: #2080841: l() method on \Drupal needs to be static

pwolanin’s picture

All 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.

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

  1. +++ b/core/modules/book/lib/Drupal/book/BookBreadcrumbBuilder.php
    @@ -0,0 +1,107 @@
    + * Contains Drupal\book\BookBreadcrumbBuilder.
    

    Nitpick: missing "\" at the beginning.

  2. +++ b/core/modules/book/lib/Drupal/book/BookBreadcrumbBuilder.php
    @@ -0,0 +1,107 @@
    +      // @todo Replace with link generator service when
    +      //   https://drupal.org/node/2047619 lands.
    +      $links = array(l($this->translation->translate('Home'), '<front>'));
    +      $book = $attributes['_drupal_menu_item']['map'][1]->book;
    

    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.

  3. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Routing/RouteSubscriber.php
    @@ -81,7 +81,10 @@ public function routes(RouteBuildEvent $event) {
    +            '_form' => '\Drupal\field_ui\FieldOverview',
    +            '_title' => t('Manage fields'),
    +          ) + $defaults,
    

    I just realized that we can't call t() at the current yaml defined titles ...

  4. +++ b/core/modules/filter/filter.routing.yml
    @@ -17,6 +17,7 @@ filter_admin_overview:
    diff --git a/core/modules/image/lib/Drupal/image/PathProcessor/PathProcessorImageStyles.php b/core/modules/image/lib/Drupal/image/PathProcessor/PathProcessorImageStyles.php
    
    diff --git a/core/modules/image/lib/Drupal/image/PathProcessor/PathProcessorImageStyles.php b/core/modules/image/lib/Drupal/image/PathProcessor/PathProcessorImageStyles.php
    index c0c6f31..4418476 100644
    
    index c0c6f31..4418476 100644
    --- a/core/modules/image/lib/Drupal/image/PathProcessor/PathProcessorImageStyles.php
    
    --- a/core/modules/image/lib/Drupal/image/PathProcessor/PathProcessorImageStyles.php
    +++ b/core/modules/image/lib/Drupal/image/PathProcessor/PathProcessorImageStyles.php
    
    +++ b/core/modules/image/lib/Drupal/image/PathProcessor/PathProcessorImageStyles.php
    +++ b/core/modules/image/lib/Drupal/image/PathProcessor/PathProcessorImageStyles.php
    @@ -46,12 +46,17 @@ public function processInbound($path, Request $request) {
    
    @@ -46,12 +46,17 @@ public function processInbound($path, Request $request) {
         $rest = preg_replace('|^' . $path_prefix . '|', '', $path);
     
         // Get the image style, scheme and path.
    -    list($image_style, $scheme, $file) = explode('/', $rest, 3);
    +    if (substr_count($rest, '/') >= 2) {
    +      list($image_style, $scheme, $file) = explode('/', $rest, 3);
     
    -    // Set the file as query parameter.
    -    $request->query->set('file', $file);
    +      // Set the file as query parameter.
    +      $request->query->set('file', $file);
     
    -    return $path_prefix . $image_style . '/' . $scheme;
    +      return $path_prefix . $image_style . '/' . $scheme;
    +    }
    +    else {
    +      return $path;
    +    }
       }
     
    

    I try to understand how image styles has to do with breadcrumbs. Is this change really needed? I reverted that for now.

  5. +++ b/core/modules/menu_link/menu_link.module
    @@ -195,3 +195,20 @@ function menu_link_maintain($module, $op, $link_path, $link_title = NULL) {
    +  // the menu in which the link is found.
    +  if (!empty($attributes['_route']) && $attributes['_route'] == 'menu_link_edit' && !empty($attributes['menu_link'])) {
    

    We should better use RouteObjectInterface::ROUTE_NAME

  6. +++ b/core/modules/menu_link/menu_link.module
    @@ -195,3 +195,20 @@ function menu_link_maintain($module, $op, $link_path, $link_title = NULL) {
    \ No newline at end of file
    

    Missing end of line.

  7. +++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php
    @@ -0,0 +1,192 @@
    +class PathBasedBreadcrumbBuilder implements BreadcrumbBuilderInterface {
    +  /**
    

    Nitpick: missing empty line.

  8. +++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php
    @@ -0,0 +1,192 @@
    +      $links[] = l($this->translation->translate('Home'), '<front>');
    

    As before we need a t() function.

jibran’s picture

A non technical review.

  1. +++ b/core/modules/book/lib/Drupal/book/BookBreadcrumbBuilder.php
    @@ -0,0 +1,107 @@
    +    // @todo - like \Drupal\forum\ForumBreadcrumbBuilder this depends on the
    +    // legacy non-route node view. It must be updated once that's converted.
    

    Please add issue link.

  2. +++ b/core/modules/book/lib/Drupal/book/BookBreadcrumbBuilder.php
    @@ -0,0 +1,107 @@
    +      // @todo Replace with link generator service when
    +      //   https://drupal.org/node/2047619 lands.
    

    #2047619: Add a link generator service for route-based links is fixed.

  3. +++ b/core/modules/menu_link/menu_link.module
    @@ -195,3 +195,20 @@ function menu_link_maintain($module, $op, $link_path, $link_title = NULL) {
    \ No newline at end of file
    

    :)

  4. +++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php
    @@ -0,0 +1,192 @@
    +  /**
    +   * Site config object.
    +   *
    +   * @var \Drupal\Core\Config\Config
    +   */
    +
    +
    

    config object is not in the patch anymore. It moved out :D

  5. +++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php
    @@ -0,0 +1,192 @@
    +          if (($menu_item['type'] & MENU_LINKS_TO_PARENT) == MENU_LINKS_TO_PARENT) {
    

    I think we need a bitwise operator here.

  6. +++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php
    @@ -0,0 +1,192 @@
    +            // @todo Revisit when
    

    incomplete @todo.

  7. +++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php
    @@ -0,0 +1,192 @@
    +          // @todo Replace with a #type => link render element when
    +          //   https://drupal.org/node/2047619 lands.
    ...
    +    // @todo Replace with a #type => link render element when
    +    //   https://drupal.org/node/2047619 lands.
    

    We can remove this @todo.

  8. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.php
    @@ -58,35 +59,14 @@ function setUp() {
    +    // @todo Revert to 'Configuration' when https://drupal.org/node/2075701 is
    +    //   in.
    

    no need of space before in.

  9. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.php
    @@ -495,16 +402,19 @@ function testBreadCrumbs() {
    +    // Since the 'admin' path is not accessible, we still expect
    +    // only the Home link.
    

    I think we can shift "only the" to first comment line.

  10. +++ b/core/modules/system/system.api.php
    @@ -1398,6 +1351,23 @@ function hook_module_implements_alter(&$implementations, $hook) {
    + *   Array of breadcrumb links.
    

    I think this should be more descriptive. An Array of html breadcrumb links. Returned by build method. Something like that.

  11. +++ b/core/modules/system/system.api.php
    @@ -1398,6 +1351,23 @@ function hook_module_implements_alter(&$implementations, $hook) {
    + *   Attributes representing the current page.
    

    same here it should mention something about $request as well.

jibran’s picture

Status: Needs review » Needs work

X-post with @dawehner and testbot. My review is for #76.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.53 KB
46.55 KB

Please add issue link.

Done

#2047619: Add a link generator service for route-based links is fixed.

Fixed

:)

Fixed in #78

config object is not in the patch anymore. It moved out :D

Good catch!

I think we need a bitwise operator here.

Do you have a congrete suggestion?

incomplete @todo.

Fixed.

We can remove this @todo.

Please have a look whether this works for you now.

no need of space before in.

According to my IDE this would be 81 chars.

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.

Good points!

What do you think about the two interdiffs?

jibran’s picture

I think from my previous review you missed 8th point.

+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.php
@@ -58,35 +59,14 @@ function setUp() {
+    // @todo Revert to 'Configuration' when https://drupal.org/node/2075701 is
+    //   in.

@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 IRC const MENU_LINKS_TO_PARENT = 0x0008;

  1. +++ b/core/modules/book/book.services.yml
    @@ -1,7 +1,7 @@
    +    arguments: ['@plugin.manager.entity', '@string_translation', '@link_generator']
    
    +++ b/core/modules/system/system.services.yml
    @@ -12,7 +12,7 @@ services:
    +    arguments: ['@request', '@plugin.manager.entity', '@access_manager', '@string_translation', '@router', '@path_processor_manager', '@config.factory', '@link_generator']
    

    entity.manager now.

  2. +++ b/core/modules/system/system.api.php
    @@ -1354,9 +1354,11 @@ function hook_module_implements_alter(&$implementations, $hook) {
    + *   An array of breadcrumb links, returned by the breadcrumb manager build
    + *   method.
    

    This should mention something about html as well because it contains html.

  3. +++ b/core/modules/system/system.api.php
    @@ -1354,9 +1354,11 @@ function hook_module_implements_alter(&$implementations, $hook) {
    + *   Attributes representing the current page, coming from
    

    I think we can drop coming.

  4. +++ b/core/modules/book/lib/Drupal/book/BookBreadcrumbBuilder.php
    @@ -79,7 +77,8 @@ public function build(array $attributes) {
    +            // @todo change this once the node view route is converted, see
    +            //   https://drupal.org/node/1987778.
    

    This should be align

dawehner’s picture

FileSize
40.03 KB
50.95 KB
  • Reverted the image style path processor changes, we need this to not fail on path processing with less parameters.
  • Fixed all the things from #82
  • Added _title on routes for many core routes to fix the breadcrumb tests.
jibran’s picture

Thank you @dawehner for fixing #82.

  1. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.php
    @@ -58,35 +59,14 @@ function setUp() {
    +    // @todo Revert to 'Configuration' when https://drupal.org/node/2075701 is
    +    // in.
    

    Sorry we have to revert this change. @tim.plunkett pointed me to the multi-line @todo standard http://paste2.org/hsgkh2gj

  2. +++ b/core/modules/system/system.api.php
    @@ -1398,6 +1351,30 @@ function hook_module_implements_alter(&$implementations, $hook) {
    + *   An array of breadcrumb link a tags, returned by the breadcrumb manager build
    

    More then 80 chars.

  3. +++ b/core/modules/system/system.api.php
    @@ -1398,6 +1351,30 @@ function hook_module_implements_alter(&$implementations, $hook) {
    + *   @code
    + *     array('<a href="/">Home</a>');
    + *   @endcode
    

    I like this thank you for adding this. :)

  4. +++ b/core/modules/system/system.api.php
    @@ -1398,6 +1351,30 @@ function hook_module_implements_alter(&$implementations, $hook) {
    + *   Attributes representing the current page, coming from
    

    I think you missed it.

    I think we can drop coming.
  5. +++ b/core/modules/system/system.api.php
    @@ -1398,6 +1351,30 @@ function hook_module_implements_alter(&$implementations, $hook) {
    + *   May include the following keys:
    

    May includes the following key:

dawehner’s picture

FileSize
3.42 KB
52.61 KB

I think you missed it.
I think we can drop coming.

Well, 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.

pwolanin’s picture

Issue tags: -blocked

Updated 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.

pwolanin’s picture

Issue summary: View changes

add

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

pwolanin’s picture

Seems like drupal_set_breadcrumb() is deprecated but still supported for 8.x, so updating the summary

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

pwolanin’s picture

Some minor cleanup of code comments and an unused variable.

amateescu’s picture

The 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:

  1. +++ b/core/includes/menu.inc
    @@ -1408,8 +1407,7 @@ function menu_tree_page_data($menu_name, $max_depth = NULL, $only_active_trail =
      *   - active_trail: An array of mlids, representing the coordinates of the
      *     currently active menu link.
      *   - only_active_trail: Whether to only return links that are in the active
    - *     trail. This option is ignored, if 'expanded' is non-empty. Internally
    - *     used for breadcrumbs.
    + *     trail. This option is ignored, if 'expanded' is non-empty.
    

    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()?

  2. +++ b/core/modules/book/lib/Drupal/book/BookBreadcrumbBuilder.php
    @@ -0,0 +1,104 @@
    +/**
    + * Provides a breadcrumb builder for nodes in a book.
    + */
    +class BookBreadcrumbBuilder implements BreadcrumbBuilderInterface {
    ...
    +   * Constructs the MenuLinkBreadcrumbBuilder.
    

    Actually constructs a BookBreadcrumbBuilder.

  3. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Routing/RouteSubscriber.php
    @@ -81,7 +81,10 @@ public function routes(RouteBuildEvent $event) {
    -          array('_form' => '\Drupal\field_ui\FieldOverview') + $defaults,
    +          array(
    +            '_form' => '\Drupal\field_ui\FieldOverview',
    +            '_title' => t('Manage fields'),
    +          ) + $defaults,
    
    @@ -106,7 +109,10 @@ public function routes(RouteBuildEvent $event) {
    -          array('_form' => '\Drupal\field_ui\DisplayOverview') + $defaults,
    +          array(
    +            '_form' => '\Drupal\field_ui\DisplayOverview',
    +            '_title' => t('Manage display'),
    +          ) + $defaults,
    

    Where is 'Manage form display'?

dawehner’s picture

  1. The stuff is basically removed by removing menu_get_active_breadcrumb() from menu.inc
  2. Good catch, fixed it
  3. Even better, fixed that as well.
larowlan’s picture

  1. +++ b/core/modules/menu_link/menu_link.module
    @@ -195,3 +196,20 @@ function menu_link_maintain($module, $op, $link_path, $link_title = NULL) {
    +
    

    Blank line

  2. +++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php
    @@ -0,0 +1,212 @@
    +            $title = str_replace(array('-', '_'), ' ', Unicode::ucfirst(end($path_elements)));
    

    Do we need a todo regarding _title_callback?

  3. +++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php
    @@ -0,0 +1,212 @@
    +          $links[] = l($title, $route_request->attributes->get('_system_path'));
    

    We have a link generator now.

  4. +++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php
    @@ -0,0 +1,212 @@
    +    $request = Request::create($path);
    

    Anyway we can use the new duplicate helper here? I don't think we can, but worth asking.

  5. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.php
    @@ -58,35 +59,14 @@ function setUp() {
    +    // @todo Revert to 'Configuration' when https://drupal.org/node/2075701 is
    

    This @todo can go now, we've got 'Configuration' in the _title (and the test has been reverted).

  6. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.php
    @@ -164,7 +144,7 @@ function testBreadCrumbs() {
    +      "admin/config/content/formats/manage/$format_id" => Unicode::ucfirst(Unicode::strtolower($format->name)),
    

    This will need a todo to remove once _title_callback is allowed in routing.yml

  7. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.php
    @@ -495,16 +402,19 @@ function testBreadCrumbs() {
    +    // page title, and that the breadcrumb is just the Home linke (because the
    

    linke? typo

  8. +++ b/core/modules/system/system.api.php
    @@ -1398,6 +1351,30 @@ function hook_module_implements_alter(&$implementations, $hook) {
    + *   An array of breadcrumb link a tags, returned by the breadcrumb manager build
    ...
    + *   - builder: the instance of \Drupal\Core\Breadcrumb\BreadcrumbBuilderInterface
    

    > 80 chars

  9. +++ b/core/modules/system/system.api.php
    @@ -1398,6 +1351,30 @@ function hook_module_implements_alter(&$implementations, $hook) {
    + *
    

    extra line? or is that expected with @endcode?

dawehner’s picture

We have a link generator now.

The 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?

Anyway we can use the new duplicate helper here? I don't think we can, but worth asking.

Good idea!

extra line? or is that expected with @endcode?

You are right, see https://drupal.org/node/1354#code

pwolanin’s picture

Order of arguments is wrong.

pwolanin’s picture

Adjusting 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.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Routing/RequestHelper.php
@@ -49,6 +49,11 @@ class RequestHelper {
     $request = $original_request->duplicate($query, $post, $attributes, $cookies, $files, $server);
+    // Request::duplicate() clones the attributes, but we want to re-populate
+    // them from scratch when using this method.
+    if (empty($attributes)) {
+      $request->attributes->replace(array());
+    }
 

Maybe we should document on the documentation of the function?

pwolanin’s picture

Ok, 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.

pwolanin’s picture

fixes the notices - I don't know why the fails are coming and going.

dawehner’s picture

FileSize
55.69 KB

I 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.

pwolanin’s picture

Priority: Normal » Major
FileSize
3.17 KB
53.4 KB

So, 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.

pwolanin’s picture

This change resolves a fatal error on PathLanguageTest, but there is still a DB exception in the debug output.

might be a core bug.

dawehner’s picture

FileSize
989 bytes
53.73 KB

Here is a quick fix

dawehner’s picture

FileSize
944 bytes
53.72 KB

Peter suggested a better fix.

pwolanin’s picture

That's fine, though we don't even need the else

pwolanin’s picture

Priority: Major » Critical

bumping to critical per catch since it's blocking other menu link conversions.

ParisLiakos’s picture

Status: Needs review » Needs work

Looks mostly awesome

  1. +++ b/core/modules/menu_link/menu_link.module
    @@ -195,3 +196,19 @@ function menu_link_maintain($module, $op, $link_path, $link_title = NULL) {
    +    if (($menu_link instanceof MenuLink) && !$menu_link->isNew()) {
    

    check against MenuLinkInterface?

  2. +++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php
    @@ -0,0 +1,228 @@
    +          // @todo Replace with a #type => link render element so that the alter
    +          // hook can work with the actual data.
    +          $links[] = l($title, $route_request->attributes->get('_system_path'));
    

    why dont we use the linkGenetor here?

  3. +++ b/core/modules/system/system.api.php
    @@ -1398,6 +1351,29 @@ function hook_module_implements_alter(&$implementations, $hook) {
    +  $breadcrumb[] = Drupal::l(Drupal::t('Text'), 'example_route_name');
    

    Drupal:t() ?
    there is no such thing afaik.
    just t()

Finally i think PathBasedBreadcrumbBuilder could really use some unit tests

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.69 KB
53.62 KB

why dont we use the linkGenetor here?

Well, 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 :)

dawehner’s picture

FileSize
473 bytes
53.76 KB

This was a stupid mistake.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

I think with the last couple rounds of review this is pretty well polished.

ParisLiakos’s picture

it is either $this->t() or t(). and a forgotten comment

pwolanin’s picture

oops - good catch - that was left from the earlier t() problems.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Didn'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:

  1. +++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php
    @@ -0,0 +1,227 @@
    +        if ($route_request->attributes->get('_legacy')) {
    

    Please reverse the if/else so that converted routes get checked first.

  2. +++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php
    @@ -0,0 +1,227 @@
    +    $request = Request::create($this->request->getBaseUrl() . '/' . $path);
    

    This could duplicate the request now rather than creating it from scratch: https://drupal.org/node/2086077.

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.php
    @@ -160,8 +139,10 @@ function testBreadCrumbs() {
    +      "admin/config/content/formats/manage/$format_id" => Unicode::ucfirst(Unicode::strtolower($format->name)),
    

    Why is the ucfirst/strtolower necessary?

pwolanin’s picture

@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.

catch’s picture

I 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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.52 KB
53.74 KB
  • Rerole
  • Added a followup: #2090293: Fix RequestHelper and added a @todo in the code.
  • This is needed as a temporary workaround until we have title callbacks back, see the @todo in both places:
        // @todo Remove this part once we have a _title_callback, see
        //   https://drupal.org/node/2076085.
    
pwolanin’s picture

route name changed from menu_link_edit to menu.link_edit

dawehner’s picture

This was easier to fix as thought.

pwolanin’s picture

Oops, the actual route for the link is menu.menu_edit, not menu.link_edit.

dawehner’s picture

#126: breadcrumb-2070651-126.patch queued for re-testing.

jibran’s picture

retagging.

dawehner’s picture

Status: Needs review » Needs work
Issue tags: +API change, +MenuSystemRevamp, +Stalking Crell

The last submitted patch, breadcrumb-2070651-126.patch, failed testing.

dawehner’s picture

FileSize
52.39 KB

Now that node/{node} is converted we need the title callback to work, oh wait, this issue is blocked on that one here.

mcrittenden’s picture

Status: Needs work » Needs review

Couldn't tell if dawehner forgot to set to CNR or if that was intentional, so let's let testbot decide.

dawehner’s picture

That was not intentional.

Status: Needs review » Needs work

The last submitted patch, breadcrumb-2070651-132.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
52.4 KB

so, lets reroll this one

Status: Needs review » Needs work

The last submitted patch, breadcrumb-2070651-136.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
6.48 KB
55.18 KB

This adds a title callback for the node view so the breadcrumb is correct on the edit page.

dawehner’s picture

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeView.php
@@ -0,0 +1,35 @@
+
+  /**
+   * The _title_callback for the node.view route.
+   *
+   * @param \Drupal\node\NodeInterface $node
+   */
+  public function pageTitle(NodeInterface $node) {
+    return String::checkPlain($node->label());
+  }

What 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.

  1. +++ b/core/modules/book/lib/Drupal/book/BookBreadcrumbBuilder.php
    @@ -0,0 +1,103 @@
    +            // @todo change this once the node view route is converted, see
    +            // https://drupal.org/node/1987778.
    +            if ($item = menu_get_item($menu_link->link_path)) {
    +              if ($item['access']) {
    +                $links[] = l($menu_link->label(), $menu_link->link_path, $menu_link->options);
    +              }
    +            }
    

    Can we do that already?

  2. +++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php
    @@ -0,0 +1,237 @@
    +          if (!$title) {
    +            // @todo Skip this part once we have a _title_callback, see
    +            //   https://drupal.org/node/2076085.
    +            $title = str_replace(array('-', '_'), ' ', Unicode::ucfirst(end($path_elements)));
    +          }
    

    So we can also skip that part?

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.php
    @@ -160,8 +139,10 @@ function testBreadCrumbs() {
    +    // @todo Remove this part once we have a _title_callback, see
    +    //   https://drupal.org/node/2076085.
    

    Another place

pwolanin’s picture

re: #2 - I think we still want that as a fallback (using the raw path string).

Status: Needs review » Needs work

The last submitted patch, breadcrumb-2070651-138.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
5.06 KB
55.6 KB

Worked 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.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/book/lib/Drupal/book/BookBreadcrumbBuilder.php
@@ -75,13 +85,8 @@ public function build(array $attributes) {
-            // Legacy hook_menu page callback.
-            // @todo change this once the node view route is converted, see
-            // https://drupal.org/node/1987778.
-            if ($item = menu_get_item($menu_link->link_path)) {
-              if ($item['access']) {
-                $links[] = l($menu_link->label(), $menu_link->link_path, $menu_link->options);
-              }
+            if ($this->accessManager->checkNamedRoute($menu_link->route_name, $menu_link->route_parameters)) {
+              $links[] = $this->linkGenerator->generate($menu_link->label(), $menu_link->route_name, $menu_link->route_parameters, $menu_link->options);

yay!

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.phpundefined
@@ -0,0 +1,237 @@
+    // General path-based breadcrumbs. Use the original, aliased path.

This 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'?

+++ b/core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.phpundefined
@@ -0,0 +1,237 @@
+          if (!$title) {
+            // @todo Skip this part once we have a _title_callback, see
+            //   https://drupal.org/node/2076085.

#2076085: Resolve the need for a 'title callback' using the route was committed - can we resolve this @todo?

pwolanin’s picture

@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.

pwolanin’s picture

The 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.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Right 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.

alexpott’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record, +Stalking Crell

Committed 00c06fc and pushed to 8.x. Thanks!

pwolanin’s picture

Title: Introduce a path-based breadcrumb builder, remove the one that's based on {menu_links}. » Change notice: Introduce a path-based breadcrumb builder, remove the one that's based on {menu_links}.
Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs change record, +Approved API change

1st pass change notice: https://drupal.org/node/2098323

pwolanin’s picture

fixing tags

aspilicious’s picture

Status: Fixed » Needs review

Don'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?

mcrittenden’s picture

I suppose the change notice needs work to answer the question in #151, so changing status.

dawehner’s picture

Status: Needs work » Fixed

Don'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?

I am sorry but the new breadcrumb api changed that already, see https://drupal.org/node/2026025/revisions/view/2808661/2855137

plach’s picture

Title: Change notice: Introduce a path-based breadcrumb builder, remove the one that's based on {menu_links}. » Introduce a path-based breadcrumb builder, remove the one that's based on {menu_links}.
jibran’s picture

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: +Prague Hard Problems
xjm’s picture

Issue summary: View changes

Updated issue summary.