Changing exclude criteria and reindexing does not remove newly excluded nodes from search index

I have run into this same problem. Even re-indexing didn't not remove the newly excluded nodes.

I'm working on a fix here and making nice progress. I'm uploading a fail test in this comment to expose the bug.

Given the complexity of the issue though (and my unfamiliarity with the module) I'm planning on adding a bit more to the test. For starters, though, this is the most basic fail test and is a good place to start.

The last submitted patch, 3: search_exclude-exclude-change-bug-3321882-3-FAIL.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Fail looked like this:

There was 1 error:

1) Drupal\Tests\search_exclude\Functional\ExcludedContentTest::testExcludingContent
Behat\Mink\Exception\ResponseTextException: The text "Your search yielded no results" was not found anywhere in the text of the current page.

/var/www/html/vendor/behat/mink/src/WebAssert.php:811
/var/www/html/vendor/behat/mink/src/WebAssert.php:262
/var/www/html/modules/contrib/search_exclude/tests/src/Functional/ExcludedContentTest.php:117
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

which is the bug we were trying to expose. Fix coming soon.

Here's a patch that addresses the bug and also adds test coverage. The key bug is exposed in #3, but the extra test coverage here makes me a lot more confident that I'm not breaking anything.

@danflanagan8 , It looks like you put in tests for the patch. What do we need to make this Reviewed & tested by the community?

I think both code review (the R in RTBC) and manual testing (the T in RTBC) need to be done by one or more people in the Community (the C in RTBC).

This is an old issue, too, so it would be great to turn the patch into an MR so that the tests can run (assuming the project is configured to run tests in Gitlab).

I'm no longer using this module and I don't have a lot of contribution time at the moment. I probably won't be able to offer too much support. The issue has 6 followers, though, so maybe it will get over the finish line yet!

Cheers!

#8 thanks for your help so far 🙏, I am just linking this up with #3149523: [META] Add tests → in the meantime.

On a fresh Drupal 10 install, I repeated the error shown above. I applied the patch.
I tried adding a content type with content already in it after applying the patch. I configured search exclude to exclude this content type.
It worked.
I tried undoing this, and it is now showing up in the search as intended.
I added a content type with no entities and configured search exclude to exclude this content type. I added entities for this content type, and the content didn't show up as intended.
I configured search exclude to allow the above content type to show up in the search, and it worked as intended.
Nothing on the clean Drupal install seemed to be affected.
I tested Advanced Search in the same way as above.
The search exclude takes out the chosen content types as choices in the advanced search as well after I reran cron.

I'm going to do a code review in a bit.

On a fresh Drupal 10 install, I repeated the error shown above. I applied the patch.
I tried adding a content type with content already in it after applying the patch. I configured search exclude to exclude this content type.
It worked.
I tried undoing this, and it is now showing up in the search as intended.
I added a content type with no entities and configured search exclude to exclude this content type. I added entities for this content type, and the content didn't show up as intended.
I configured search exclude to allow the above content type to show up in the search, and it worked as intended.
Nothing on the clean Drupal install seemed to be affected.
I tested Advanced Search in the same way as above.
The search exclude takes out the chosen content types as choices in the advanced search as well after I reran cron.

The SQL looks secure and correct.
I think the comments are sufficient.
The PHP Code looks like it conforms with Drupal coding standards

Let me know if I need to test more things.

#12 Thank you for testing!

Let's put these changes in a Merge Request, so that the pipeline tests it on D11 too.

Will have a merge conflict with 🐛 Address PHPStan Error Active depending on which is merged first as they both change how the searchIndex service is called.

I've converted this into a merge request and updated the tests so that they pass.
Please make sure these tests are actually testing what we need.