Problem/Motivation
Follow up of #2381277: Make Views use render caching and remove Views' own "output caching"
+++ b/core/modules/views/src/Plugin/views/argument_default/QueryParameter.php
@@ -95,7 +95,7 @@ public function isCacheable() {
+ return ['url.query_args.' . $this->options['query_param']];
'url.query_args.' -> 'url.query_args:'
Proposed resolution
Remaining tasks
User interface changes
API changes
Beta phase evaluation
| Issue category | Task, because cache contexts are not as specific as they could be. |
|---|---|
| Issue priority | Normal - Major, because it affects cacheability. |
| Prioritized changes | The main goal of this issue is Performance. It is a follow-up to a critical (and technically part of the critical scope). |
| Disruption | Non-disruptive, internal changes only. |
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 2501905-2.patch | 1.68 KB | olli |
Comments
Comment #1
fabianx commentedComment #2
olli commentedHere's a patch with unit test. I tried to test this manually by looking at the headers but it seems to send only url context. I think that is due to this code in ArgumentPluginBase:
If I remove the line
$contexts[] = 'url';, I see the correct context in response headers.Comment #3
dawehnerMh that unit test kinda doesn't really test anything, don't you think so? It feels better to go with an integration test instead?
Do you have an oppinion about it?
Comment #4
olli commentedI agree. What kind of integration test you think would be good?
Comment #5
wim leersTo test: a view that uses this plugin, verifies the presence of this cache context, and that accessing a URL with *another* query argument still results in a render cache hit.
Comment #6
wim leersComment #7
olli commentedThanks, @Wim Leers. Maybe someone else can give this a shot because I don't see how to do that because that 'url' context seems to "eat" my more specific url.query_args context (see my failed attempt in comment #2). Is there similar tests to look at to "assert presence of this cache context" and to "assert a render cache hit"?
While trying to test this manually in #2 I got a "Notice: Array to string conversion in Drupal\Core\Render\RenderCache->createCacheID() (line 291 of core/lib/Drupal/Core/Render/RenderCache.php)." because the QueryArgsCacheContext::getContext() method does not return a string, but an array when using a query string like '?value[0]=a&value[1]=b'. Should we fix that here too or in another issue?
Comment #8
dawehnerIt seems to be that something else requires the more specific cache context.
If you want though, you can setup a bare rendering:
With that you can pretty much achieve the same, without all the problems with a full blown drupal.
Comment #9
olli commented@dawehner: I think the "something else" is ArgumentPluginBase, filed #2514784: \Drupal\views\Plugin\views\argument\ArgumentPluginBase should allow subplugins to specify a more specific url cache context.
#2509436: Make CachePluginBase::generateResultsKey() to pass valid data to CacheContextsManager::convertTokensToKeys could fix the notice in #7.2
Comment #10
wim leersComment #12
dawehnerIs this basically postponed on the other issues you listed above?
Comment #25
smustgrave commentedThis task still relevant?