Working in a custom module I need to alter the breadcrumb for some cases and I use hook_system_breadcrumb_alter(). But I noticed I can't remove a Link from the breadcrumb using the methods ::getLinks() and ::setLinks() because of the logicException in the method ::setLinks() of the same class:
Once breadcrumb links are set, only additional breadcrumb links can be added.
I don't get the reason behind this. Is it cache? Can we have a method in the Drupal\Core\Breadcrumb\Breadcrumb class for this purpose? Or at least explain in the exception a bit more about the reasons this can't be done. I just tried this one that I attached using a comparison between route names and works as I expected.
Call Breadcrumb::setLinks() on a Breadcrumb object that already has links.
::removeLink()logicException in ::getLinks()Agree on what to do. Implement. Possible code is in the merge request. Tests are needed.
None.
None.
It will be possible to remove links from a Breadcrumb object.
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | 2966004-17.patch | 742 bytes | spadxiii |
| #13 | add_remove_link-2966004-8.6.x-dev-6.patch | 913 bytes | arijits.drush |
| #6 | add_remove_link-2966004-8.6.x-dev-5.patch | 840 bytes | prashant.c |
| #6 | interdiff-1-5.txt | 232 bytes | prashant.c |
| add_remove_link.patch | 832 bytes | nachosalvador |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
cilefen commentedComment #3
chanderbhushan commented@cilefen, I have tested your patch its working fine.
Comment #4
rodrigoaguilera@chanderbhushan Thank you for taking the time to test it.
I think the method should be called removeLinkByRouteName(string $routeName) as this the real behaviour of the method.
The logic should be changed accordingly.
Comment #5
rodrigoaguileraThis is the issue were that method was introduced
#2483183: Make breadcrumb block cacheable
Comment #6
prashant.c@rodrigoaguilera
I don't think the function name should be changed as to keep the consistency in the function name
setLinks()addLink()we should keep it as
removeLink.Made a minor addition to the function description.
Comment #7
lauriiiComment #8
rodrigoaguileraMy only reasoning behind changing the name is to keep the functionality of using a route to delete a link in the breadcrumb (it can actually delete many links that use the same route).
A function called removeLink that accepts a link as a parameter should make a more strict comparison other than the route name.
I feel we should choose a path here. whether a real removeLink() method is added that removes exactly the link that you pass or we remove the link using the route name with a change in the method name.
Leaving it as is is a bit confusing.
My opinion is that we should add the ability to remove a link using a route name.
Comment #9
berdirWould also need to investigate why it is like this, but definitely +1, I've had this use case more than once and you currently have to get the links and create a completely new breadcrumb object, in which it is easy to forget adding the existing cacheablity metadata.
Comment #10
borisson_Also +1 on the idea, this seems like a great idea. We still need those tests though. I guess we should wait on the subsystem maintainer review before adding those tests.
Comment #13
arijits.drushWe faced issue which #8 suspected before, if we have more than 1 contextual filter in a view then matching only route removes all links of the view from breadcrumb.
Submitting patch which checks for route + parameter with its order.
Comment #14
rodrigoaguileraI think it looks better now with additional checks.
Still needs tests and a subsystem maintainer review
Comment #15
avpadernoComment #16
dawehnerI was looking at the existing breadcrumb class, it's certainly not a pure value object by any form of standard.
It looks like originally the exception got introduced to avoid weirdnesses with the cacheability metadata, see https://www.drupal.org/project/drupal/issues/2483183#comment-10216507 , but it's funny that you get the same problem when creating your new object as well.
When adding a
removeLinkmethod I wonder whether you'd run into some issues as well.In general my gut feeling somehow would have suggested a filter method instead, given that it might be tricky to know exactly which link you want to remove. Equality of links in this patch is provided by route + route parameters, but there is still the title involved as well. Using a filter method, you would move this responsibility to the caller.
Comment #17
spadxiii commentedAdded a small patch (based on 8.7.x) that adds a filterLinks method to the Breadcrumb class. It accepts a callable, which is used to array_filter the items. The callable gets both the link and the key of each breadcrumb-item.
It still needs tests, so I'll leave the issue on 'needs work'
Comment #22
ivnish#13 works
Comment #27
nachosalvador commentedI've faced this issue again and I've created my own Breadcrumb builder as a workaround.
Hope it helps to someone :)
Comment #30
liam morlandI made a merge request with the patch in #13 plus adding the ability to use
::setLinks()when the links are not empty. Before making tests, I perhaps a maintainer could approve the direction of this.Comment #31
smustgrave commentedIssue summary appears incomplete
Also think test coverage could be expanded to test calling the removeLink function.
Comment #32
liam morland