Breadcrumbs support

Has something been discussed about this, more than the description?
I would try making a patch for this.
Thx.

We haven't really decided on a direction to go here; so any suggestions that you have would be a good place to start.

I started working on this.
I was thinking on having in the facet source config the options for breadcrumb: Activate breadcrumb, Show homepage and other possible settings !? since in 8, only 1 breadcrumb plugin will apply, so we need to provide some flexibility to the breadcrumb.

How should we display the breadcrumbs when having multiple active items on the same facets?
All grouped under the same crumb or each active items in different crumb?
Home > Path > To > Search page > Facet_name1: Item 1, Item 2 > Facet_name2: > Part1 > Facet_name3: Elem1
or
Home > Path > To > Search page > Item 1 > Item 2 > Part1 > Part2 > Elem1

I think the first option makes more sense. But in a second step, maybe this could also be configurable? If enough people request the other style?

I've started working on a breadcrumb plugin, but than I found that alter hook still exists for breadcrumb and it would make more sense to use a hook that will append the crumbs to currently generated breadcrumb (with any plugin)
Now I'm little stuck on generating the urls for each crumb (to exclude the following crumb filters). I could use the alias . separator . value pattern, but than I see in the default query processor, that more things happens when generating the url (ie hierarchy)
Any suggestion on how I could make use of buildUrls function to get the link? Being an active facet, the result url will have all pram except the active one.
So one solution could be iterating for each facet results trough all query params to remove the ones that will go to next crumbs and than adding the current result filter (which is removed by buildUrls because is active) - but this seems little overdoing and maybe I'm missing a simpler solution. Thx.

@Marcvangend opened an issue for this exact thing recently: #2861586: Make it easier to programmatically generate facet links. Not sure if that would help here? I think it would. For now, I don't really have an idea. Looping trough all params sounds like a bad idea if we keep pretty paths in mind. So manually building the link is something that will break.

I typed this here in the comment box, so not sure if this will work but I think it should

    $facets = $this->facetsManager->getFacetsByFacetSourceId($facetsource_id);
    $this->facetsManager->updateResults($facetsource_id);

    foreach ($facets as $facet) {
      $processed_facets[] = $this->facetsManager->build($facet);
    }


// .. loop over processed facets, get the results and use their urls
foreach ($processed_facets as $pf) {
foreach ($pf->getResults as $r){
if ($r->isActive()) {
$links[] = $r->getUrl();
}
}
}

Thx. This is what I was trying to use, but the thing is that the url that comes from $r->getUrl(); is an url with all the active params + if current result is active, the url will not have its value in query - so it doesn't help too much.
The query has to be constructed while creating the crumbs - when you add a new crumb: add the previous crumb filters + current crumb filters - next crumbs filters.

Maybe we need to add a getUrlWithActiveStillApplied to the result class? That would help I think?

I was thinking more to the processor, to have a buildSingleUrl to give you the url for the current result facet alone, with hierarchy if needed, but without other facet source params - and this function would be called inside buildUrls and afterwards will append other parameters and stuff.

I attach a patch with my current work on this.

This looks great so far, thanks for all the great work! I have some nitpicks but I realise that this is not finished yet.

1. +++ b/facets.module
   @@ -179,3 +181,101 @@ function facets_entity_predelete(EntityInterface $entity) {
   +  $entity_type_manager = \Drupal::service('entity_type.manager');
   

   We should probably add a @var comment for this one as well, as that looks like this is the only one that doesn't have an inline comment.

2. +++ b/facets.module
   @@ -179,3 +181,101 @@ function facets_entity_predelete(EntityInterface $entity) {
   +  if (!empty($facet_sources_definitions)) {
   

   Can we invert this? That should make it possible to reduce a level of nesting, making it easier to know what's going on here :P

   if (empty($facet_source_definitions) {
     return;
   }

3. +++ b/facets.module
   @@ -179,3 +181,101 @@ function facets_entity_predelete(EntityInterface $entity) {
   +          /** @var \Drupal\facets\UrlProcessor\UrlProcessorPluginManager $url_processor_manager */
   +          $url_processor_manager = \Drupal::getContainer()->get('plugin.manager.facets.url_processor');
   

   Let's do this outside the of the loop, where we get the rest of the services and use \Drupal::service instead of ::getContainer, just for consistency.

4. +++ b/facets.module
   @@ -179,3 +181,101 @@ function facets_entity_predelete(EntityInterface $entity) {
   +                  // Clone the result so we can mark it as inactive
   +                  // to be added to the url parameters when calling buildUrls.
   

   Nice, this is really clever. Great!

Attaching a patch for #12 and fixing the issue causing fail test #13

At first glance, this looks great, but we need tests for this.

Uploading a patch including tests (not complete though) - uploading to try with drupal test bot for now.
I'm having an issue testing in my local (if using more than once $this->drupalPlaceBlock - in makes the second block broken) - have you seen this issue before?

I'm having an issue testing in my local (if using more than once $this->drupalPlaceBlock - in makes the second block broken) - have you seen this issue before?

I have seen that before, yeah. See \Drupal\Tests\facets\Functional\IntegrationTest::testMultipleFacets

I fixed that there by doing this:

    // Create 2 facets.
    $this->createFacet('Snow Owl', 'snow_owl');
    // Clear all the caches between building the 2 facets - because things fail
    // otherwise.
    $this->resetAll();
    $this->createFacet('Forest Owl', 'forest_owl', 'category');

Thanks, that was it, so simple. I had spent more than 1 hour trying to debug this thing :(

Attaching the patch with tests.

This patch adds some additional comments + reduces cyclomatic complexity.

Fixing the refactored patch from #27

Pushed, we can always refactor / improve later.