Problem/Motivation

Since #2481453: Implement query parameter based content negotiation as alternative to extensions, any controller returning a render array may be rendered in multiple wrapper formats.

This means that the response for the controller varies by the 'url.query_args:' . MainContentViewSubscriber::WRAPPER_FORMAT cache context.

Proposed resolution

Ensure that the 'url.query_args:' . MainContentViewSubscriber::WRAPPER_FORMAT cache context is present for any response that is rendered by MainContentViewSubscriber, because MainContentViewSubscriber renders it in a different wrapper format depending on that URL query argument.

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Data model changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because this breaks cacheability of responses.
Issue priority Major because it blocks significant other issues.
Prioritized changes The main goal of this issue is performance/cacheability.
Disruption Zero disruption.
CommentFileSizeAuthor
#17 interdiff.txt1.49 KBWim Leers
#17 wrapper_format_url_query_argcache_context-2552013-17.patch22.92 KBWim Leers
#11 interdiff.txt5.09 KBWim Leers
#11 wrapper_format_url_query_argcache_context-2552013-11.patch22.39 KBWim Leers
#2 wrapper_format_url_query_argcache_context-2552013-2.patch17.48 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented
Status: Active » Needs review
FileSize
wrapper_format_url_query_argcache_context-2552013-2.patch17.48 KB
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting commented 
+++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentTranslationUITest.php
@@ -20,7 +20,7 @@ class MenuLinkContentTranslationUITest extends ContentTranslationUITestBase {
-  protected $defaultCacheContexts = ['languages:language_interface', 'theme', 'user.permissions', 'user.roles:authenticated'];
+  protected $defaultCacheContexts = ['languages:language_interface', 'theme', 'url.query_args:_wrapper_format', 'user.permissions', 'user.roles:authenticated'];

Question: So we need to not change that many places, because we use url.query_args in other places already?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented

Yes, or url. url implies url.*, i.e. url.query_args, url.query_args:foo, et cetera. See https://www.drupal.org/developing/api/8/cache/contexts#optimizing

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting commented

Do we need some explicit test coverage for this issue? I mean adding the expected cache contexts is one thing, but having a real case is another.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented

Great question!

I'd say: Yes, of course! — but… we only set the X-Drupal-Cache-Contexts header for CacheableResponseInterface responses. And… only HtmlResponse implements that. Not AjaxResponse (which is what AJAX/dialog/modal all use). So we cannot actually test this. AFAICT this is also why #2472323: Move modal / dialog to query parameters didn't introduce test coverage for this.

So… not sure what to do. Ideas? It feels like this patch's test coverage updates are sufficient for now, until other wrapper formats have cacheable responses?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting commented

To be honest it feels like the code could still be broken in theory now after reading your latest comment. We could for sure write a test wrapper which implements the CacheableResponseInterface

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Ok, let's do that.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented

#2476407: Use CacheableResponseInterface to determine which responses should be cached will also introduce a CacheableJsonResponse that also implements CacheableResponseInterface. So adding a test could be a followup to that issue?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

#9: but we still won't be able to render a render array into a JSON response. Except, of course, we could do that for testing purposes. We'd have to come up with a fairly silly wrapper format anyway.

How about this: I'll adopt the CacheableJsonResponse class from the patch over there in this patch, and whichever patch lands first then makes the other patch smaller :)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
wrapper_format_url_query_argcache_context-2552013-11.patch22.39 KB
interdiff.txt5.09 KB

Et voilà.

Status: Needs review » Needs work

The last submitted patch, 11: wrapper_format_url_query_argcache_context-2552013-11.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

This too is failing with the same random fails: #2552687: Test failures in ConfigFormOverrideTest and ContainerRebuildWebTest on newly spun up testbot instances.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review

Wim Leers queued 11: wrapper_format_url_query_argcache_context-2552013-11.patch for re-testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting commented 
+    $this->drupalGet('');
+    $this->assertCacheContext('url.query_args:' . MainContentViewSubscriber::WRAPPER_FORMAT);
+    $this->drupalGet('', ['query' => [MainContentViewSubscriber::WRAPPER_FORMAT =>  'json']]);
+    $this->assertCacheContext('url.query_args:' . MainContentViewSubscriber::WRAPPER_FORMAT);

If we would have missed that cache context we would have served the wrong kind of context, therefore I think we should have an integration test coverage
which ensures that you can cache two different wrapper formats and get different output.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
wrapper_format_url_query_argcache_context-2552013-17.patch22.92 KB
interdiff.txt1.49 KB

d'oh, of course!

Status: Needs review » Needs work

The last submitted patch, 17: wrapper_format_url_query_argcache_context-2552013-17.patch, failed testing.

Wim Leers queued 17: wrapper_format_url_query_argcache_context-2552013-17.patch for re-testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting commented
Status: Needs review » Reviewed & tested by the community

Cool. Thank you!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 98e2b93 on 8.0.x 
    Issue #2552013 by Wim Leers: Follow-up for #2481453: ensure the 'url....

Status: Fixed » Closed (fixed)

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