Using the Index hierarchy processor, when a child item is active, the parent item is not present in the breadcrumb.

It seems that in facets_system_breadcrumb_alter() at line 295 of facets.module, we just iterate on active parents and not on active children too. So when a child is active (and then the parent is inactive), we miss the parent and don't place it into the breadcrumb.

The patch attached seems solving that and place the inactive parent item in the breadcrumb. We need also to place the child in the breadcrumb, I look foward to do it this week.

CommentFileSizeAuthor
#10 missing_parent_item_in-2917854-10.patch2.58 KBborisson_
#10 missing_parent_item_in-2917854-10--testonly.patch1.99 KBborisson_
#8 missing_parent_item_in-2917854-8.patch599 bytesborisson_
#4 facet_breadcrumb_detect_active_child-2-composer.patch600 bytesKarimB
#3 facet_breadcrumb_detect_active_child-2.patch692 bytesKarimB
facet_breadcrumb_detect_active_child.patch693 bytesKarimB
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

KarimB created an issue. See original summary.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and at Dazzle commented
Issue tags: +Needs tests

Can we use || instead of or in this patch? This will be a new coding standard in the future (see #2890524: Disallow `or` or `and` in Logical Operators (use `||` or `&&`)).

I'd also love to see test coverage for this, not sure if we should do this in BreadCrumbIntegrationTest or in HierarchicalIntegrationTest though, I think the first makes more sense?

If you don't know how to - or can't get around to writing the tests I don't mind having a go at it.

KarimB’s picture

KarimB CreditAttribution: KarimB commented
FileSize
facet_breadcrumb_detect_active_child-2.patch692 bytes

Sure we can @borisson_ ... Here is the patch with || .

Yes, it makes more sense to test it with BreadCrumbIntegrationTest but I'm not an expert writing tests :)

KarimB’s picture

KarimB CreditAttribution: KarimB commented
FileSize
facet_breadcrumb_detect_active_child-2-composer.patch600 bytes

Now the patch without the complete route, so we can use it with composer.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and at Dazzle commented
Status: Active » Needs review

Setting to needs review so this shows up more prominently on my radar. Also to see if it breaks any of the current tests.

Status: Needs review » Needs work

The last submitted patch, 4: facet_breadcrumb_detect_active_child-2-composer.patch, failed testing. View results

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and at Dazzle commented
Version: 8.x-1.0-alpha11 » 8.x-1.x-dev
Issue tags: +Needs reroll

Patch doesn't apply on head anymore.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and at Dazzle commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
missing_parent_item_in-2917854-8.patch599 bytes

Patch was corrupt and didn't apply, rerolled.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and at Dazzle commented
Status: Needs review » Needs work

Back to NW for tests.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and at Dazzle commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
missing_parent_item_in-2917854-10--testonly.patch1.99 KB
missing_parent_item_in-2917854-10.patch2.58 KB

The last submitted patch, 10: missing_parent_item_in-2917854-10--testonly.patch, failed testing. View results

  • borisson_ committed cb9e730 on 8.x-1.x 
    Issue #2917854 by borisson_, KarimB: Missing parent item in breadcrumb...
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and at Dazzle commented
Status: Needs review » Fixed

Committed and pushed, thanks!

Status: Fixed » Closed (fixed)

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

kebne’s picture

kebne CreditAttribution: kebne commented

This issue might persist since not all parents are shown in my breadcrumb trail. Running ver 8.x-1.4.

Facets shows:
Clothes
-Male
--Jackets

Crumb shows:
Home > Store > Clothes > Jackets

Should be:
Home > Store > Clothes > Male > Jackets

Another example:
Facet shows:
Spare parts
-Car parts
--Engine
---Engine Belt
----Timing Belt

Crumb shows:
Home > Store > Spare parts > Timing Belt