Currently the path for the breadcrumb uses the path from the request context of the page; this works fine in a normal use case. Given that we are passing in the route match for the current request anyway could this be used?

This would also allow a breadcrumb to be used in the context of a search result or other view mode of the content by calling the build method with a route match for the request for the given page.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andywhale created an issue. See original summary.

andy_w’s picture

Assigned: andy_w » Unassigned
Status: Needs work » Needs review
FileSize
741 bytes

Status: Needs review » Needs work

The last submitted patch, 2: 2979021-2-use-route-match-for-breadcrumb-path.diff, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Greg Boggs’s picture

How does core do this?

andy_w’s picture

Currently in core the PathBasedBreadcrumbBuilder is also using the context rather than the route_match passed in, but this also seems to be incorrect behaviour, I've found an issue in the core queue (2884217) to resolve this in core.

andy_w’s picture

Status: Needs work » Needs review
Greg Boggs’s picture

Thanks for finding this! I wish crumbs had their own core queue. Can we align our approach to cores a bit more closely maybe?

+ // Set request context from passed in $route_match if route is available.
+ $url = $route_match->getRouteObject() ? Url::fromRouteMatch($route_match) : NULL;
+ if ($url && $request = $this->getRequestForPath($url->toString(), [])) {
+ $route_match_context = new RequestContext();
+ $route_match_context->fromRequest($request);
+ $this->context = $route_match_context;
+ }
+

Greg Boggs’s picture

Status: Needs review » Needs work
andy_w’s picture

Yeah, I like the approach used in that ticket, I've rewritten the patch in line with the core queue patch and it seems to work exactly as expected.

andy_w’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 9: 2979021-9-use-route-match-for-breadcrumb-path.diff, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

andy_w’s picture

Moved the context replacer into its own function and caught the exception for invalid routes, so it just falls back to the current route.

andy_w’s picture

Status: Needs work » Needs review
cameron prince’s picture

Status: Needs review » Reviewed & tested by the community

I've been using this patch on a few projects for several months now and it is working well.

  • diamondsea committed 6657c93 on 8.x-1.x authored by andy_w
    Issue #2979021 by andy_w, Greg Boggs, cameronprince, diamondsea: Route...
diamondsea’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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