Warning: Invalid condition field = NULL

@dewalt

I have review and try to apply patch and it's working for me.

So moving this issue to RTBC

Thank you!!

The patch looks fine to me, but I'm having trouble reproducing the issue (and if I can reproduce it manually, then I can add a test) with the instructions provided, i.e.:

Add filter with "empty" condition to any Search Api view.

Looks like it also happens on 0 values of numeric filter.

@dewalt or @hetal.solanki, could either of you add some more details?

Here's what I've tried...

1. Run the steps to set up a local environment, including apply the patch or switching to the issue fork (I used the issue fork but either will work)
2. Edit the test view at /admin/structure/views/view/test_elasticsearch_index_search
3. Under Filter criteria, click the Add button
4. Check Category, click the Add and configure filter criteria button
5. Set Operator to Is empty (NULL)
6. Click the Apply button
7. Click the Save button on the view
8. Go to the test view at /test-elasticsearch-index-search
9. Observe that the results are filtered
10. Go to /admin/reports/dblog
    • Expected behavior: I see the error Invalid condition category = NULL
    • Actual behavior: The error log is empty
11. Edit the test view at /admin/structure/views/view/test_elasticsearch_index_search
12. Under Filter criteria, choose And/Or Rearrange from the drop-button
13. Click Remove in the Test entity - mul changed revisions and data table datasource: Category = row, i.e.: to delete the test filter added in the previous test
14. Under Filter criteria, click the Add button
15. Check Width, click the Add and configure filter criteria button
16. Set Operator to Is equal to, and set Value to 0
17. Click the Apply button
18. Click the Save button on the view
19. Go to the test view at /test-elasticsearch-index-search
20. Observe that the results are filtered
21. Go to /admin/reports/dblog
    • Expected behavior: I see the error Invalid condition category = NULL
    • Actual behavior: The error log is empty

(whoops, wrong status, I should have changed it to "maintainer needs more info"... when you add more steps to reproduce, please change it back to RTBC)

I guess it might be related by some faulty Search API configuration or source data issue, but on the other hand search_api_opensearch module has this change committed since day 0 https://git.drupalcode.org/project/search_api_opensearch/-/blob/3.x/src/...

I don't know how the view made the request.
But when you made a programmatically search_api query, this warning will trigger each time you search contains a condition with a null or false value.

In src/SearchAPI/Query/FilterBuilder.php, the only way to trigger "exists" condition is to have a null value, but this null value triggers a warning earlier.

first the warning :

if (!$condition->getField() || !$condition->getValue() || !$condition->getOperator()) {
          // @todo When using views the sort field is coming as a filter and
          // messing with this section.
          $this->logger->warning("Invalid condition %condition", ['%condition' => $condition]);
        }

then the "exists" only if $condition->getValue() is null :

public function buildFilterTerm(Condition $condition, array $index_fields = []) {
    // Handles "empty", "not empty" operators.
    if (is_null($condition->getValue())) {
      return match ($condition->getOperator()) {
        '<>' => ['exists' => ['field' => $condition->getField()]],
        '=' => ['bool' => ['must_not' => ['exists' => ['field' => $condition->getField()]]]],
        default => throw new SearchApiException(sprintf('Invalid condition for field %s', $condition->getField())),
      };
    }

Moving to needs review so I can take a second look at it.

https://git.drupalcode.org/project/elasticsearch_connector/-/merge_reque... reviewed : works

I will try to review this again in the near future, but off the top of my head, the merge request still needs tests... There are already some tests for this part of the code in tests/src/Unit/SearchAPI/Query/FilterBuilderTest.php.

Tests ultimately benefit you, because tests ensure that future changes to the module (i.e.: by other contributors) don't cause regressions in the functionality that you rely on! Please let us know if you need help writing tests!

Briefly checking the status of this issue after releasing 8.0.0-alpha5...

1. The existing tests are passing — so this merge request didn't cause any regressions
2. But the new code path doesn't have any tests, and it will need a test before I can merge it.
3. The issue summary will need an update
4. A maintainer will need to review the code in detail

Since we're missing tests, I'm moving this back to "Needs work". Thanks for understanding!

Apologies; I forgot about this issue and filed a duplicate - #3584210: Irrelevant warnings when filter value is null, where I ported some other changes from search_api_opensearch. I'm going to close this issue as a duplicate of #3584210: Irrelevant warnings when filter value is null and credit everyone who participated in this issue on that one.

Thanks for your patience with me!