Make ViewAjaxResponse cacheable

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies.

This looks great to me.

CR that's added looks great and has all the correct info, RTBC+1

Thanks for the additional reviews and CR. Committed/pushed to 11.x, thanks!

Updating credit thanks!

Entity browser and our project tests break completely with this, something here isn't correct.

Looking at the response, it has no cacheability metadata at all. $response->addCacheableDependency(CacheableMetadata::createFromRenderArray($preview)); isn't working, $preview not yet rendered at this point, the cacheability metadata is in $preview['view'] and not picked up by createFromRenderArray().

IMHO, this shouldn't rely on the rendering to set cacheability metadata on there IMHO. The controller implements and uses specific query parameters, such as view_name and view_display_id. It should IMHO always explicitly add those specific query parameters or just url.query_args to the response, otherwise there might be edge cases that might be missing them, for example a weird display that outputs something unexpected?

We should probably roll this back and recommit with that fixed. Not at computer but re-opening so it doesn't get lost.

Reverted from 11.x for now.

I unpublished the change record.

Sure we want to change to change ViewAjaxTest to use Nodebase? Doesn't that make node a hard dependency in view

I'm experiencing issue when browser cache is disabled (e.g. using "Disable cache" in Chrome DevTools). The first AJAX request works fine, but the second one fails with the following error:

Error: Call to a member function get() on null в Drupal\views\Plugin\views\display\DisplayPluginBase->getHandlers() (строка 867 из /var/www/html/web/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php)
#0 /var/www/html/web/core/modules/views/src/ViewExecutable.php(1100): Drupal\views\Plugin\views\display\DisplayPluginBase->getHandlers('field')
#1 /var/www/html/web/core/modules/views/src/ViewExecutable.php(958): Drupal\views\ViewExecutable->_initHandler('field', Array)
#2 /var/www/html/web/core/modules/views/src/ViewExecutable.php(2586): Drupal\views\ViewExecutable->initHandlers()
#3 [internal function]: Drupal\views\ViewExecutable->__wakeup()
#4 /var/www/html/web/core/lib/Drupal/Component/Serialization/PhpSerialize.php(21): unserialize('O:34:"Drupal\\vi...')
#5 /var/www/html/web/modules/contrib/redis/src/Cache/CacheBase.php(381): Drupal\Component\Serialization\PhpSerialize::decode('O:34:"Drupal\\vi...')
#6 /var/www/html/web/modules/contrib/redis/src/Cache/Predis.php(69): Drupal\redis\Cache\CacheBase->expandEntry(Array, false)
#7 /var/www/html/web/modules/contrib/redis/src/Cache/CacheBase.php(159): Drupal\redis\Cache\Predis->getMultiple(Array, false)
#8 /var/www/html/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(319): Drupal\redis\Cache\CacheBase->get('http://vitrina....', false)
#9 /var/www/html/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(134): Drupal\page_cache\StackMiddleware\PageCache->get(Object(Symfony\Component\HttpFoundation\Request))
#10 /var/www/html/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(87): Drupal\page_cache\StackMiddleware\PageCache->lookup(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#11 /var/www/html/web/core/modules/ban/src/BanMiddleware.php(50): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#12 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(48): Drupal\ban\BanMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#13 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(51): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#14 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/AjaxPageState.php(36): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#15 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/StackedHttpKernel.php(51): Drupal\Core\StackMiddleware\AjaxPageState->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#16 /var/www/html/web/core/lib/Drupal/Core/DrupalKernel.php(741): Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#17 /var/www/html/web/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#18 {main}

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Thanks for working on this @prudloff! It would be really nice to get better Views AJAX caching support.

I guess the expected result after patching is that these:

x-drupal-cache: UNCACHEABLE (no cacheability)
x-drupal-dynamic-cache: UNCACHEABLE (no cacheability)

Change to this?

x-drupal-cache: HIT
x-drupal-dynamic-cache: HIT

To make it clear what to check for after patching, could an "Expected result" be added in the Issue Summary?

My comment in #22 may still stand but moving to NW as it has merge conflicts.

Unable to reproduce the issue #22

NR isn’t the correct status

Believe all feedback has been addressed

Ran the test-only https://git.drupalcode.org/issue/drupal-3509179/-/jobs/7771835 shows the coverage

CR is already written and straight forward.

Tweaked the IS slightly incase this needed a snippet (just copied from the CR)

LGTM

One small comment on the MR, but it confused me reviewing this issue twice.

I added a couple comments about the ->expects($this->any()) added usages here that apparently should be removed per #3561671: [meta] Refactor tests to use stubs instead of mocks where mocks do not configure expectations. Though I don't know that it should be a blocker here as long it gets cleaned up there?

Otherwise +1 RTBC.

I think this could benefit from more explicit, real test coverage that caching actually works. At least in our case, there are ... complications. I see that the responses, at least some of them, are being cached, but the query strings are also always different.

I think part of that is conflicts with #2823541: Table clicksort is lost when using views exposed filter & Pager exposed '#items', which we need for our custom block filter display plugin, closely related to #2605218: Views Block Display skips preBlockBuild() call on ajax rebuild in combination with the new setBrowserUrl ajax command. It's quite hard to get into a case where the query strings are the same. But another thing is that the views dom id is being passed around, and that changes every time the view is rendered, so that too complicates cache hits.

After playing around with it a bit, I managed to get a page cache hit, which resulted in a fatal error with this backtrace:

The website encountered an unexpected error. Try again later.
Error: Call to a member function get() on null in Drupal\views\Plugin\views\display\DisplayPluginBase->getHandlers() (line 852 of core/modules/views/src/Plugin/views/display/DisplayPluginBase.php).

Drupal\views\Plugin\views\display\DisplayPluginBase->getHandlers() (Line: 1100)
Drupal\views\ViewExecutable->_initHandler() (Line: 958)
Drupal\views\ViewExecutable->initHandlers() (Line: 2582)
Drupal\views\ViewExecutable->__wakeup()
unserialize() (Line: 21)
Drupal\Component\Serialization\PhpSerialize::decode() (Line: 197)
Drupal\Core\Cache\DatabaseBackend->prepareItem() (Line: 152)
Drupal\Core\Cache\DatabaseBackend->getMultiple() (Line: 110)
Drupal\Core\Cache\DatabaseBackend->get() (Line: 321)
Drupal\page_cache\StackMiddleware\PageCache->get() (Line: 136)
Drupal\page_cache\StackMiddleware\PageCache->lookup() (Line: 89)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 50)
Drupal\ban\BanMiddleware->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 53)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 54)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 745)
Drupal\Core\DrupalKernel->handle() (Line: 19)

Not sure if this needs specific handlers or displays to trigger, but try adding another request to your test to test the cache hit/miss response headers. Browser testing would also be good I think, but I'm not sure if we can detect in JS tests that it's being cached? maybe if we query the cache tables?

The problem here is that the ViewAjaxResponse object stores the view, and that gets serialized together with the response. I'm not entirely sure where that's even used. I see for example \Drupal\geolocation\EventSubscriber\AjaxResponseSubscriber::onResponse() doing some stuff with ajax responses but nothing in core outside of the unit test? Should we remove the view on serialization of the view?

Rerolled on main and added a test on X-Drupal-Cache

I don't understand the test failure. Can somebody have a quick look, please?

Rerolled and fixed tests 🤞

@prudloff was right in #41: we had to implement custom serialization on ViewAjaxResponse like the views_ajax_get module did.

Previously failing tests are now green!

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rerolled to fix the conflict