Index items only at end of page request

I like this, it's a great idea, both for perceived performance and for other tricky problems. However, I think this implementation is a little bit different than how I'd like to see it.

I would love to see this get moved to an evensubcriber that subscribes to a PostResponseEvent. If I understand this correctly - it's a little bit earlier in the process, but that doesn't really matter. Because an event subscriber is a lot easier to test, we could write a unit and/or kernel test for this behavior - making it more robust.

Not sure if that is enough to set it back to NW though.

Great work! Let's get this in.

FYI, this broke paragraph tests, because we install a demo module with search integration and default content in the test process. Then we access the site and check the search in the test. But the test "page request" is still ongoing and it hasn't been indexed yet.

Not complaining, just leaving this here in case someone else did something similar.

+++ b/src/Utility/PostRequestIndexing.php
@@ -0,0 +1,108 @@
+   */
+  public static function getSubscribedEvents() {
+    $events[KernelEvents::TERMINATE][] = ['onKernelTerminate'];
+
+    return $events;

There is actually a better way to do this.

This event is *always* called on every request (except anonymous page cache hits), so we have to instantiate it just to find out that there is nothing to do.

There is a feature specifically to allow to call a method on services *if* they have been instantiated.

See \Drupal\Core\EventSubscriber\KernelDestructionSubscriber. You add the needs_destruction tag to the service definition and implement \Drupal\Core\DestructableInterface

Setting this back to active based on @Berdir's feedback, we should implement the DestructableInterface.

I don't know if it helps, but I stumbled across this issue for similar reasons. Tests in my project started failing when items were not in the index. I have "index items immediately" set. It sounds like the items are not indexed because a page request is not creating them, the tests are, is that accurate?

Isn't the problem with this approach though, that now the index process becomes request driven, rather than the hook for node_access_records_alter? Is it possible that nodes could be created without a request?

I applied this patch and my tests are still failing. I will investigate further this week, but just wanted to give some early feedback. I can't totally tell from the comments if this is still expected...? Are you saying my tests that are creating content need to use this new trait in order to run correctly?

I just sort of think that if I choose "Index items immediately" option, it's a little misleading. It should read "Index items immediately (that are created via admin screens)."

Yeah, that was more or less unavoidable, of course. But we provide PostRequestIndexingTrait for that, using that you should easily be able to fix all tests.

No, the trait didn't work for me, I had to run it in the install hook of my demo module. That's because I created my content in a hook_install() and the test also installed another module after that, which builds a new container and then the new service didn't have the content-to-be-indexed anymore.

A safer version would be to just index *all* content in that test.

@dpagini: It is not supposed to fix your case, and yes, you will have to adjust your test. Tests are in general not covered by the backwards-compatibility policy, also not by Drupal core. It's fairly common that tests need to be updated in contrib/custom modules after updates of other modules.

I don't necessarily mind changing the test if I have to, but this does really change the behavior that was in place prior to this original commit. I just want to make sure the tests don't hint at a larger issue... Before this commit went in, creating a node programmatically (via a cron job maybe as an example?), would have been indexed. After this update, those programmatic nodes would no longer be indexed, right? I haven't dug in yet, but at what point DO those types of nodes get indexed? Do they ever? Or is that what the trait is intended for? Should that be indicated somewhere for other users maybe?

I have Behat tests failing after this went in, for the same reason it was described in #14. What I did is I've implemented an @AfterStep hook. Unfortunately I couldn't use PostRequestIndexingTrait because in my FeatureContext class I cannot use the trait. I'm receiving

PHP Fatal error: Trait 'Drupal\Tests\search_api\Kernel\PostRequestIndexingTrait' not found

This is, probably, because of the special autoloading that unit tests have.

Here's my context hook:

  /**
   * @AfterStep
   */
  public function indexEntities() {
    \Drupal::service('search_api.post_request_indexing')->destruct();
  }

I have the same Behat problem: all tests about Search API driven overview pages fail.

I got the following error while using the fix by claudiu.cristea
Fatal error: Call to undefined method Drupal\search_api\Utility\PostRequestIndexing::destruct() (Behat\Testwork\Call\Exception\FatalThrowableError)

This works for me:

public function indexEntities() {
  \Drupal::service('search_api.post_request_indexing')->onKernelTerminate();
}

@zero2one, you should use the patch from #19.

> I just want to make sure the tests don't hint at a larger issue...

I think it is fine.

The thing with tests is that they are a continuous process that remains active for the whole test run. So the destruct/indexing never happens ar at least not early enough for the test steps. Indexing on cron for example should still work because running cron (even in drush) should terminate the kernel which would then in turn cause those methods to be called.

And the worst case is just that the content will be picked up the next time cron runs and it will be indexed then as if the immediate-index checkbox isn't enabled. I think :)

Looks solid, let's get this in!