Adds cache context for s param to fix issue where one user's search string can be displayed to other users.

[rupertj]

If the search block is built and displayed immediately after a cache clear for a user running a search, their search string is cached and displays in the block on subsequent page loads, for other users.

To recreate the issue:

Load a LocalGov site in two different browsers to simulate two separate users. (eg one chrome window, one safari). Be logged out in both browsers.

Open a terminal and clear the cache with Drush.

In one browser, enter a search term into sitewide search block and submit the form. Once the page loads, hit refresh in the other browser. You'll see the search term you entered in the first browser appear.

[ekes]

I'm guessing it should be pretty easy to write a FunctionalTest for this? We could even c&p it into any other similar searches (directories)?

[finnlewis]

Hi @rupertj

Thanks for this!

I'm not currently able to reproduce the behaviour.

I enabled caching by commenting out all the lines in our settings.lndo.php like https://github.com/localgovdrupal/localgov_project/blob/2.x/assets/composer/settings.lando.php#L69 and tested in two browsers.

@ekes have you tried?

@rupertj could you chuck us a couple of screen shots so I can be sure what I've looking for?

Many thanks,

Finn

[finnlewis]

Just to confirm, I'm, getting x-drupal-cache: HIT in both browsers headers.

[rupertj]

Cheers for taking a look. We actually first ran into this issue on beta.lbhf.gov.uk, and I added the code in the MR to fix it before we put it live. I managed to recreate it on a new stock install of LDG running locally on ddev too though.

I'll grab a quick video of it happening if you like?

[rupertj]

https://user-images.githubusercontent.com/326243/192262484-3d000c53-dbdc-4b76-b480-19729c3b5b90.mov

[ekes]

I also am not recreating it locally. The patch itself makes sense. So I'd still be most happy if there was a test for it.

[ekes]

Writing tests I notice in the debug output the X-Drupal-Cache-Contexts already includes url which should as I understand change it for every part of the URL including the query.

Full value: 'X-Drupal-Cache-Contexts' => 'languages:language_interface theme url user.node_grants:view user.permissions',

Maybe that url is being added by something we're assuming is there, and isn't necessarily? What's yours got @rupertj [edit: as I chat about this on a MM it is clearly better that we would vary only on the query:s rather than everything so it would be good to work this out]

[rupertj]

Here's what I've got in my response headers:

HTTP/2 200 OK
server: nginx/1.20.1
date: Mon, 10 Oct 2022 15:32:00 GMT
content-type: text/html; charset=UTF-8
vary: Accept-Encoding
cache-control: must-revalidate, no-cache, private
x-drupal-dynamic-cache: UNCACHEABLE
x-ua-compatible: IE=edge
content-language: en
x-content-type-options: nosniff
x-frame-options: SAMEORIGIN
permissions-policy: interest-cohort=()
x-drupal-cache-tags: block_view config:block.block.localgov_breadcrumbs_scarfolk config:block.block.localgov_home_welcome_block_scarfolk config:block.block.localgov_mainnavigation_scarfolk config:block.block.localgov_mainpagecontent_scarfolk config:block.block.localgov_messages_scarfolk config:block.block.localgov_page_header_block_scarfolk config:block.block.localgov_poweredbylocalgovdrupal_scarfolk config:block.block.localgov_servicepagerelatedlinks_scarfolk config:block.block.localgov_servicepagerelatedtopics_scarfolk config:block.block.localgov_servicescalltoaction_scarfolk config:block.block.localgov_servicesmenu_scarfolk config:block.block.localgov_sitebranding_scarfolk config:block.block.localgov_sitewide_search_block_scarfolk config:block.block.localgov_tabs_scarfolk config:block.block.views_block__services_block_service_list_scarfolk config:block_list config:search_api.index.localgov_sitewide_search config:system.site config:user.role.anonymous config:views.view.localgov_sitewide_search http_response rendered
x-drupal-cache-contexts: languages:language_interface route theme url user.permissions
x-drupal-cache-max-age: 0 (Uncacheable)
expires: Sun, 19 Nov 1978 05:00:00 GMT
x-generator: Drupal 9 (https://www.drupal.org)
x-drupal-cache: HIT
content-encoding: gzip
X-Firefox-Spdy: h2

I had to add http.response.debug_cacheability_headers: true to services.yml to get that to appear, but that hasn't stopped the bug showing up for me.

Those headers were from a codebase without my patch applied. With the patch applied, the x-drupal-cache-contexts header doesn't change.

[ekes]

[thinking aloud]

With the patch applied, the x-drupal-cache-contexts header doesn't change.

So the cache context for the whole page is varying by the full URL in both cases. This is actually not ideal, we can make it just change by the required query, but shouldn't cause cached searches to appear on the wrong page. Guessing search_api / facets will set the url context? Yet in one case cached searches appear, this must then be somewhere internal (or bigpipe related?). But, how would an internal thing be different between phpunit functional testing environment & lando with ddev?

This might be the moment to break out https://www.drupal.org/node/3162480 or https://www.drupal.org/project/cache_review

[finnlewis]

@rupertj we still cannot replicate the behaviour. I will try to set up a DDEV local to see if I can mimic your setup and replicate.

[graham-web]

We're seeing this too on a customer site :-(

Another user's search terms can pop up on random pages of the site, not only on the search page.

I can reproduce this locally also (on DDEV I'm afraid!) - with the default lando.settings.php loaded, and just the render cache backend override commented. @rupertj's fix does appear to solve the issue locally - will roll it out to real sites tomorrow.

I had a go with Cache Review module:

Before patch:

After patch:

[graham-web]

@ekes Something like this will do it (on top of #49 obviously)

    // Check caching of block.
    $this->placeBlock('localgov_sitewide_search_block', ['region' => 'header']);
    $url_search = Url::fromRoute('view.localgov_sitewide_search.sitewide_search_page', [], ['query' => ['s' => 'canary']]);
    $this->drupalGet($url_search);
    $url_front = Url::fromRoute('<front>');
    $this->drupalGet($url_front);
    $this->assertSession()->responseNotContains('canary');

[ekes]

@graham-web Thanks for that!

[ekes]

Merged all that into a PR https://github.com/localgovdrupal/localgov_search/pull/50