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
- Decide if this is an issue worthy of a fix.
- Decide on an approach.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 3243313-4--remove_search_api_base_path_option_from_views.patch | 805 bytes | drunken monkey |
| #2 | 3243313-2--fix_views_exposed_form_block_caching.patch | 739 bytes | drunken monkey |
Issue fork search_api-3243313
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
Comment #2
drunken monkeyThanks 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.)Comment #3
drunken monkeyAny 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.
Comment #4
drunken monkeyI 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.
Comment #6
drunken monkeyCommitted. Thanks again!