OtherFacet condition - non-empty and multiple values

That's not yet implemented but sounds like a great feature!

From the leftover todo's in #12.
- Review.
I'll do that when work's done (so later today). For upcoming patches please also provide interdiffs, I had taken a look at the first patch but have no idea about the changes since.
- Wording, descriptions, etc. - Somebody natively English speaking would be good.
I'm not a native english speaker but I'll have a look as part of the review.
- Styling (?)
Not sure if that'll be needed, this should all be drupal core's task? I'll have a look at how it looks during review so we might have to add some changes our selves or open a core-issue.
- Rename this condition? "Other facet" is not really that helpful. Maybe just "Facets"?
Yeah, that makes sense I guess.
- updating existing settings, if that is an issue
Not while we haven't reached the beta phase, no. We'll start providing upgrade paths as soon as we hit beta. But that won't happen before Search Api reaches beta.

Overall this is looking good, I had some nitpicks though.

1. +++ b/src/Plugin/Condition/OtherFacet.php
   @@ -101,30 +101,73 @@ public static function create(ContainerInterface $container, array $configuratio
   +          '#title' => t('Enable condition'),
   ...
   +          '#title' => t('Condition mode'),
   ...
   +            'presence' => t('Check for facet being present'),
   +            'not_empty' => t('Check for facet being selected / not empty'),
   +            'values' => t('Check for facet being set to specific values'),
   ...
   +          '#title' => t('Values'),
   ...
   +          '#title' => t('Negate condition'),
   

   Let's use $this->t() here instead.

2. +++ b/src/Plugin/Condition/OtherFacet.php
   @@ -142,8 +198,7 @@ public function submitConfigurationForm(array &$form, FormStateInterface $form_s
        return $this->t(
   -      'The facet is @facet also rendered on the same page.',
   -      ['@facet' => $this->configuration['facets']]
   +      'This block depends on @facet facet conditions being met.', ['@facet' => count($this->configuration['facets'])]
        );
   

   I like this change, much clearer!

3. +++ b/src/Plugin/Condition/OtherFacet.php
   @@ -151,51 +206,65 @@ public function summary() {
   +      // If loops allow to skip expensive code. A switch loop on 'condition'
   +      // would work too, but would get complicated as well.
   

   This comment is not very useful, let's remove it or replace it with something that happens in the code.

4. +++ b/src/Plugin/Condition/OtherFacet.php
   @@ -151,51 +206,65 @@ public function summary() {
   +              if (
   +                $result->isActive()
   +                && (
   +                  in_array($result->getRawValue(), $values)
   +                  || in_array($result->getDisplayValue(), $values)
   +                )
   +              ) {
   +                $return = TRUE;
   +              }
   +            }
   

   This looks like it's indented very weird.

Changed title as this patch also covers the problem of multiple facets in #2784945.

Fixed the following warning: Invalid argument supplied for foreach() in Drupal\facets\Plugin\Condition\OtherFacet->evaluate() (line 220 of modules/contrib/facets/src/Plugin/Condition/OtherFacet.php).

Added some help text to the UI.

I tested out the latest patch and its working correctly for me. I was just thinking of a possible case where this function does something odd.

Say I have facet A and facet B with facet B only visible when facet A is selected. Currently if you have an item in facet A selected and and then select an item in facet B then unselect the item in facet A the selection for facet B is still selected but now it is hidden as the nothing is selected in facet A.

I was wondering if this should have be if the parent filter is unselected then all the dependent filters values that are only shown when the parent is selected are unselected.

I hope this sort of made some sense. I will attempt a patch later for this specific set of events but wanted to throw this out there.

Nick

This is looking great. I've been testing this out most of today.

I agree with @ndrake86. It would be awesome if when a user unselects a parent facet, then any dependent facet selections would be cleared. This is one area that threw me off initially and I think clearing the dependent filters when the parent is unselected would make the feature very usable.

Tests 'll need to be fixed for this as well. Otherwise I like the direction here.

Next to tests #19 needs a reroll (due to changes in #2834754).

Rerolled #19.

Let's see what the testbot thinks.

New patch. Previous one failed because of ...

1. Superfluous whitespace in Otherfacet.php
2. Modified parent test class for IntegrationTest

Interdiff:

- if (!is_array($enabled_conditions)) {
+ if (!is_array($enabled_conditions) || empty($enabled_conditions)) {

So, we're trying to rewrite the condition to a processor instead of an actual condition. I'm not sure if we should continue this work first. I'd say we should probably postpone this?

Postponing this on the work in #2796569: New Dependent Facet processor

The issue this was postponed on was committed recently. So work on this can start again.

Since this is now implemented as a processor - this is no longer a condition plugin. This feature still is a something we can improve but the current patch isn't really useable anymore.