Problem/Motivation

It looks like the full ViewExecutable is serialized in cache through the associated query.

https://git.drupalcode.org/project/search_api/-/blob/8.x-1.x/src/Plugin/views/query/SearchApiQuery.php?ref_type=heads#L285

$this->query->setOption('search_api_view', $view);

Is it normal ?

It causes all handlers to be initialized on the wakeup phase.

Double initialization of the View Bulk Operations handler makes it unusable for example.

Issue fork search_api-3485287

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mably created an issue. See original summary.

mably’s picture

mably commented
Related issues: +#3485284: Incompatibilities with search_api
mably’s picture

mably commented

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

mably’s picture

mably commented

Setting the search_api_view query option to NULL logically disables the serialization of the ViewExecutable object.

Small hack of the cacheSet method:

    $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(),
    ];

And then we could add it back in the cacheGet method:

        try {
          $query = $results->getQuery();
          $query->setOption('search_api_view', $view);
          $this->getQuery()->setSearchApiQuery($query);
        }
        catch (SearchApiException) {
          // Ignore.
        }
mably’s picture

mably commented
Status: Active » Needs review

mably opened merge request !186

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

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Category: Support request » Bug report

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.

mably’s picture

mably commented

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

At least, I don't the problems I had before with Views Bulk Operations, no more double instantiations.

Thanks for the improvements and fixes!

mably’s picture

mably commented
Status: Needs review » Reviewed & tested by the community
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Fixed

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

mably’s picture

mably commented

You're welcome 🙂

  • drunken monkey committed 95d3f516 on 8.x-1.x authored by mably 
    Issue #3485287 by mably, drunken monkey: Fixed Views cache plugins to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.