Views Table style plugin breaks dynamic cache

Hi Andreyjan,

I successfully applied the patch. But still, the HTTP response header returns the "UNCACHEABLE" state for "X-Drupal-Dynamic-Cache". I hope the patch #2 is failed.

@siva_drupal sorry if this duplicates any work you might be doing, but I dove into this and didn't see you'd assigned it to yourself.

The fix looks good, it just needed tests, here is a test only patch and the combined test/fix. The test-only version is also the interdiff to #2

@lendude and @andreyjan patch #10 is not working for me. Still, the "X-Drupal-Dynamic-Cache" returns the same "UNCACHEABLE" state.(uncachable_user.png)

@siva_drupal try re-saving the View after applying the patch. For some reason that's needed for me to get this to work. Clearing the cache doesn't help.

I'm a little confused why this is needed though. But it does indicate we need something like an upgrade path for this.
Ok this setting gets saved to config on save and doesn't get checked at run time.

So yeah, we need an upgrade path for this.

Hi @Lendude and @andreyjan after re-saving the views the patch #10 working fine. Thanks for the update.

This was added in #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface

The diff there was this:

-  public function isCacheable() {
-    return TRUE;
+  public function getCacheMaxAge() {
+    return 0;
 

So that is clearly incorrect, hence this change is appropriate.

+++ b/core/modules/views/tests/src/Functional/Plugin/StyleTableTest.php
@@ -229,4 +230,18 @@ public function testGrouping() {
+  function testTableCacheability() {

missing visibility public, fails PHPCS

@larowlan thanks for taking a look. Updated the IS to include your research.

We also needed an upgrade path. This addresses #19 and adds the upgrade path and a test for it.

Bleh, ignore #20.

Now without the Webdriver changes included.....

Sorry about the noise..... now a 8.4.x and an 8.5.x patch.

The 8.4.x patch has the wrong post update, no idea how that happend.

Upping the 8.5.x patch by itself to make it clear what needs reviewing.

needed another reroll, *fingers crossed*

+++ b/core/modules/views/views.post_update.php
@@ -321,3 +321,20 @@ function views_post_update_filter_placeholder_text() {
+        continue 2;

For better longterm readability I would have built a list of views to e saved. continue 2 might be something people don't know it exists. Still, this is my personal choice, I just wanted to put it out :)

I had a quick look whether the table style plugin provides the necessary cache contexts I would expect: url query parameters for table sorting, so enabling is totally fine.

+++ b/core/modules/views/views.post_update.php
@@ -321,3 +321,20 @@ function views_post_update_filter_placeholder_text() {
+/**
+ * Fix cache max age for table displays.
+ */
+function views_post_update_table_display_cache_max_age() {
+  /* @var \Drupal\views\Entity\View[] $views */
+  $views = View::loadMultiple();
+  foreach ($views as $view) {
+    $displays = $view->get('display');
+    foreach ($displays as $display_name => &$display) {
+      if (isset($display['display_options']['style']['type']) && $display['display_options']['style']['type'] === 'table') {
+        $view->save();
+        continue 2;
+      }
+    }
+  }
+}

Let's batch. Just to be safe. All updates that are loops we should batch because we cannot know how many things there are.

@alexpott yeah absolutely, stole your code from #2942986: Views should be dependent on providers of table data via hook_views_data :)

Also fixed a copy/paste leftover.

Thank you for implementing the batch saving!

Committed and pushed 4c127e1a9b to 8.6.x and f5f0896031 to 8.5.x. Thanks!

Backported to 8.5.x as an important bugfix for cacheability of sites.

Possibly related: #2999869: InvalidArgumentException: Placeholders must have a trailing [] if they are to be expanded with an array of values. in Drupal\Core\Database\Connection->expandArguments() when updating the database to 8.6.x.

See also #3038408: Update config/*/views.view.*.yml tables with correct dynamic cache max-age.