Problem/Motivation

BreadcrumbManager is the class used for the breadcrumb service. The key methods called on it are addBuilder() and build(). However, it's using the BreadcrumbBuilderInterface which doesn't include addBuilder() and has the irrelevant applies()

Proposed resolution

Create a new interface more specific for this service that includes addBuilder()

Remaining tasks

Add the interface and enforce it.

User interface changes

N/A

API changes

Add a new interface

CommentFileSizeAuthor
#12 2237241-12.patch4.33 KBpwolanin
#12 increment.txt830 bytespwolanin
#9 2237241-9.patch4.22 KBpwolanin
#9 increment.txt4.26 KBpwolanin
#7 2237241-7.patch3.59 KBpwolanin
#7 increment.txt3.71 KBpwolanin
#2 2237241-2.patch2.86 KBpwolanin
#2 increment.txt2.86 KBpwolanin
#1 2237241-1.patch3.18 KBpwolanin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Active » Needs review
FileSize
2237241-1.patch3.18 KB

1st pass patch.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
increment.txt2.86 KB
2237241-2.patch2.86 KB

Tim suggests avoiding an API change by keeping having both interfaces in place for this class.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbServiceInterface.php
@@ -0,0 +1,36 @@
+interface BreadcrumbServiceInterface {

Can we find a beter name, maybe just BreadcrumbManagerInterface
Btw. I would love to extend the BreadcrumbBuilderInterface instead

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

So, to me it doesn't make any sense to extend the other interface - they are really independent.

I *think* this manager was sing the builder interface since someone had an idea it might be chained? I'd be ha[[y to remove the builder interface from it, but Tim thought that was an APi change.

Calling the interface Manager vs. Service doesn't matter much to me, but it's the method that called during the compiler pass that makes it distinct, so it seemed service specific.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

'service' tells you not a single information. On the other hand I don't get 100% why we need to add addBuilder to this interface. This method
is coupled to the container pass, much like the container configuration itself. So if you configure the container different you should kind of know what you are doing.

On the other hand I would be 100% fine to break the API here, ... it is pointless.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

So, I think the addBuilder should be part of the interface since if I substitute a different class in mymodule.services.yml it needs to work properly when used in that compiler pass. Doing that as an exercise for a presentation was what made me realize there was not a useful interface.

In the compiler pass it does:

$manager = $container->getDefinition('breadcrumb');

So it's de-coupled from the concrete class that's behind that service name.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
increment.txt3.71 KB
2237241-7.patch3.59 KB

This renames the interface and documents the exception and makes the manager just implement the manager interface.

Also, there is a small but obvious optimization that we should pre-sort the builders, rather than re-sorting them on every page request.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Needs work

The build() method is not irrelevant. By design, you can swap in a single breadcrumb builder in place of the COR object and everything still works. In fact, the breadcrumb block interacts with the system via the build() method. That is the required method. See http://palantir.net/blog/d8ftw-hacking-core-without-killing-kittens

If we're going to add an interface, we probably want something better than "manager". I'd suggest ChainBreadcrumbInterface, since it's the same Chain of Responsibility as ChainRouter. That should extend BreadcrumbBuilderInterface, since it has to present the same "I'm just a builder" image to the rest of the world.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review
FileSize
increment.txt4.26 KB
2237241-9.patch4.22 KB

ok, here you go.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php
@@ -13,11 +13,13 @@
+ * the addBuilder() method, then uses the highest priority one to build
+ * breadcrumbs when build() is called.

Not really. ... it uses the one which applies but asks in the order of the builders.

Crell’s picture

Crell CreditAttribution: Crell commented

Much nicer! #10 should be addressed but otherwise the code looks fine to me.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
increment.txt830 bytes
2237241-12.patch4.33 KB

how about this?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Fixed

Great, thank you!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Fixed » Reviewed & tested by the community

You meant this, right? :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Not a huge fan of "ChainBreadcrumbBuilderInterface" TBH, but #8 points out that we already do this with ChainRouter, so I guess that's fair.

Committed and pushed to 8.x. Thanks!

  • Commit 1104509 on 8.x by webchick: 
    Issue #2237241 by pwolanin: BreadcrumbManager does not have a proper...

Status: Fixed » Closed (fixed)

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