I just notice in ExposedFormPluginBase class function getCacheContexts() has no break statement in a loop.
/** @var \Drupal\views\Plugin\views\sort\SortPluginBase $sort_handler */
foreach ($this->displayHandler->getHandlers('sort') as $sort_handler) {
if ($sort_handler->isExposed()) {
$has_exposed_sort_handler = TRUE;
}
}
if ($has_exposed_sort_handler) {
$contexts[] = 'url.query_args:sort_order';
}In my view we don't need to continue iteration after $has_exposed_sort_handler is already TRUE
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | add_break_statement_in_views_exposed_cache-2872180-20.patch | 621 bytes | nikhil_110 |
| #2 | add_break_statement_in_views_exposed_cache-2872180-2.patch | 621 bytes | nikolay shapovalov |
Comments
Comment #2
nikolay shapovalov commentedComment #3
nikolay shapovalov commentedComment #4
armrus commentedFine.
Comment #5
cilefen commentedThank you for the patch.
Even though this is a small change, I would like to see a test for this cache context. ExposedFormTest is close, but it looks like this particular cache context is being folded/optimized away.
Comment #6
nikolay shapovalov commentedI'm not familiar with writing tests, but in ExposedFormPluginBase there is function testExposedSortAndItemsPerPage().
And all cache context was already tested.
Exact match on cache context to url.query_args:sort_order was removed by https://www.drupal.org/node/2351015 (diff)
So changes is already covered by tests, no new tests needed.
Comment #7
cilefen commentedOne can comment out
$has_exposed_sort_handler = TRUE;yet testExposedSortAndItemsPerPage() passes, so there is no specific test:Something else is setting a cache context on url.query_args.
Comment #10
borisson_Based on #5, adding needs tests tag and setting back to needs work.
Comment #20
nikhil_110 commentedAttached patch against drupal 10.1.x