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

Files: 
CommentFileSizeAuthor
#146 breadcrumb-2070651-146.patch55.75 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,665 pass(es).
[ View ]
#146 2070651-142-146.increment.txt1.45 KBpwolanin
#142 breadcrumb-2070651-142.patch55.6 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,388 pass(es).
[ View ]
#142 interdiff.txt5.06 KBdawehner
#138 breadcrumb-2070651-138.patch55.18 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,933 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#138 2070651-136-138.increment.txt6.48 KBpwolanin
#136 breadcrumb-2070651-136.patch52.4 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 58,545 pass(es), 12 fail(s), and 0 exception(s).
[ View ]
#132 breadcrumb-2070651-132.patch52.39 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,191 pass(es), 12 fail(s), and 0 exception(s).
[ View ]
#126 breadcrumb-2070651-126.patch53.74 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch breadcrumb-2070651-126.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#126 2070651-124-126.increment.txt714 bytespwolanin
#124 breadcrumb-2070651-124.patch53.74 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,666 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#124 interdiff.txt714 bytesdawehner
#122 breadcrumb-2070651-122.patch53.74 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,961 pass(es), 83 fail(s), and 2 exception(s).
[ View ]
#122 2070651-120-122.increment.txt1.28 KBpwolanin
#120 breadcrumb-2070651-120.patch53.74 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,660 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#120 interdiff.txt2.52 KBdawehner
#115 breadcrumb-2070651-115.patch53.69 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 59,041 pass(es).
[ View ]
#115 interdiff.txt1.4 KBParisLiakos
#113 breadcrumb-2070651-113.patch53.76 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,962 pass(es).
[ View ]
#113 interdiff.txt473 bytesdawehner
#111 breadcrumb-2070651-111.patch53.62 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,395 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#111 interdiff.txt1.69 KBdawehner
#107 breadcrumb-2070651-107.patch53.72 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,765 pass(es).
[ View ]
#107 interdiff.txt944 bytesdawehner
#106 breadcrumb-2070651-105.patch53.73 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#106 interdiff.txt989 bytesdawehner
#105 breadcrumb-2070651-105.patch53.68 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#105 2070651-104-105.increment.txt2.64 KBpwolanin
#104 breadcrumb-2070651-104.patch53.4 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 59,043 pass(es), 34 fail(s), and 3 exception(s).
[ View ]
#104 2070651-102-104.increment.txt3.17 KBpwolanin
#102 breadcrumb-2070651-102.patch55.69 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,551 pass(es), 34 fail(s), and 3 exception(s).
[ View ]
#100 breadcrumb-2070651-100.patch55.69 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,889 pass(es), 34 fail(s), and 3 exception(s).
[ View ]
#100 2070651-98-100.increment.txt931 bytespwolanin
#98 breadcrumb-2070651-98.patch55.69 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,949 pass(es), 2 fail(s), and 2,151 exception(s).
[ View ]
#98 2070651-96-98.increment.txt3.7 KBpwolanin
#96 breadcrumb-2070651-96.patch54.08 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,702 pass(es).
[ View ]
#96 2070651-95-96.increment.txt1.73 KBpwolanin
#95 breadcrumb-2070651-95.patch53.2 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 59,040 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#95 2070651-94-95.increment.txt861 bytespwolanin
#94 breadcrumb-2070651-94.patch53.2 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
#94 interdiff.txt5.36 KBdawehner
#92 breadcrumb-2070651-92.patch53 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,872 pass(es).
[ View ]
#92 interdiff.txt2.46 KBdawehner
#90 breadcrumb-2070651-90.patch52.58 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,442 pass(es).
[ View ]
#90 2070651-87-90.increment.txt1.78 KBpwolanin
#87 breadcrumb-2070651-87.patch52.61 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,801 pass(es).
[ View ]
#87 interdiff.txt3.42 KBdawehner
#84 breadcrumb-2070651-84.patch50.95 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,413 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#84 interdiff.txt40.03 KBdawehner
#81 breadcrumb-2070651-81.patch46.55 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,741 pass(es), 24 fail(s), and 48 exception(s).
[ View ]
#81 interdiff.txt4.53 KBdawehner
#77 breadcrumbs-by-path-2070651-76.patch46.58 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#77 interdiff.txt13.76 KBdawehner
#76 breadcrumbs-by-path-2070651-76.patch46.88 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,311 pass(es).
[ View ]
#76 2070651-74-76.increment.txt975 bytespwolanin
#74 breadcrumbs-by-path-2070651-74.patch46.78 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,243 pass(es), 2 fail(s), and 16 exception(s).
[ View ]
#74 2070651-72-74.increment.txt4.66 KBpwolanin
#72 breadcrumbs-by-path-2070651-72.patch46.16 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 57,932 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#72 2070651-67-72.increment.txt8.29 KBpwolanin
#67 breadcrumbs-by-path-2070651-67.patch38.2 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,284 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#67 2070651-63-67.increment.txt0 bytespwolanin
#63 breadcrumbs-by-path-2070651-63.patch33.37 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,201 pass(es), 16 fail(s), and 0 exception(s).
[ View ]
#63 2070651-58-63.increment.txt3.97 KBpwolanin
#62 rebase-2070651.58.patch30.06 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#58 breadcrumbs-by-path-2070651.58.interdiff.txt3.89 KBlarowlan
#58 breadcrumbs-by-path-2070651.58.patch30.06 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 58,396 pass(es), 25 fail(s), and 0 exception(s).
[ View ]
#56 breadcrumbs-by-path-2070651.56.interdiff.txt179 byteslarowlan
#56 breadcrumbs-by-path-2070651.56.patch28.83 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 58,139 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
#52 breadcrumbs-by-path-2070651.52.interdiff.txt12 KBlarowlan
#52 breadcrumbs-by-path-2070651.52.patch29 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 56,195 pass(es), 61 fail(s), and 47 exception(s).
[ View ]
#50 breadcrumbs-by-path-2070651-50.patch20.9 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,291 pass(es), 32 fail(s), and 48 exception(s).
[ View ]
#48 2070651-45-47.increment.txt3.26 KBpwolanin
#47 breadcrumbs-by-path-2070651-47.patch115.88 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch breadcrumbs-by-path-2070651-47.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#47 breadcrumbs-by-path-2070651-47.patch115.88 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch breadcrumbs-by-path-2070651-47_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#45 breadcrumbs-by-path-2070651-45.patch20.59 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 57,987 pass(es), 93 fail(s), and 107 exception(s).
[ View ]
#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
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php.
[ View ]
#43 breadcrumbs-by-path-2070651-43.patch20.63 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php.
[ View ]
#40 breadcrumbs-by-path-2070651-40.patch20.59 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 57,754 pass(es), 671 fail(s), and 5,632 exception(s).
[ View ]
#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
FAILED: [[SimpleTest]]: [MySQL] 58,161 pass(es), 39 fail(s), and 2 exception(s).
[ View ]
#30 breadcrumbs-by-path-2070651.30.interdiff.txt5.81 KBlarowlan
#30 breadcrumbs-by-path-2070651.30.patch21.58 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 58,036 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
#27 breadcrumbs-by-path-2070651.27.interdiff.txt15.65 KBlarowlan
#27 breadcrumbs-by-path-2070651.27.patch20.81 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 57,548 pass(es), 432 fail(s), and 399 exception(s).
[ View ]
#24 breadcrumbs-by-path-2070651.24.interdiff.txt3.35 KBlarowlan
#24 breadcrumbs-by-path-2070651.24.patch23.96 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 58,047 pass(es), 40 fail(s), and 0 exception(s).
[ View ]
#18 breadcrumbs-by-path-2070651.18.interdiff.txt2.62 KBlarowlan
#18 breadcrumbs-by-path-2070651.18.patch23.55 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 57,959 pass(es), 56 fail(s), and 2 exception(s).
[ View ]
#15 breadcrumbs-by-path-2070651.15.interdiff.txt1.09 KBlarowlan
#15 breadcrumbs-by-path-2070651.15.patch23.08 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 56,811 pass(es), 104 fail(s), and 74 exception(s).
[ View ]
#14 breadcrumbs-by-path-2070651.14.interdiff.txt17.45 KBlarowlan
#14 breadcrumbs-by-path-2070651.14.patch23.1 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#11 breadcrumb-2070651-10.patch13.46 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,985 pass(es), 28 fail(s), and 0 exception(s).
[ View ]
#11 interdiff.txt990 bytestim.plunkett
#7 breadcrumb-2070651-7.patch13.54 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,711 pass(es), 28 fail(s), and 3 exception(s).
[ View ]
#7 interdiff.txt4.07 KBtim.plunkett
#5 breadcrumb-2070651-4.patch10.08 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,963 pass(es), 35 fail(s), and 1 exception(s).
[ View ]
#5 interdiff.txt815 bytestim.plunkett
#3 breadcrumbs-by-path-2070651.3.interdiff.txt2.47 KBlarowlan
#3 breadcrumbs-by-path-2070651.3.patch10.06 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 50,543 pass(es), 2,968 fail(s), and 2,624 exception(s).
[ View ]
#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
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#1 breadcrumbs-by-path-2070651.1.patch9.84 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#1 Screen Shot 2013-08-21 at 6.41.28 PM.png48.24 KBlarowlan

Comments

Status:Active» Needs review
StatusFileSize
new48.24 KB
new9.84 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

StatusFileSize
new9.86 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new1.16 KB
new35 KB
new25.36 KB

And this one has them in the correct order :)

Screen Shot 2013-08-21 at 6.52.02 PM.pngScreen Shot 2013-08-21 at 6.52.11 PM.png

StatusFileSize
new10.06 KB
FAILED: [[SimpleTest]]: [MySQL] 50,543 pass(es), 2,968 fail(s), and 2,624 exception(s).
[ View ]
new2.47 KB

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

Issue tags:+MenuSystemRevamp
StatusFileSize
new815 bytes
new10.08 KB
FAILED: [[SimpleTest]]: [MySQL] 57,963 pass(es), 35 fail(s), and 1 exception(s).
[ View ]

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

StatusFileSize
new4.07 KB
new13.54 KB
FAILED: [[SimpleTest]]: [MySQL] 57,711 pass(es), 28 fail(s), and 3 exception(s).
[ View ]

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

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

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

StatusFileSize
new990 bytes
new13.46 KB
FAILED: [[SimpleTest]]: [MySQL] 57,985 pass(es), 28 fail(s), and 0 exception(s).
[ View ]

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

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

Issue tags:+API change
StatusFileSize
new23.1 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new17.45 KB

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?

StatusFileSize
new23.08 KB
FAILED: [[SimpleTest]]: [MySQL] 56,811 pass(es), 104 fail(s), and 74 exception(s).
[ View ]
new1.09 KB

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

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?

Status:Needs work» Needs review
StatusFileSize
new23.55 KB
FAILED: [[SimpleTest]]: [MySQL] 57,959 pass(es), 56 fail(s), and 2 exception(s).
[ View ]
new2.62 KB

Moves menu_link.access to a core service

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

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.

Then #2026077: Rewrite menu_get_active_breadcrumb() is definitely duplicate.

StatusFileSize
new23.96 KB
FAILED: [[SimpleTest]]: [MySQL] 58,047 pass(es), 40 fail(s), and 0 exception(s).
[ View ]
new3.35 KB

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.

Status:Needs work» Needs review

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

Status:Needs work» Needs review
StatusFileSize
new20.81 KB
FAILED: [[SimpleTest]]: [MySQL] 57,548 pass(es), 432 fail(s), and 399 exception(s).
[ View ]
new15.65 KB

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new21.58 KB
FAILED: [[SimpleTest]]: [MySQL] 58,036 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
new5.81 KB

As promised

Status:Needs review» Needs work

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

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?

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

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

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.

  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

StatusFileSize
new23.21 KB
FAILED: [[SimpleTest]]: [MySQL] 58,161 pass(es), 39 fail(s), and 2 exception(s).
[ View ]

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.

StatusFileSize
new24.49 KB

Status:Needs work» Needs review
StatusFileSize
new9.32 KB
new20.59 KB
FAILED: [[SimpleTest]]: [MySQL] 57,754 pass(es), 671 fail(s), and 5,632 exception(s).
[ View ]

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.

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

Status:Needs review» Needs work
StatusFileSize
new20.63 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php.
[ View ]
new20.63 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/PathBasedBreadcrumbBuilder.php.
[ View ]

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

StatusFileSize
new1.93 KB

bah

Status:Needs work» Needs review
StatusFileSize
new1.34 KB
new20.59 KB
FAILED: [[SimpleTest]]: [MySQL] 57,987 pass(es), 93 fail(s), and 107 exception(s).
[ View ]

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)

StatusFileSize
new115.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch breadcrumbs-by-path-2070651-47_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new115.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch breadcrumbs-by-path-2070651-47.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

too aggressive in pruning the constructor...

HEAD tests are broken at the moment too...

StatusFileSize
new3.26 KB

StatusFileSize
new20.9 KB
FAILED: [[SimpleTest]]: [MySQL] 58,291 pass(es), 32 fail(s), and 48 exception(s).
[ View ]

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

StatusFileSize
new29 KB
FAILED: [[SimpleTest]]: [MySQL] 56,195 pass(es), 61 fail(s), and 47 exception(s).
[ View ]
new12 KB

hopefully moving in the right direction

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new28.83 KB
FAILED: [[SimpleTest]]: [MySQL] 58,139 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
new179 bytes

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

StatusFileSize
new30.06 KB
FAILED: [[SimpleTest]]: [MySQL] 58,396 pass(es), 25 fail(s), and 0 exception(s).
[ View ]
new3.89 KB

Should keep moving in right direction.

Status:Needs review» Needs work

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

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

StatusFileSize
new30.06 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new3.97 KB
new33.37 KB
FAILED: [[SimpleTest]]: [MySQL] 58,201 pass(es), 16 fail(s), and 0 exception(s).
[ View ]

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

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

Issue summary:View changes

Updated issue summary.

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.

Status:Needs work» Needs review
StatusFileSize
new0 bytes
new38.2 KB
FAILED: [[SimpleTest]]: [MySQL] 58,284 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

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

This should say "Constructs the BookBreadcrumbBuilder" :)

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.

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.

Status:Needs work» Needs review
StatusFileSize
new8.29 KB
new46.16 KB
FAILED: [[SimpleTest]]: [MySQL] 57,932 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Adds an alter hook.

  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.

StatusFileSize
new4.66 KB
new46.78 KB
FAILED: [[SimpleTest]]: [MySQL] 58,243 pass(es), 2 fail(s), and 16 exception(s).
[ View ]

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

StatusFileSize
new975 bytes
new46.88 KB
PASSED: [[SimpleTest]]: [MySQL] 58,311 pass(es).
[ View ]

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.

Issue summary:View changes

Updated issue summary.

StatusFileSize
new13.76 KB
new46.58 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
  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.

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.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new4.53 KB
new46.55 KB
FAILED: [[SimpleTest]]: [MySQL] 58,741 pass(es), 24 fail(s), and 48 exception(s).
[ View ]

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?

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

StatusFileSize
new40.03 KB
new50.95 KB
FAILED: [[SimpleTest]]: [MySQL] 58,413 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
  • 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.

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:

StatusFileSize
new3.42 KB
new52.61 KB
PASSED: [[SimpleTest]]: [MySQL] 58,801 pass(es).
[ View ]

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.

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.

Issue summary:View changes

add

Issue summary:View changes

Updated issue summary.

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

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

StatusFileSize
new1.78 KB
new52.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,442 pass(es).
[ View ]

Some minor cleanup of code comments and an unused variable.

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

StatusFileSize
new2.46 KB
new53 KB
PASSED: [[SimpleTest]]: [MySQL] 58,872 pass(es).
[ View ]
  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.

  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?

StatusFileSize
new5.36 KB
new53.2 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

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

StatusFileSize
new861 bytes
new53.2 KB
FAILED: [[SimpleTest]]: [MySQL] 59,040 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

Order of arguments is wrong.

StatusFileSize
new1.73 KB
new54.08 KB
PASSED: [[SimpleTest]]: [MySQL] 58,702 pass(es).
[ View ]

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.

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

StatusFileSize
new3.7 KB
new55.69 KB
FAILED: [[SimpleTest]]: [MySQL] 58,949 pass(es), 2 fail(s), and 2,151 exception(s).
[ View ]

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.

StatusFileSize
new931 bytes
new55.69 KB
FAILED: [[SimpleTest]]: [MySQL] 58,889 pass(es), 34 fail(s), and 3 exception(s).
[ View ]

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

StatusFileSize
new55.69 KB
FAILED: [[SimpleTest]]: [MySQL] 58,551 pass(es), 34 fail(s), and 3 exception(s).
[ View ]

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.

Priority:Normal» Major
StatusFileSize
new3.17 KB
new53.4 KB
FAILED: [[SimpleTest]]: [MySQL] 59,043 pass(es), 34 fail(s), and 3 exception(s).
[ View ]

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.

StatusFileSize
new2.64 KB
new53.68 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

might be a core bug.

StatusFileSize
new989 bytes
new53.73 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Here is a quick fix

StatusFileSize
new944 bytes
new53.72 KB
PASSED: [[SimpleTest]]: [MySQL] 58,765 pass(es).
[ View ]

Peter suggested a better fix.

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

Priority:Major» Critical

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

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

Status:Needs work» Needs review
StatusFileSize
new1.69 KB
new53.62 KB
FAILED: [[SimpleTest]]: [MySQL] 58,395 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

StatusFileSize
new473 bytes
new53.76 KB
PASSED: [[SimpleTest]]: [MySQL] 58,962 pass(es).
[ View ]

This was a stupid mistake.

Status:Needs review» Reviewed & tested by the community

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

StatusFileSize
new1.4 KB
new53.69 KB
PASSED: [[SimpleTest]]: [MySQL] 59,041 pass(es).
[ View ]

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

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

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?

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

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.

Status:Needs work» Needs review
StatusFileSize
new2.52 KB
new53.74 KB
FAILED: [[SimpleTest]]: [MySQL] 58,660 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
  • 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.

StatusFileSize
new1.28 KB
new53.74 KB
FAILED: [[SimpleTest]]: [MySQL] 58,961 pass(es), 83 fail(s), and 2 exception(s).
[ View ]

route name changed from menu_link_edit to menu.link_edit

Issue tags:-MenuSystemRevamp, -Stalking Crell
StatusFileSize
new714 bytes
new53.74 KB
FAILED: [[SimpleTest]]: [MySQL] 58,666 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

This was easier to fix as thought.

StatusFileSize
new714 bytes
new53.74 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch breadcrumb-2070651-126.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

retagging.

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

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

StatusFileSize
new52.39 KB
FAILED: [[SimpleTest]]: [MySQL] 58,191 pass(es), 12 fail(s), and 0 exception(s).
[ View ]

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

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.

That was not intentional.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new52.4 KB
FAILED: [[SimpleTest]]: [MySQL] 58,545 pass(es), 12 fail(s), and 0 exception(s).
[ View ]

so, lets reroll this one

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new6.48 KB
new55.18 KB
FAILED: [[SimpleTest]]: [MySQL] 58,933 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

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

+++ 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

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.

Status:Needs work» Needs review
StatusFileSize
new5.06 KB
new55.6 KB
PASSED: [[SimpleTest]]: [MySQL] 58,388 pass(es).
[ View ]

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.

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!

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?

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

StatusFileSize
new1.45 KB
new55.75 KB
PASSED: [[SimpleTest]]: [MySQL] 58,665 pass(es).
[ View ]

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.

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.

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

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

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

fixing tags

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?

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

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

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

Status:Fixed» Closed (fixed)

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

Issue tags:+Prague Hard Problems

Issue summary:View changes

Updated issue summary.