Problem/Motivation

So this may be considered a feature, rather than a bug, but not in my use-case.

in \Drupal\search_api\Plugin\views\query\SearchApiQuery::build there's the following code:

    // If the View and the Panel conspire to provide an overridden path then
    // pass that through as the base path.
    if (($path = $this->view->getPath()) && strpos(Url::fromRoute('<current>')->toString(), $path) !== 0) {
      $this->query->setOption('search_api_base_path', $path);
    }

Because of the call to Url::fromRoute('<current>')->toString() when this is rendered the 'route' cache context is added to the render context.

This is fine for 99% of views, since they probably only ever exist on a single route. But if you happen to use an exposed form, and place that as a block on other pages on your site then the render cache for this view is suddenly segmented per route. This can cause a significant performance regression.

I've looked and I can't see a single usage of the option search_api_base_path that's added to the query. It looks like it was added in c6ff38 but then the facet blocks it was added for no-longer exist.

Steps to reproduce

As above.

Proposed resolution

Remove this code, or maybe to preserve BC there could be some config whereby it's still added for existing installs, but for new installs of search_api it isn't.

Remaining tasks

  1. Decide if this is an issue worthy of a fix.
  2. Decide on an approach.

Issue fork search_api-3243313

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:

Comments

Steven Jones created an issue. See original summary.

drunken monkey’s picture

Component: General code » Views integration
Status: Active » Needs review
Issue tags: +Needs change record
StatusFileSize
new739 bytes

Thanks for reporting this problem!
I checked back with the Facets maintainers and this option is apparently indeed unused. However, we can’t be 100% sure no-one is using it, of course.

Your suggestion of introducing a hidden variable for this sounds like a potential solution, but a bit too much effort for such a simple change, and an option that probably no-one cares about. But, maybe better to err on the safe side in any case?
Another potential workaround for now might be the attached patch – could you please try that? (Note: Unfortunately, MRs cannot be tested in this project until #3190024: Problem with test dependencies when testing issue forks is resolved.) However, I guess that might still disrupt the site of anyone using this, as the cache might then lack a required context (if the option does indeed depend on the route).

In any case, though, I think we should add a change record to just remove this option for our 2.0.0 version. (Unfortunately, though, I don’t think it’s possible in this case to trigger a deprecation notice – except, I guess, if we hack it into the \Drupal\search_api\Query\Query::getOption() method itself.)

drunken monkey’s picture

Any input on my patch?

Anyways, on reflection, I think just removing the option would also be feasible. I don’t think it’s really covered by our BC policy, and chances are pretty good that, indeed, no-one is using it. To be on the safe side, I also asked around on Slack whether anyone needs it. Let’s see what comes back.

drunken monkey’s picture

I had no feedback from anyone actually using the option, so let’s go ahead with just removing it.

Attached is a patch, and here is a change record.

drunken monkey’s picture

Status: Needs review » Fixed
Issue tags: -Needs change record

Committed. Thanks again!

Status: Fixed » Closed (fixed)

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