HTML filter discards conditions that contain a NULL (IS EMPTY/IS NOT EMPTY) as value

Problem/Motivation

First. Sorry, this is going to be a long explanation because it took some long afternoon time to understand/debug the issue.

The problem:

When creating a Search API driven Views and adding a Filter Query that uses the IS EMPTY or IS NOT EMPTY operator, the resulting query, when executed, never contains the field being Filtered against because the condition does not persist until where the search is executed. This was tested using Search API Solr initially (and I started my debugging route from there not finding any issue at that level).

After chasing the condition and condition groups and every method involved starting from the Views Query Plugin until the execution of the query I found out that when a Post Processor Filter (e.g HTML) was enabled and the Filtered against Field was configured for that processor (::testField == true), the processor was also affecting Filter Query generation and discarding any Values coming with a NULL value. This seems like a bug. Also disabling the processor is not an option if during Index time one really wants to remove HTML tags. (or do any other pre index work).

The offending code at https://git.drupalcode.org/project/search_api/-/blob/8.x-1.x/src/Process... is

 /**
   * Preprocesses the query conditions.
   *
   * @param \Drupal\search_api\Query\ConditionInterface[]|\Drupal\search_api\Query\ConditionGroupInterface[] $conditions
   *   An array of conditions, as returned by
   *   \Drupal\search_api\Query\ConditionGroupInterface::getConditions(),
   *   passed by reference.
   */
  protected function processConditions(array &$conditions) {
    $fields = $this->index->getFields();
    foreach ($conditions as $key => &$condition) {
      if ($condition instanceof ConditionInterface) {
        $field = $condition->getField();
        if (isset($fields[$field]) && $this->testField($field, $fields[$field])) {
          // We want to allow processors also to easily remove complete
          // conditions. However, we can't use empty() or the like, as that
          // would sort out filters for 0 or NULL. So we specifically check only
          // for the empty string, and we also make sure the condition value was
          // actually changed by storing whether it was empty before.
          $value = $condition->getValue();
          $empty_string = $value === '';
          $this->processConditionValue($value);

          // Conditions with (NOT) BETWEEN operator deserve special attention,
          // as it seems unlikely that it makes sense to completely remove them.
          // Processors that remove values are normally indicating that this
          // value can't be in the index – but that's irrelevant for (NOT)
          // BETWEEN conditions, as any value between the two bounds could still
          // be included. We therefore never remove a (NOT) BETWEEN condition
          // and also ignore it when one of the two values got removed.
          // Processors who need different behavior have to override this
          // method.
          $between_operator = in_array($condition->getOperator(), ['BETWEEN', 'NOT BETWEEN']);
          if ($between_operator && (!is_array($value) || count($value) < 2)) {
            continue;
          }

          if ($value === '' && !$empty_string) {
            unset($conditions[$key]);
          }
          else {
            $condition->setValue($value);
          }
        }
      }
      elseif ($condition instanceof ConditionGroupInterface) {
        $child_conditions = &$condition->getConditions();
        $this->processConditions($child_conditions);
      }
    }
  }

What is wrong with this code?

A few things:

This is meant to detect if a we are actually in the presence of an empty string or not

  // We want to allow processors also to easily remove complete
  // conditions. However, we can't use empty() or the like, as that
  // would sort out filters for 0 or NULL. So we specifically check only
  // for the empty string, and we also make sure the condition value was
  // actually changed by storing whether it was empty before.
 $empty_string = $value === '';

so for $value == NULL or $value == 0 $empty_string will be FALSE. (so far so good, still it does not keep track of the fact that originally this was a NULL or a 0)

$this->processConditionValue($value); 
> will pass via reference $value around and will eventually convert the NULL or a 0 into string. NULL will become a ''; In the case of the HTML processor being enabled this is already a bit miss leading, the documentation and help text says nothing about a Query/Filter time stripping of HTML Tags but since I'm focusing on NULL that is not the problem.

This in turn is the actual bug:

<?
    if ($value === '' && !$empty_string) {
            unset($conditions[$key]);
    }
    else {
            $condition->setValue($value);
    }

$value === '' && !$empty_string will basically remove the condition carrying the NULL value and also assign back the empty string if it was originally an empty string.

What we want here is that NULL is preserved or a 0 (as explained In the in code comment) and that the value is set back into the condition only if:
A.- The processed Value is !== to the original one (even for readability) and !== ''
B.- Unset conditions that started with a string/numeric value and ended being === ''
C.- Keep conditions that started being ==='' whatever the processed value is
D.- Keep conditions that started being NULL and ended being ===='' but do not assign the value back (keep the NULL)

My understanding of the coded as it is right now.
1.- If the condition had initially an empty string as value the code sets the same empty string back.
2.- If the ::processConditionValue takes the passed condition and transforms it into a '' and it was not an empty string before (NULL, an HTML tag without content if that is the processor enabled here), it will unset the condition.

Steps to reproduce

- Enabled HTML Processor filter for any given Solr Field
- Create a Views using Search API (Solr) as Content Source and add a Filter that uses IS NOT EMPTY for that field
- Check your Solr Logs or enable the Solr Debug Module or add a debug statement at ::processConditions and see how the Condition is being - unset. You will notice that the Condition is not present at all in the final query.
- Uncheck the field from the HTML Processor filter
- Check again, the NULL is passed correctly into the final query and it produces a correct e.g +(*:* -sm_your_field:[* TO *]) Filter

Proposed resolution

Not sure yet which use cases were covered already in current logic and what special case is being managed by letting empty string pass but not empty strings resulting from processing, but changes that could be done are:
- Since all condition values will be casted string anyways (including a 0) by ::processConditionValue and then ::process we only need to give NULL a special treatment.
e.g adding a strict check against === NULL to identify an original NULL and just let it pass through as it is if $value after processing is ==='', if not (let's say a special post processor in its own ::process implementations converts all NULLs into the "SUPERNULL" string use the later.

If we do fix just the initial checking logic and keep the current (setValue('') unmodified this +(*:* -sm_your_field:[* TO *]) will become +(-sm_your_field:"") which is not the Filter we are looking for.

Sorry for the long explanation. I can propose a diff/make a pull here but want to be sure I'm understanding the logic correctly and why an original empty is kept but an empty as a result of processing the value not. And if there are other use cases/edge cases not covered in my proposed solution. E.g is there anyway that a 0 will not become a '0'? should we also check for that?

Remaining tasks

Discuss the original code and see if my understanding of the issue is correct. Get feedback on the proposed solution. I can do the code. Thanks a lot.

Thanks a lot again.