The full ViewExecutable is serialized in cache, is it normal?

Here is the code that sets the cache:

    $view = $this->getView();
    $query = $this->getQuery();
    $data = [
      'result' => $view->result,
      'total_rows' => $view->total_rows ?? 0,
      'current_page' => $view->getCurrentPage(),
      'search_api results' => $query->getSearchApiResults(),
    ];

https://git.drupalcode.org/project/search_api/-/blob/8.x-1.35/src/Plugin/views/cache/SearchApiCachePluginTrait.php?ref_type=tags#L118-125

Resetting the search_api_view query option seems to disable the serialization of ViewExecutable.

Small hack below:

    $view = $this->getView();
    $query = clone $this->getQuery();
    $query->setOption('search_api_view', NULL);
    $query->getSearchApiQuery()->getOriginalQuery()->setOption('search_api_view', NULL);
    $data = [
      'result' => $view->result,
      'total_rows' => $view->total_rows ?? 0,
      'current_page' => $view->getCurrentPage(),
      'search_api results' => $query->getSearchApiResults(),
    ];

drunken monkey → made their first commit to this issue’s fork.

Thanks a lot for reporting this problem and already providing an MR!

You’re right, caching the view object here makes no sense at all and can lead to various unintended consequences. We also already remove that option when storing the query as part of a saved search in the Saved Searches module (see here) for the same reason. So removing it here seems like a very good idea.

Looking at the cache contents for SQL views and at \Drupal\views\Plugin\views\cache\CachePluginBase::cacheSet(), though, it seems like they even avoid caching the result entities, which might not be as much a potential source of bugs but might bloat the cache size considerably. So doing something similar in our cache plugins might be a good next step – probably in a follow-up issue, though.

Regarding the concrete code in your MR I spotted a potential problem when the $expire === 0 line is true, since the search_api_view option values would then not be restored. I now just moved all that code down to right before the $this->getCacheBackend()->set() call and threw in a try/finally for good measure to make extra-sure this temporary change will always be reverted. (It would also have worked without moving the $data initialization down, too, but I found it clearer this way and there doesn’t actually seem to be a reason it was initialized so far up in the method body until now.)

One final question: I see that in cacheGet() you don’t set the search_api_view option on the original query. I assume that’s because that is always null at this point, or what’s the reason? (In hindsight, it seems an oversight that there is no way to tell whether the $originalQuery property is actually set on a query, but I guess this issue is the wrong place to add it.)

Otherwise, please just test/review the modified MR and I can merge it.
In any case, thanks again, and sorry for the delay.

Hi @drunken-monkey, tested locally, seems to work fine.

Thanks for the improvements and fixes!

Great to hear, thanks a lot for testing!
Merged.
Thanks again!

You're welcome 🙂