Problem/Motivation

"url.query_args" cache context gets added to every page, making our caching tables extremely big without any need.
We noticed this problem because the marketing department started adding tracking id's to url's for facebook/google/twitter/linkedin and every one of those started generating a new entry into the cache table for alle pages, regardless of there being a webform on them.

We tracked the code down to: (webform.module)

...
/**
 * Implements hook_menu_local_tasks_alter().
 */
function webform_menu_local_tasks_alter(&$data, $route_name, RefinableCacheableDependencyInterface $cacheability) {
...
  // Allow webform query string parameters to be transferred
  // from canonical to test URL.
  $route_names = [
    'entity.webform.canonical', 'entity.webform.test_form',
    'entity.node.canonical', 'entity.node.webform.test_form',
  ];
  if (in_array($route_name, $route_names)) {
    if ($query = \Drupal::request()->query->all()) {
      foreach ($route_names as $route_name) {
        if (isset($data['tabs'][0][$route_name])) {
          $url =& $data['tabs'][0][$route_name]['#link']['url'];
          $url->setOption('query', $query);
        }
      }
    }

    // Query string to cache context webform canonical and test routes.
    $cacheability->addCacheContexts(['url.query_args']);
  }
}
...

Steps to reproduce

Enable webform module and visit any page with any query argument in the url. This will generate a new entry in the page_cache. Regardless of being logged in or not.

Proposed resolution

Not sure, since i don't currently understand what this code is supposed to do...
Can the "addCacheContexts" be deleted? Or does it need to move up so it is only added if "$query = \Drupal::request()->query->all()" actually returns a result? Or is it an admin-only feature.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#7 interdiff-3201136-2-7.txt419 bytesjrockowitz
#7 3201136-7.patch1.88 KBjrockowitz
#2 3201136-2.patch1.88 KBjrockowitz

Comments

weseze created an issue. See original summary.

jrockowitz’s picture

jrockowitz commented
Version: 8.x-5.24 » 8.x-5.x-dev
Status: Active » Needs review
StatusFileSize
new 3201136-2.patch1.88 KB

This admin only behaviors and the attached patch refactors the code to only apply this behavior with $cacheability->addCacheContexts(['url.query_args']); when a user can test a webform.

Status: Needs review » Needs work

The last submitted patch, 2: 3201136-2.patch, failed testing. View results

weseze’s picture

weseze commented

This fixes my caching issue and everything still seems to work fine.

However the cache context tag is not added on a page that contains the webform (through a webform reference field through paragraphs) when I'm logged in as a content editor and allowed to use the "Test" functionality.
Is this something the webform module should do out-of-the-box? Is this out of scope and should that be handled in the reference field / paragraphs module. (possibly a cache context bubbling-up issue?) Or am I just wrong in assuming the cache tag should be present when there is testable webform on a page?

If this is out-of-scope (or if I am wrong) you can consider this "RTBC" ;)

EDIT: perhaps it is good enough to simply add "url.query_args:_webform_test" ? That would prevent any other url query string from causing cache entries because of webforms.

jrockowitz’s picture

jrockowitz commented

This behavior also allows any prepopulated input and variant query param to be transferred between the View and Test tab.

jrockowitz’s picture

jrockowitz commented
Related issues: +#3063287: Allow webform query string parameters to be transfered from canonical to test URL.

Here is the original feature request #3063287: Allow webform query string parameters to be transfered from canonical to test URL.

jrockowitz’s picture

jrockowitz commented
Status: Needs work » Needs review
StatusFileSize
new 3201136-7.patch1.88 KB
new interdiff-3201136-2-7.txt419 bytes

The issue from #4 was a typo.

jrockowitz’s picture

jrockowitz commented
Status: Needs review » Fixed

Because I want able to easily fix the broken test and resolve #4, I decided to commit the patch. Please reopen the ticket if there are further issues.

  • jrockowitz authored 8d6e046 on 8.x-5.x 
    Issue #3201136 by jrockowitz: "url.query_args" cache context gets added...

  • jrockowitz authored 8d6e046 on 6.x 
    Issue #3201136 by jrockowitz: "url.query_args" cache context gets added...

Status: Fixed » Closed (fixed)

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

mikeyr’s picture

mikeyr commented

Hi There,

I recently updated from 6.0.3 to 6.0.5 (I noticed this was committed in 6.0.4). This specific patch commit seems to be causing my site to white screen on certain webforms with the following error:

Error: Call to a member function setOption() on null in webform_menu_local_tasks_alter() (line 278 of /modules/contrib/webform/webform.module)

This is only happening when trying to view a form at a url with a query/parameter, and only when viewed as a non-administrator.

If I revert back to 6.0.3 my form loads fine (but I can't because 6.0.5 is a security update).

mikeyr’s picture

mikeyr commented

Hi Again,

I changed the following in webform.module (starting on line 266) and it seems to resolve the issue on my site.

$route_names = [
    'entity.webform.test_form', 'entity.webform.canonical',
    'entity.node.webform.test_form', 'entity.node.canonical',
  ];
  // Adding route_arrays as separate variable from route_names
  $route_arrays =[
    'entity.webform.test_form' => 'entity.webform.canonical',
    'entity.node.webform.test_form' => 'entity.node.canonical',
  ];
  // Swapped in $route_arrays as the haystack in array_key_exists
  if (in_array($route_name, $route_names) || array_key_exists($route_name, $route_arrays)) {