Problem/Motivation

Back in #2318377: Determine whether a view is cacheable and its required contexts, store this i/t config entity we introduced a CachePluginInterface so that views plugin can specify which context their output/filtering is varied by.
CacheableDependencyInterface got introduced in the meantime which has the same feature, but instead of a boolean flag for cacheability
has a max-age.

Proposed resolution

Replace all usages and remove \Drupal\views\Plugin\CacheablePluginInterface

Remaining tasks

None.

User interface changes

None.

API changes

Yes: CachablePluginInterface is removed in favor of CacheableDependencyInterface.

Data model changes

Yes: config schema change.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because nothing is broken, nothing is new; we're just removing a one-off interface in favor of the standardized interface.
Issue priority Major because Views using a one-off interface rather than the standardized makes the DX & learning curve more difficult than it has to be, with likely negative consequences for overall cacheability in the D8 ecosystem.
Prioritized changes The main goal of this issue is cacheability/DX, and thus indirectly, perforamnce.
Disruption Disruptive for core/contributed modules that provide Views handlers & plugins because it requires a BC break and a data model change (config schema change). The disruption is limited to the extent that (almost?) nobody has written Views handlers/plugins in D8 contrib yet.
Finally, due to the bug #2538348: Single config export UI exports the wrong entity properties and sometimes in the wrong format, there is another aspect of disruption. Besides Views handlers/plugins, this also impacts default Views IF and ONLY IF they contain cacheability metadata; if they don't, they're not impacted; the cacheability metadata will be added automatically upon being imported/saved.
CommentFileSizeAuthor
#215 replace-2464427-215-interdiff.txt662 bytesBerdir
#215 replace-2464427-215.patch74.04 KBBerdir
#212 interdiff.txt1.46 KBWim Leers
#212 replace-2464427-212.patch77.83 KBWim Leers
#208 interdiff.txt664 bytesWim Leers
#208 replace-2464427-208.patch76.92 KBWim Leers
#207 interdiff.txt1.85 KBWim Leers
#207 replace-2464427-207.patch77.04 KBWim Leers
#206 interdiff.txt2.84 KBWim Leers
#206 replace-2464427-206.patch77.5 KBWim Leers
#204 interdiff.txt1.37 KBWim Leers
#204 replace-2464427-204.patch74.71 KBWim Leers
#203 interdiff.txt8.86 KBWim Leers
#203 replace-2464427-203.patch74.67 KBWim Leers
#201 rebase-interdiff.txt5.07 KBWim Leers
#200 interdiff.txt2.31 KBWim Leers
#200 replace-2464427-200.patch72.45 KBWim Leers
#190 interdiff.txt1.47 KBWim Leers
#190 replace-2464427-190.patch77.26 KBWim Leers
#186 interdiff.txt1.6 KBWim Leers
#186 replace-2464427-186.patch77.22 KBWim Leers
#179 interdiff.txt2.71 KBWim Leers
#179 replace-2464427-179.patch77.4 KBWim Leers
#177 interdiff.txt6.8 KBWim Leers
#177 replace-2464427-174.patch78.31 KBWim Leers
#173 replace-2464427-173.patch78.12 KBjibran
#171 replace-2464427-171.patch73.32 KBjibran
#171 interdiff.txt4.8 KBjibran
#166 replace-2464427-166.patch78.12 KBjibran
#166 interdiff.txt1.09 KBjibran
#163 replace-2464427-163.patch78.34 KBjibran
#163 interdiff-replace.txt24.57 KBjibran
#163 interdiff-review.txt2.26 KBjibran
#157 replace-2464427-157-interdiff.txt916 bytesBerdir
#157 replace-2464427-157.patch78.55 KBBerdir
#152 replace-2464427-152.patch78.43 KBjibran
#152 interdiff.txt117.65 KBjibran
#151 rebase-interdiff.txt700 bytesWim Leers
#151 replace-2464427-151.patch194.92 KBWim Leers
#140 replace-2464427-140.patch195.26 KBjibran
#140 interdiff.txt17.53 KBjibran
#140 screenshot-d8.dev 2015-08-08 03-42-21.png579.84 KBjibran
#140 screenshot-d8.dev 2015-08-08 03-39-06.png236.15 KBjibran
#134 replace-2464427-133.patch183.18 KBjibran
#134 interdiff.txt234.65 KBjibran
#131 replace-2464427-131.patch181.18 KBjibran
#131 interdiff.txt983 bytesjibran
#129 replace-2464427-129.patch181.25 KBjibran
#129 interdiff.txt1.69 KBjibran
#126 replace-2464427-126-interdiff.txt1.9 KBBerdir
#126 replace-2464427-126.patch181.35 KBBerdir
#125 screenshot-d8.dev 2015-07-30 19-42-38.png230.18 KBjibran
#123 replace-2464427-118.patch179.89 KBjibran
#118 replace-2464427-118.patch179.89 KBjibran
#118 interdiff.txt876 bytesjibran
#107 replace-2464427-107.patch179.03 KBjibran
#107 interdiff.txt566 bytesjibran
#105 replace-2464427-105.patch179.03 KBjibran
#105 interdiff.txt507 bytesjibran
#97 replace-2464427-2535082-97.patch188.61 KBjibran
#97 replace-2464427-97-do-not-test.patch179.24 KBjibran
#97 interdiff.txt1.2 KBjibran
#89 replace-2464427-89.patch179.07 KBjibran
#89 interdiff.txt569 bytesjibran
#88 replace-2464427-88.patch179.16 KBjibran
#88 interdiff.txt1.21 KBjibran
#88 resolve.txt2.4 KBjibran
#88 conflict.txt2.07 KBjibran
#86 replace-2464427-85.patch178.99 KBjibran
#86 interdiff.txt3.12 KBjibran
#84 replace-2464427-84.patch185.08 KBjibran
#84 interdiff.txt211.55 KBjibran
#81 replace-2464427-81.patch66.92 KBjibran
#81 interdiff.txt5.58 KBjibran
#79 replace-2464427-79.patch65.7 KBjibran
#79 interdiff.txt5.94 KBjibran
#78 replace-2464427-78.patch60.95 KBjibran
#78 interdiff.txt1.21 KBjibran
#75 interdiff.txt740 bytesWim Leers
#74 remove_cachepluginbase-2464427-74.patch63.42 KBWim Leers
#68 interdiff.txt1.91 KBWim Leers
#68 remove_cachepluginbase-2464427-68.patch291.37 KBWim Leers
#65 interdiff.txt229.57 KBWim Leers
#65 remove_cachepluginbase-2464427-65.patch290.35 KBWim Leers
#63 interdiff.txt2.74 KBWim Leers
#63 remove_cachepluginbase-2464427-63.patch61.66 KBWim Leers
#53 interdiff.txt6.57 KBWim Leers
#53 remove_cachepluginbase-2464427-53.patch61.47 KBWim Leers
#50 interdiff.txt3.69 KBWim Leers
#50 remove_cachepluginbase-2464427-50.patch59.5 KBWim Leers
#45 remove_cachepluginbase-2464427-45-interdiff.txt7.16 KBBerdir
#45 remove_cachepluginbase-2464427-45.patch54.75 KBBerdir
#43 remove_cachepluginbase-2464427-41-interdiff.txt7.16 KBBerdir
#43 remove_cachepluginbase-2464427-41.patch53.84 KBBerdir
#28 rebase-interdiff.txt11.76 KBWim Leers
#28 remove_cachepluginbase-2464427-28.patch49.37 KBWim Leers
#23 replace-2464427-23.patch36.24 KBborisson_
#23 interdiff.txt1.31 KBborisson_
#21 replace-2464427-19.patch36.16 KBborisson_
#19 replace-2464427-18.patch36.16 KBborisson_
#17 replace-2464427-17.patch36.16 KBborisson_
#17 interdiff.txt6.67 KBborisson_
#13 interdiff.txt739 bytesborisson_
#13 replace-2464427-13.patch29.15 KBborisson_
#2 remove_cacheableplugininterface-2464427-2.patch30.34 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Wim Leers’s picture

Title: Replace CachePluginInterface with CacheableDependencyInterface » Replace CacheablePluginInterface with CacheableDependencyInterface
Status: Active » Needs review
FileSize
30.34 KB

Here's an initial patch. Didn't run any tests. But this should capture most of the grunt work.

dawehner’s picture

MH, its a bit confusing honestly ... the cacheablePluginInterface was just use for the result caching so far, but now we talk about using it for more?

Wim Leers’s picture

No, we want to use it for exactly the same things. The intent is still exactly the same. Just consolidating on a single interface.

dawehner’s picture

Yeah, sure, but previously we had a dedicated interface, which obviously help to document it implicit to be something else.

Wim Leers’s picture

True.

Status: Needs review » Needs work

The last submitted patch, 2: remove_cacheableplugininterface-2464427-2.patch, failed testing.

Wim Leers’s picture

Thought: If we're worried about documenting the meaning, we could have CacheablePluginInterface implement CacheableDependencyInterface and use {@inheritdoc} + extra comments to document the precise meaning.

dawehner’s picture

Well yeah, I'm also wondering whether the field plugins might need to implement those for rendering as well, which then lead to different interfaces we have.

Wim Leers’s picture

Issue tags: +Performance, +D8 cacheability
Berdir’s picture

So, as just discussed with @Wim, I think this is blocking #2458763: Remove the ability to configure a block's cache max-age, as we want to return the proper max age from blocks there.

I don't see a problem with interfaces, yes, it's different and we might be losing a bit specific documentation for plugins, but the advantage is that we're then using the same terminology as everything else, the block issue shows why not doing that is a problem.

If you want to avoid the getCacheTags() method everywhere, you could add it to a base plugin somewhere without actually implementing the interface. But having it makes you aware of it and makes you think about it...

borisson_’s picture

Assigned: Unassigned » borisson_
borisson_’s picture

FileSize
29.15 KB
739 bytes

This should fix most of the errors in the patch from #2.

borisson_’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Hah, nice catch :)

Status: Needs review » Needs work

The last submitted patch, 13: replace-2464427-13.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
6.67 KB
36.16 KB

Fixed a test, let's see if testbot agrees that this is a good thing.

Status: Needs review » Needs work

The last submitted patch, 17: replace-2464427-17.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
36.16 KB

Made the patch from the right directory now, this should apply.

Status: Needs review » Needs work

The last submitted patch, 19: replace-2464427-18.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
36.16 KB

Does it apply now?

Status: Needs review » Needs work

The last submitted patch, 21: replace-2464427-19.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
1.31 KB
36.24 KB

This fixes the last test failure.

Fabianx’s picture

  1. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
    @@ -2282,19 +2284,20 @@ public function calculateCacheMetadata () {
    +      $cache_plugin->alterCacheMetadata($max_age, $cache_contexts, $cache_tags);
    

    This should operate on a CacheableMetadata object.

  2. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
    @@ -2282,19 +2284,20 @@ public function calculateCacheMetadata () {
    +    return [$max_age, $cache_contexts];
    

    This should return a CacheableMetadata object instead of its own value array.

Thanks for the work on this!

We just introduced a CacheableMetadata object, which should be used in 99% of the cases and also be used for doing all the merging, if possible.

This is all internal and Cache::merge* while being a public API should not be used directly, because CacheableDependencyInterface / CacheableMetadata can be extended at any point in the future, so changes should be simple ...

That all said:

- In the interest of getting this patch committed and focused, lets just switch:

- the alterMetaData
- the return of the getCacheableMetadata()

to operate on CacheableMetadata objects.

Then this would be RTBC for the first round ...

Fabianx’s picture

Status: Needs review » Needs work
borisson_’s picture

Assigned: borisson_ » Unassigned

I've looked at this again and I don't see how I should use the CacheableMetadata object here. I think there need to be more changes in
- cachePluginBase
- displayPluginBase / displayPluginInterface and everything implementing those.

If you can explain how I have to change these I want to help but I'm afraid I don't see it for now. Wim 'll probably have a better idea, seeing as he wrote the entire patch and all I did was fix some tests. :)

Wim Leers’s picture

Now that #2381277: Make Views use render caching and remove Views' own "output caching" has landed, I think it's even more important we do this. Views having its own special snowflake interface will hurt DX.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
49.37 KB
11.76 KB

Reroll, to have a patch that applies to HEAD. Interdiff shows the work I had to do besides resolving conflicts to get D8 to render pages again: a bunch more CacheablePluginInterface implementations appeared, I had to convert them too.

Now going over the last bunch of comments.

Status: Needs review » Needs work

The last submitted patch, 28: remove_cachepluginbase-2464427-28.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: failed to clear checkout directory.

Testbot--

Status: Needs review » Needs work

The last submitted patch, 28: remove_cachepluginbase-2464427-28.patch, failed testing.

Fabianx’s picture

+1 to #27, yes working on that issues clearly showed a lot of confusion around that.

The last submitted patch, 28: remove_cachepluginbase-2464427-28.patch, failed testing.

Wim Leers’s picture

Eh, #28's test run shows a WSOD… re-testing in hope that that will make it a non-WSOD page… The patch also still applies cleanly to latest HEAD.

(Also: all unit tests pass with this patch.)

The last submitted patch, 28: remove_cachepluginbase-2464427-28.patch, failed testing.

Wim Leers’s picture

Still WSOD :( This will be a PITA to debug then…

Wim Leers’s picture

The last submitted patch, 28: remove_cachepluginbase-2464427-28.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
53.84 KB
7.16 KB

Found the problem, not testbot's fault after all (Other than choking on 350k exceptions). And congrants @Wim, you possibly just won the most-exceptions-in-a-patch challenge ;)

Also, yet another reason that we should stop a test after the first exception/fail :)

Status: Needs review » Needs work

The last submitted patch, 43: remove_cachepluginbase-2464427-41.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
54.75 KB
7.16 KB

I guess I need to replace cacheable with max_age: 0? Also found one more plugin that needed to change to getCacheMaxAge(). Not exactly sure what to do about the sort test there?

Status: Needs review » Needs work

The last submitted patch, 45: remove_cachepluginbase-2464427-45.patch, failed testing.

Wim Leers’s picture

Thanks so much, berdir! :)

And congrants @Wim, you possibly just won the most-exceptions-in-a-patch challenge ;)

:D

I guess I need to replace cacheable with max_age: 0?

Yes.

Not exactly sure what to do about the sort test there?

max-age = 0.

Berdir’s picture

Yes, I'm aware that something needs to mark the view as max-age: 0. But why isn't it happening anymore? The random sort plugin does exactly that.

Wim Leers’s picture

Indeed, and I thought I already did that but I figured I missed that one.

Seems like a bug in how Views collects/merges the cacheability metadata of the plugins/handlers it uses then?

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
59.5 KB
3.69 KB

I said in #2 I only did the grunt work. That explains the failures: nothing was actually already using max-age + tags :)

Status: Needs review » Needs work

The last submitted patch, 50: remove_cachepluginbase-2464427-50.patch, failed testing.

Wim Leers’s picture

The 2 failures from #45 have been fixed. The 62 new failures are likely due to CIDs/cache tags being different.

Wim Leers’s picture

#24:

  1. Done.
  2. Done.

With that, all feedback is now addressed.

Wim Leers’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Assigned: Unassigned » Wim Leers

This will fail — am fixing locally.

Xano’s picture

Issue tags: +Plugin system

Status: Needs review » Needs work

The last submitted patch, 53: remove_cachepluginbase-2464427-53.patch, failed testing.

Wim Leers’s picture

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2271,21 +2271,12 @@ public function preExecute() {
-      if ($plugin instanceof CacheableDependencyInterface) {
-        $max_age = Cache::mergeMaxAges($max_age, $plugin->getCacheMaxAge());
-        $cache_contexts = Cache::mergeContexts($cache_contexts, $plugin->getCacheContexts());
-        $cache_tags = Cache::mergeTags($cache_tags, $plugin->getCacheTags());
-      }
-      else {
-        $max_age = 0;
-      }

This should always have caused every view to not be cached, because the else-branch is always called (almost every view out there uses only a subset of the possible plugin types, in that case $plugin === NULL and therefore the else-branch is executed). That's actually what we're seeing in #50.

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2271,19 +2271,12 @@ public function preExecute() {
-      if ($plugin instanceof CacheablePluginInterface) {
-        $cache_contexts = array_merge($cache_contexts, $plugin->getCacheContexts());
-        $is_cacheable &= $plugin->isCacheable();
-      }
-      else {
-        $is_cacheable = FALSE;
-      }

This is the code in HEAD. Note that it's equivalent.

So why didn't HEAD prevent every single view from being cached?

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2151,9 +2151,9 @@ protected function applyDisplayCachablityMetadata(array &$element) {
-      ->setCacheMaxAge($cache->getCacheMaxAge())
+      ->setCacheMaxAge(Cache::mergeMaxAges($cache->getCacheMaxAge(), isset($this->display['cache_metadata']['max_age']) ? $this->display['cache_metadata']['max_age'] : Cache::PERMANENT))

The culprit. Rather than looking at the calculated max-age, it was merely looking at the cache plugin's max age.

Wim Leers’s picture

Priority: Major » Critical
Issue tags: +D8 critical triage deferred

I think that due to #59, this is critical. Tentatively marking as such.

dawehner’s picture

Well to be clear the point 1) was changed as part of render caching issue. Just have a look at the random sort handler, ... it does special tricks to use max-age
so well I don't think that this is more critical than it used to be.

+++ b/core/modules/block_content/config/optional/views.view.block_content.yml
@@ -464,7 +464,7 @@ display:
-      cacheable: false
+      max_age: 0
   page_1:

@@ -486,4 +486,4 @@ display:
-      cacheable: false
+      max_age: 0

etc.

Are we sure that it makes sense for all of them here to set max-age 0, I kinda doubt that

dawehner’s picture

Issue tags: -D8 critical triage deferred

I doubt wim can set those d8 critical tags.

Wim Leers’s picture

Priority: Critical » Major

Demoting again.

-      else {
-        $max_age = 0;
-      }

This branch led me to conclude that it was intended to disable caching. But dawehner reminded me on IRC that Views handlers/plugins are *opting* in to affecting cacheability. Not implementing CacheableDependencyInterface means they don't affect cacheability. So that branch was just wrong, it should not have been there at all.

(Also: I added that tag because it obviously needed to be triaged still. It is my understanding that that just signals to the committers that this issue still needs to be triaged. Therefore anybody can add it.)

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
FileSize
61.66 KB
2.74 KB

This patch fixes a bug introduced by #53, as well as the problem outlined in #58, which was also responsible for a whole lot of failures. Now, cacheable plugins are opt-in again.

Status: Needs review » Needs work

The last submitted patch, 63: remove_cachepluginbase-2464427-63.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
290.35 KB
229.57 KB

That's more like it :)


That failure in UpdatePathTestBaseTest is apparently due to a D8 DB dump that was added in #1314214: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols), and that is now outdated.

Updating that was… quite painful. Instructions are lacking. And during the git add -p process, this is what happens:

$ 
┬␋└.┌␊␊⎼⎽ ▒├ ┬␋└┌␊␊⎼⎽-▒␌─┤␋▒ ␋┼ ·/W⎺⎼┐/␍⎼┤⎻▒┌-├⎼␊⎽ ⎺┼ 8.0.│*
$ ±␋├ ⎽├▒├┤⎽
O┼ ␉⎼▒┼␌␤ 8.0.│
Y⎺┤⎼ ␉⎼▒┼␌␤ ␋⎽ ▒␤␊▒␍ ⎺° '⎺⎼␋±␋┼/8.0.│' ␉≤ 1 ␌⎺└└␋├.
  (┤⎽␊ "±␋├ ⎻┤⎽␤" ├⎺ ⎻┤␉┌␋⎽␤ ≤⎺┤⎼ ┌⎺␌▒┌ ␌⎺└└␋├⎽)
Y⎺┤ ▒⎼␊ ␌┤⎼⎼␊┼├┌≤ ␉␋⎽␊␌├␋┼

No fun ;) And that's also why the patch is suddenly so enormous.

Status: Needs review » Needs work

The last submitted patch, 65: remove_cachepluginbase-2464427-65.patch, failed testing.

Wim Leers’s picture

Oops, a debug statement made it in there.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
291.37 KB
1.91 KB

This should be green.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - if green. Do we need a change record?

jibran’s picture

This changes the existing views so should we write an update hook to re-save all the existing views?

catch’s picture

Status: Reviewed & tested by the community » Needs work

Yes we should probably have that and not update the db dump at this point.

Wim Leers’s picture

So … should that update hook basically erase the config cache and resave every view config entity? Are we sure that cannot time out? Also, it seems we have zero examples of hook_update_N() in HEAD?

Fabianx’s picture

#72: hook_update_N() can be batched and because an upgrade path is needed now, unfortunately re-saving is what will need to be done.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
63.42 KB

This should do the trick, but does not. Just re-saving the view is not sufficient it seems: the data stored in the config system remains unchanged. Looks like we'll need somebody with deeper CMI knowledge to figure this one out.

Wim Leers’s picture

FileSize
740 bytes

Forgot the interdiff. Note that the DB dump change was reverted, but having that in the interdiff is rather… pointless/painful.

jibran’s picture

+++ b/core/modules/views/views.install
@@ -11,3 +11,19 @@
+  $storage = \Drupal::entityManager()->getStorage('view');
+  $views = $storage->loadMultiple(NULL);

Why not just load enabled views?

Wim Leers’s picture

@jibran: Feel free to take this patch to the finish line.

As I said in #74: the update hook doesn't seem to work locally. Yet somehow it's passing tests. I don't understand why.

So if you have time, don't think I want to finish this patch per se :P

jibran’s picture

@Wim Leers gee, thanks for dumping the patch on me :P

This code is from \Drupal\views\Plugin\views\display\DisplayPluginBase::calculateCacheMetadata() Probably out of scope.

    // Iterate over ordinary views plugins.
    foreach (Views::getPluginTypes('plugin') as $plugin_type) {
      $plugin = $this->getPlugin($plugin_type);
      if ($plugin instanceof CacheableDependencyInterface) {
        $cache_metadata = $cache_metadata->merge(CacheableMetadata::createFromObject($plugin));
      }
    }

    // Iterate over all handlers. Note that at least the argument handler will
    // need to ask all its subplugins.
    foreach (array_keys(Views::getHandlerTypes()) as $handler_type) {
      $handlers = $this->getHandlers($handler_type);
      foreach ($handlers as $handler) {
        if ($handler instanceof CacheableDependencyInterface) {
          $cache_metadata = $cache_metadata->merge(CacheableMetadata::createFromObject($handler));
        }
      }
    }

Shouldn't we ignore the same $plugin_type(i.e field using @ViewsField("field")) and $handler_type here unless I'm missing something? It will surely improve the performance of saving view.

In this patch.

  • Updated docs.
  • Loaded all enable views.
  • Updated cacheability metadata.

Pending hook_update test. I'll try to figure out how to write tests after having a look at #2498625: Write tests that ensure hook_update_N is properly run.

jibran’s picture

FileSize
5.94 KB
65.7 KB

Update hook works just fine. Wrote some tests unable to run those locally feel free to move forward. :)

Why not just load enabled views?

Because we want to upgrade all views present in the storage.

Status: Needs review » Needs work

The last submitted patch, 79: replace-2464427-79.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
5.58 KB
66.92 KB

Fixed some copy paste errors but it's still failing.

Status: Needs review » Needs work

The last submitted patch, 81: replace-2464427-81.patch, failed testing.

dawehner’s picture

Here is some suggestion how to fix the tests:

  • Drupal\views\Tests\TestViewsTest: I think better dump the DB with the view inside there, given that this tests better the scenario which will happen on actual sites.
  • This should also allow you to get rid of the schema in views_test_updates. Keep in mind, again, this is how actual sites should look like, and testing this is the entire point of the issue.
  • Drupal\views\Tests\Update\UpdateTest:
        // Increment the schema version.
        \Drupal::state()->set('views', 8001);
    

    Use drupal_set_installed_schema_version instead().

    In general I'm curious why you don't have to set this here at the beginning of the test.

jibran’s picture

Status: Needs work » Needs review
FileSize
211.55 KB
185.08 KB

@dawehner thanks for the suggestions it helped me a lot I think DB dump will be great we can also use this DB dump in #2514784: \Drupal\views\Plugin\views\argument\ArgumentPluginBase should allow subplugins to specify a more specific url cache context as well.

  • Re-rolled the patch
  • Found conflict
    -       $key_data += \Drupal::service('cache_contexts_manager')->convertTokensToKeys($this->displayHandler->getCacheMetadata()['contexts'])->getKeys();
     -      $key_data += \Drupal::service('cache_contexts_manager')->convertTokensToKeys($this->displayHandler->getCacheMetadata()->getCacheContexts());
    ++      $key_data += \Drupal::service('cache_contexts_manager')->convertTokensToKeys($this->displayHandler->getCacheMetadata()->getCacheContexts())->getKeys();
    
  • Re-wrote the update test.
  • Found out archive view is missing the cachemetadate.
  • Tests are not passing locally.

Status: Needs review » Needs work

The last submitted patch, 84: replace-2464427-84.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
3.12 KB
178.99 KB

Removed unused test module.

Status: Needs review » Needs work

The last submitted patch, 86: replace-2464427-85.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
2.07 KB
2.4 KB
1.21 KB
179.16 KB

YAR(yet another reroll) and fixed the test fails now we have a working upgrade path with tests.

jibran’s picture

FileSize
569 bytes
179.07 KB

Missed the schema file changes.

jibran’s picture

[Docker\Exception\APIException]                                      
03:33:23   Insertion failed because database is full: database or disk is full  

:/

The last submitted patch, 88: replace-2464427-88.patch, failed testing.

Wim Leers’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/config/schema/views.schema.yml
@@ -118,6 +118,9 @@ views.view.*:
+              cacheable:
+                type: boolean
+                label: 'Cacheable'

No, this patch specifically removed that!

jibran’s picture

I know but update test doesn't pass without this so maybe we can remove this in follow up.

Berdir’s picture

The update is supposed to remove exactly that. So no, we shouldn't keep it, we need to fix the update. @jhedstrom has been writing lots of config update functions for head2head, you might want to check with him.

alexpott’s picture

Issue tags: +D8 upgrade path
alexpott’s picture

So the fails in #88 are happening (at least in Drupal\system\Tests\Entity\Update\SqlContentEntityStorageSchemaIndexTest) because ViewsEntitySchemaSubscriber::onEntityTypeUpdate is firing before the views_update_8001().

jibran’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
179.24 KB
188.61 KB

Status: Needs review » Needs work

The last submitted patch, 97: replace-2464427-2535082-97.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review

It has the same fail as #2535082: Allow hook_update_N() implementations to run before the automated entity updates it means do-not-test patch is green and ready for review.

catch’s picture

Wim Leers’s picture

Re-testing #97, per #100.

Status: Needs review » Needs work

The last submitted patch, 97: replace-2464427-2535082-97.patch, failed testing.

Wim Leers’s picture

Issue tags: +Needs reroll
jibran’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
507 bytes
179.03 KB

Yay!!! #2535082: Allow hook_update_N() implementations to run before the automated entity updates is in. Here is a reroll. Removed hook_update_dependencies as it's not needed anymore.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#68 was RTBC. @jibran worked on the upgrade path. AFAICT the upgrade path is ready. Back to RTBC.

Only found nits that can be fixed upon commit:

+++ b/core/modules/views/views.install
@@ -11,3 +11,39 @@
+  // Remove cacheable key form cache_metadata.

s/cacheable/'cacheable'/
s/form/from/

jibran’s picture

FileSize
566 bytes
179.03 KB

Here we go. Thanks @Wim Leers for the review.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs beta evaluation, +Needs change record

Considering any module that provides a default view will need to update that config I think we need a CR. Also the issue is missing a beta evaluation that explains why this is major and why it should be allowed during beta.

Wim Leers’s picture

Issue summary: View changes
Issue tags: -Needs beta evaluation
Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

From IRC:

12:36:57 WimLeers: alexpott: RE: https://www.drupal.org/node/2464427 + default views
12:36:59 Druplicon: https://www.drupal.org/node/2464427 => Replace CacheablePluginInterface with CacheableDependencyInterface #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface => 110 comments, 12 IRC mentions
12:37:34 WimLeers: alexpott: why does the export UI do this:

  $form['export']['#value'] = Yaml::encode($this->configStorage->read($name));

and not respect

  \Drupal\Core\Config\Entity\ConfigEntityTypeInterface::getPropertiesToExport()

?
12:37:50 WimLeers: alexpott: the cacheability metadata in the View config entity should not be exported
12:37:52 WimLeers: dawehner: ^^
12:38:09 dawehner: WimLeers: i know that getPropertiesToExport got added later, maybe this is why
12:38:25 WimLeers: ah hmmm
12:38:33 WimLeers: that seems like it's at least major, potentially critical?
12:38:45 WimLeers: Because it means that all exported config as of today could contain things it should not contain?
12:39:02 dawehner: WimLeers: why is the export UI critical?
12:39:20 WimLeers: dawehner: because the export UI is how people create default views for example
12:39:34 dawehner: WimLeers: so?
12:39:37 WimLeers: dawehner: and because it exports things it should not export, https://www.drupal.org/node/2464427 disrupts default views
12:39:38 Druplicon: https://www.drupal.org/node/2464427 => Replace CacheablePluginInterface with CacheableDependencyInterface #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface => 110 comments, 13 IRC mentions
12:39:53 dawehner: well then we have >100 criticals in the major queue
12:39:55 WimLeers: dawehner: i.e. data model changes in NON-EXPORTED things should not require re-exporting
12:40:02 dawehner: there was major issues way worse, just telling you
12:40:16 WimLeers: dawehner: I'm asking because this will affect every piece of exported config in the long run
12:40:25 WimLeers: dawehner: not every major has such long-term consequences as this one
12:40:40 alexpott: WimLeers: I think this is just a major bug
12:40:44 dawehner: well, I consider the export UI as pure debugging tool
12:40:53 WimLeers: alexpott: ok
12:41:01 dawehner: and config_devel the way how you actually export default views
12:41:07 WimLeers: dawehner: aha!
12:41:10 WimLeers: dawehner: I didn't know that

Filed #2538348: Single config export UI exports the wrong entity properties and sometimes in the wrong format for that. Updated IS accordingly. Because the biggest disruption that Alex Pott pointed out in #108 is actually due to that bug in the Configuration System!

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record

CR created. Omitted the part about default views, because apparently you should not export views using the UI, but using the drush config-export command. In that case, your default views would not need to be updated.

almaudoh’s picture

apparently you should not export views using the UI

So I have been doing the wrong thing all this while...didn't know about drush config-export. Is this documented anywhere?

Wim Leers’s picture

I also didn't know that. IDK about documentation about that.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
index cccd45b..c4d6a15 100644
--- a/core/modules/aggregator/config/optional/views.view.aggregator_sources.yml

--- a/core/modules/aggregator/config/optional/views.view.aggregator_sources.yml
+++ b/core/modules/aggregator/config/optional/views.view.aggregator_sources.yml

+++ b/core/modules/aggregator/config/optional/views.view.aggregator_sources.yml
@@ -141,7 +141,7 @@ display:

@@ -141,7 +141,7 @@ display:
       contexts:
         - 'languages:language_content'
         - 'languages:language_interface'
-      cacheable: false
+      max_age: 0
   feed_1:
     display_plugin: feed
     id: feed_1
@@ -398,7 +398,7 @@ display:

@@ -398,7 +398,7 @@ display:
       contexts:
         - 'languages:language_content'
         - 'languages:language_interface'
-      cacheable: false
+      max_age: 0
   page_1:
     display_plugin: page
     id: page_1
@@ -418,4 +418,4 @@ display:

@@ -418,4 +418,4 @@ display:
       contexts:
         - 'languages:language_content'
         - 'languages:language_interface'
-      cacheable: false
+      max_age: 0

What I meant by default view is something like this. Any module that provides a view in either of it's config install directories is going to have to update it. The CR needs to cover this.

jibran’s picture

Status: Needs work » Reviewed & tested by the community

Made the following changes https://www.drupal.org/node/2538352/revisions/view/8704990/8712134 to the change record. I think it addresses #115 so back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 107: replace-2464427-107.patch, failed testing.

jibran’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
876 bytes
179.89 KB

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 118: replace-2464427-118.patch, failed testing.

Status: Needs work » Needs review

jibran queued 118: replace-2464427-118.patch for re-testing.

jibran’s picture

Doesn't seem relevant.

Test name Pass Fail Exception
CollapsedDrupal\system\Tests\Entity\Update\SqlContentEntityStorageSchemaIndexTest 24 3 0
Message Group Filename Line Function Status
The node__default_langcode index was removed during update_order_test_update_8002(). Other SqlContentEntityStorageSchemaIndexTest.php 70 Drupal\system\Tests\Entity\Update\SqlContentEntityStorageSchemaIndexTest->testIndex()
The node__id__default_langcode__langcode index was created during update_order_test_update_8002(). Other SqlContentEntityStorageSchemaIndexTest.php 71 Drupal\system\Tests\Entity\Update\SqlContentEntityStorageSchemaIndexTest->testIndex()
Value true is FALSE. Other SqlContentEntityStorageSchemaIndexTest.php 79 Drupal\system\Tests\Entity\Update\SqlContentEntityStorageSchemaIndexTest->testIndex()

Status: Needs review » Needs work

The last submitted patch, 118: replace-2464427-118.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
179.89 KB

Re-uploading the same patch for CI to test.

Status: Needs review » Needs work

The last submitted patch, 123: replace-2464427-118.patch, failed testing.

jibran’s picture

Issue summary: View changes
FileSize
230.18 KB

Ahh! schema validation hits one more time. :(

Berdir’s picture

Status: Needs work » Needs review
FileSize
181.35 KB
1.9 KB

Ok, the only way I could make the test pass is to ensure that the views update functions run first.

I've also made two other changes to trust the schema which I think makes sense but it doesn't actually solve the errors, maybe the schema checker should check that flag? The problem is that that is not available in the event, so we maybe need to pass it along?

Berdir’s picture

  1. +++ b/core/modules/views/views.install
    @@ -11,3 +11,39 @@
    +  // Remove 'cacheable' key from cache_metadata.
    +  // @see https://www.drupal.org/node/2464427
    +  foreach ($config_factory->listAll('views.view.') as $view_config_name) {
    +    $view = $config_factory->getEditable($view_config_name);
    

    I think we don't use @see in inline docblocks? See block_update_8001().

  2. +++ b/core/modules/views/views.install
    @@ -11,3 +11,39 @@
    +  if (!empty($ids)) {
    +    $message = \Drupal::translation()->translate('Updated cacheability metadata for views: @ids', ['@ids' => implode(', ', $ids)]);
    +  }
    

    I don't think that printing the ID's here gives us any useful information? We update all of them, so why bother listing them?

    I have 50+ views, so that will give me a huge list of ID's...

    Possibly even drop the message completely. afaik in 7.x, we only used them if something was actually important to the user. But this really isn't...

Wim Leers’s picture

Status: Needs review » Needs work

#127.1: I think we do.

#127.2: good point!

+++ b/core/modules/system/tests/modules/update_order_test/update_order_test.install
@@ -50,4 +50,12 @@ function update_order_test_update_8002() {
+  function update_order_test_update_dependencies() {

Missing docblock.

jibran’s picture

Status: Needs work » Needs review
FileSize
1.69 KB
181.25 KB

Thanks @Berdir for fixing the tests. Would it mean that we always have to make sure that schema changes update in core runs before update_order_test update hooks?

+++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
@@ -188,7 +188,11 @@ public function onEntityTypeUpdate(EntityTypeInterface $entity_type, EntityTypeI
+      // All changes done to the views here can be trusted and this might be
+      // called during updates, when it is not safe to rely on configuration
+      // containing valid schema. Trust the data and disable schema validation
+      // and casting.
+      $view->trustData()->save();

+++ b/core/modules/views/views.install
@@ -34,7 +34,7 @@ function views_update_8001(&$sandbox) {
+    $view->save(TRUE);

If these don't fix anything here then why not move these to the follow up issue?

RE: #127

  1. You are right @see inside function block doesn't make sense. It is always converted to "See Also" on api site and having it inside the function doesn't result anything. PHPStorm also doesn't link it to anything inside the function. Anyways the linking this issue was a bad idea so I change the link to change record link and removed @see.
  2. Removed.

RE: #27
Fixed doc.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

We should be good to go again! :)

jibran’s picture

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views/src/Tests/Update/CacheabilityMetadataUpdateTest.php
@@ -0,0 +1,55 @@
+        $this->assertFalse(isset($display['cache_metadata']['cacheable']));

Shouldn't this also assert that the cacheable metadata has been replaced correctly?

The last submitted patch, 131: replace-2464427-131.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
234.65 KB
183.18 KB

Fixed the tests and #132.

Status: Needs review » Needs work

The last submitted patch, 134: replace-2464427-133.patch, failed testing.

nlisgo’s picture

Assigned: Unassigned » nlisgo

Going to take a look at this one. I'm tempted to attempt a re-roll from #129 as the interdiffs for #131 and #134 are confusing.

jibran’s picture

Why is it confusing?

nlisgo’s picture

Assigned: nlisgo » Unassigned

Confusing for me, I should say. I know why now. It's the first time I've seen a patch with a gzip in it.

The re-roll in 131 has an interdiff which is unusual for a re-roll and seems to introduce an update hook.

Going to step away from this one then.

jibran’s picture

Assigned: Unassigned » jibran

Oh I see. Ignore the interdiff in #131 it was just a reroll. It removes the conflict in views.install. Working on this right now.

jibran’s picture

Assigned: jibran » Unassigned
Status: Needs work » Needs review
FileSize
236.15 KB
579.84 KB
17.53 KB
195.26 KB

This will have the same fails as #134 but it verifies that 'cache_metadata' is updated properly.

@Berdir or @alexpott I need help to understand the fails in #134.

I ran SqlContentEntityStorageSchemaIndexTest locally
Got following updates.

After running updates got the following error.

+++ b/core/modules/views/src/Tests/Update/CacheabilityMetadataUpdateTest.php
@@ -23,7 +23,7 @@ class CacheabilityMetadataUpdateTest extends UpdatePathTestBase {
+    $this->databaseDumpFiles = [__DIR__ . '/../../../../system/tests/fixtures/update/drupal-8.0.0-cdd12a9.standard.php.gz'];

I think I can run my updates on drupal-8.bare.standard.php.gz instead of drupal-8.0.0-cdd12a9.standard.php.gz.

jibran’s picture

This is how the full update function looks now.

/**
 * Update all views to re-generate the cacheability metadata.
 */
function views_update_8002(&$sandbox) {
  $config_factory = \Drupal::configFactory();

  // Remove 'cacheable' key from cache_metadata. See change record
  // https://www.drupal.org/node/2538352 for more explanation.
  foreach ($config_factory->listAll('views.view.') as $view_config_name) {
    $view = $config_factory->getEditable($view_config_name);
    // To update the cacheability metadata of an existing view we have to
    // recalculate it. Cacheability metadata is calculated per display bases so
    // we have to load each display. To load the each display we need
    // \Drupal\views\ViewExecutable. To load the ViewExecutable we need
    // \Drupal\views\ViewEntityInterface which can be loaded using views storage
    // controller.
    /*  @var \Drupal\views\ViewEntityInterface $view_storage */
    $view_storage = \Drupal::service('entity.manager')
      ->getStorage('view')
      ->load($view->get('id'));
    $view_executable = \Drupal::service('views.executable')
      ->get($view_storage);
    $displays = $view->get('display');
    foreach (array_keys($displays) as $display_id) {
      $view_executable->setDisplay($display_id);
      $display_plugin = $view_executable
        ->getDisplay();
      // Remove the old cacheability metadata so that we can recalculate.
      unset($display_plugin->display['cache_metadata']);
      $cache_metadata = $display_plugin->getCacheMetadata();
      $view->set("display.$display_id.cache_metadata", [
        'contexts' => $cache_metadata->getCacheContexts(),
        'max_age'=> $cache_metadata->getCacheMaxAge(),
        'tags' => $cache_metadata->getCacheTags(),
      ]);
    }
    $view->save(TRUE);
  }

  return t('Updated cacheability metadata of all the views.');

}

Status: Needs review » Needs work

The last submitted patch, 140: replace-2464427-140.patch, failed testing.

Wim Leers’s picture

The second screenshot says:

Schema errors for views.view.archive with the following errors: views.view.archive:display.default.cache_metadta.cacheable missing schema, views.view.archive:display.block_1.cache_metadata.cacheable missing schema, …

In other words: a whole bunch of config still contains the cacheable key, even though this update was supposed to remove them. So, somehow, those old values seem to be persisting, despite this code, which looks like it should be overwriting all values under cache_metadata:

+++ b/core/modules/views/views.install
@@ -110,12 +110,55 @@ function views_update_8001(&$sandbox) {
+      $view->set("display.$display_id.cache_metadata", [
+        'contexts' => $cache_metadata->getCacheContexts(),
+        'max_age'=> $cache_metadata->getCacheMaxAge(),
+        'tags' => $cache_metadata->getCacheTags(),
+      ]);

So… huh? I think we need @amateescu or @Berdir to figure this out.

Berdir’s picture

So, what I expected in #126 happened now.

The problem is that save(TRUE) does *not* disable the config schema check. And in views_update_8001(), we resave all views, that causes an config schema exception, update_do_one() silently eats and logs that and marks the update as aborted. That results in views_update_8002() being skipped too, but still runs the entity update functions at the end which then explode.

Disabling strict schema checking results in the test passing but that doesn't really count I guess.

So the only alternative I see is passing that flag along to the event so that we can check it?

Berdir’s picture

Title: Replace CacheablePluginInterface with CacheableDependencyInterface » [PP-1] Replace CacheablePluginInterface with CacheableDependencyInterface
Status: Needs work » Postponed
jibran’s picture

I checked SqlContentEntityStorageSchemaIndexTest locally after applying #2550407-2: Strict config schema breaks update tests, switch to testing the configuration at the end instead. It is green now.
Meanwhile any feedback on update hook. @Wim Leers are you happy with generating the metatdata in update hook like this? Maybe @dawehner can review the views_update_8002() as well.

Wim Leers’s picture

I think it's mostly @dawehner that has to give his approval for that, not me.

Wim Leers’s picture

Title: [PP-1] Replace CacheablePluginInterface with CacheableDependencyInterface » Replace CacheablePluginInterface with CacheableDependencyInterface
Status: Postponed » Needs review

Status: Needs review » Needs work

The last submitted patch, 140: replace-2464427-140.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
194.92 KB
700 bytes

Just a reroll. Had to resolve one conflict.

jibran’s picture

FileSize
117.65 KB
78.43 KB

All the upgrade related issues are fixed now let's remove the DB file as well. Thanks @Wim Leers for the reroll and constantly making noise about it in IRC.

Wim Leers’s picture

Woot! 60% patch size reduction :D

But also 3 fails :(

jibran’s picture

Reverting #2550615: Update the DB dump to have a leading slash for the frontpage. fixes the test for me locally. Something went wrong over there.

Status: Needs review » Needs work

The last submitted patch, 152: replace-2464427-152.patch, failed testing.

Berdir’s picture

No, nothing went wrong there.

What that issue did is fix the frontpage, which now actually points to the frontpage view again. Which we access apparently in that test *before* running updates to see if the imported worked. And that results in that notice/exception because the views config hasn't been fixed yet.

Which seems very flawed, because sooner or later, we're going to change something where doing exactly results in errors. Like we do here.

What we can do here to work around that is to make that code a bit more resilient and in case of incorrect cacheability metadata, working on that at the moment.

Berdir’s picture

Status: Needs work » Needs review
FileSize
78.55 KB
916 bytes

This passes for me but as I said, I'm not sure that test is a good idea.

dawehner’s picture

What that issue did is fix the frontpage, which now actually points to the frontpage view again. Which we access apparently in that test *before* running updates to see if the imported worked. And that results in that notice/exception because the views config hasn't been fixed yet.

How does that exactly happen?

jibran’s picture

Which seems very flawed, because sooner or later, we're going to change something where doing exactly results in errors. Like we do here.

@Berdir are we going to fix it in a follow up?

@dawehner Could you please review the views_update_8002()?

Wim Leers’s picture

Wow, thanks @Berdir for that analysis.

Like @dawehner in #158, I wonder how that can ever happen. That seems frighteningly fragile?

P.S.: that's how many upgrade path issues already now that were uncovered thanks to this issue? :P

jibran’s picture

P.S.: that's how many upgrade path issues already now that were uncovered thanks to this issue? :P

That is why we have to get this in ASAP so that we can uncover more in issue queue with failing tests/patches.

dawehner’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/views/src/Tests/Update/CacheabilityMetadataUpdateTest.php
    @@ -0,0 +1,58 @@
    +    foreach (Views::getAllViews() as $view) {
    +      $displays = $view->get('display');
    +      foreach (array_keys($displays) as $display_id) {
    +        $display = $view->getDisplay($display_id);
    +        $this->assertTrue(isset($display['cache_metadata']['cacheable']));
    +      }
    +    }
    

    It is super odd to test things before the update ... I mean don't we trust what we put into the export? In general I'm a bit nervous to call out to APIs before the update.

  2. +++ b/core/modules/views/src/Tests/Update/CacheabilityMetadataUpdateTest.php
    @@ -0,0 +1,58 @@
    +        $this->assertTrue(isset($display['cache_metadata']['max_age']));
    

    Here is a question. For code we use a "-" instead, see

    ~/w/d8 (2551037) $ ag "max_age" | wc -l
          43
    ~/w/d8 (2551037) $ ag "max-age" | wc -l
         193
    ~/w/d8 (2551037) $ 
    
  3. +++ b/core/modules/views/views.install
    @@ -110,12 +110,55 @@ function views_update_8001(&$sandbox) {
    +    $view_storage = \Drupal::service('entity.manager')
    +      ->getStorage('view')
    ...
    +    $view_executable = \Drupal::service('views.executable')
    

    Just to make life a bit more readable i'd suggest to define the two vars outside of the foreach

jibran’s picture

Thanks @dawehner for the review.

  1. Good call. See interdiff-review.txt.
  2. Fixed. See interdiff-replace.txt
  3. @jibran-- for declaring those in the loop fixed. See interdiff-review.txt.
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#182++.

We discussed max_age vs. max-age in IRC with Alex Pott, jibran, and dawehner. I originally used underscores instead of dashes because I wrongly thought dashes were disallowed as config keys. Consistentcy++

DrupalCI passing. Classic Testbot should agree.

Hopefully this will finally make it in :)

jibran’s picture

jibran’s picture

FileSize
1.09 KB
78.12 KB

Removed useless asserts those can be problematic in future and a minor doc fix.

jibran’s picture

In #2514784: \Drupal\views\Plugin\views\argument\ArgumentPluginBase should allow subplugins to specify a more specific url cache context we also want to re-generate the cacheability metadata of views. I also saw the inconstancy

+++ b/core/modules/comment/config/optional/views.view.comment.yml
@@ -567,9 +567,7 @@ display:
-        - 'url.query_args.pagers:0'
-        - 'url.query_args:order'
-        - 'url.query_args:sort'
+        - url.query_args

while exporting the views. I think we should wait for all the issues which are changing the cacheability metadata and then add the update path. What do you think @Wim Leers?

Wim Leers’s picture

#167: No, we should get this in ASAP. The issue you linked to, and others like it, are just about making the cache contexts of various plugins more specific. I.e. instead of saying "this plugin makes the resulting view vary by URL", it is improved by saying "this plugin makes the resulting view vary by query arg X of the URL". This more specific metadata improves cacheability of all views using the plugin.
But it is in no way blocking this.

dawehner’s picture

+1 for the RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/views/views.install
@@ -110,12 +110,54 @@ function views_update_8001(&$sandbox) {
+    $view_storage = $view_storage_handler->load($view->get('id'));
+    $view_executable = $view_executable_factory->get($view_storage);
+    $displays = $view->get('display');

This is going to invoke entity hooks. Which we are saying we should not do. Is this the patch that causes us to bump #2538108: Add hook_post_update_NAME() for data value updates to reliably run after data format updates. However this one is tricky - we have a schema update and a data update. What's the right way of doing this?

jibran’s picture

Issue tags: -D8 upgrade path
FileSize
4.8 KB
73.32 KB

I have removed the update path from the patch we can handle this in follow up #2553381: Add upgrade path for 2464427. #2502151: Convert shortcut block to view is postpone on this one and this is ready since #69.

Status: Needs review » Needs work

The last submitted patch, 171: replace-2464427-171.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
78.12 KB

Ignore #171 I don't think we can remove upgrade path form this patch so re-uploading the old patch so we can move forward here.

Wim Leers’s picture

@jibran So, does this then not address Alex Pott's feedback in #170?

jibran’s picture

RE: #170
From #141

    // To update the cacheability metadata of an existing view we have to
    // recalculate it. Cacheability metadata is calculated per display bases so
    // we have to load each display. To load the each display we need
    // \Drupal\views\ViewExecutable. To load the ViewExecutable we need
    // \Drupal\views\ViewEntityInterface which can be loaded using views storage
    // controller.

We need a ViewExecutable to get a proper display object which generates the new cache metadata. I have tried to find a way in core to create ViewExecutable or DisplayPluginBase object directly so that we can create new cache metadata without loading a view but I was unable to find it.
I know loading the view will fire hooks but we are loading it to get a runtime objects so that we can run methods on those. Because of this I explicitly asked @dawehner's review of update function.

Perhaps @dawehner or damiankloip knows how to create ViewExecutable object without loading the view from storage as per my limited knowledge of views this is not possible. This is the best we can do, load it using entity api and save it using config. :(

jibran’s picture

Wim Leers’s picture

Status: Postponed » Needs review
FileSize
78.31 KB
6.8 KB

This was discussed on last Friday's EU criticals call. The conclusion was:

  • keep everything backwards-compatible, therefore no upgrade path is necessary
  • keeping BC means that we continue to support the cacheable key (and if it's false we map that to max-age: 0), but that as Views are modified/saved, they migrate naturally. In other words: cacheable is deprecated (but still supported) in favor of max-age.

This reroll implements that.

moshe weitzman’s picture

Issue tags: +sprint

Wim will reroll this due to test fails.

Wim Leers’s picture

FileSize
77.4 KB
2.71 KB

The last submitted patch, 177: replace-2464427-174.patch, failed testing.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

dawehner’s picture

This is

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2297,13 +2297,21 @@ public function calculateCacheMetadata () {
+      if (isset($this->display['cache_metadata']['cacheable'])) {
+        $this->display['cache_metadata']['max-age'] = ($this->display['cache_metadata']['cacheable'] === FALSE) ? 0 : Cache::PERMANENT;

I tried to make a point on the last critical meeting but people could not listen to me properly, so meh, here is the thing: Search for \'cacheable\' in core. As you will see its not used at all. Given that this change would could cause a behaviour change out there.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

@dawehner so what does this mean? I guess in order to do a non breaking change we just a comment to the views schema say this has no meaning and remove that hunk?

dawehner’s picture

Title: Replace CacheablePluginInterface with CacheableDependencyInterface » GReplace CacheablePluginInterface with CacheableDependencyInterface

@dawehner so what does this mean? I guess in order to do a non breaking change we just a comment to the views schema say this has no meaning and remove that hunk?

Exactly.

dawehner’s picture

Title: GReplace CacheablePluginInterface with CacheableDependencyInterface » Replace CacheablePluginInterface with CacheableDependencyInterface

.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
77.22 KB
1.6 KB
12:09:40 <WimLeers> dawehner: https://www.drupal.org/node/2464427#comment-10279337 -> looks like Alex also wasn't able to understand what you meant there, can you clarify? :)
12:09:42 <Druplicon> https://www.drupal.org/node/2464427 => Replace CacheablePluginInterface with CacheableDependencyInterface #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface => 183 comments, 29 IRC mentions
12:10:10 <catch> updating symfony or changing the services.yml files?
12:10:30 <alexpott> WimLeers: I think understood... 'cacheable' is just not used atm - so the patch is an API change when we don;t want it to be
12:10:58 <WimLeers> alexpott: oh, so you mean it's a *behavior* change, not an API change?
12:11:28 <WimLeers> alexpott: i.e. the API/data model is *expanded* (hence no BC break), but the fact that we now *use* that metadata whereas we did not before, means that it's a behavior change?
12:11:42 <alexpott> WimLeers: before that patch we ignore 'cacheable' - after it we don't
12:11:50 <WimLeers> alexpott: yep
12:12:07 <WimLeers> alexpott: so you and dawehner are saying "let's not map cacheable:false/true to max-age:0/PERMANENT"?
12:12:16 <WimLeers> alexpott: i.e. let's continue to ignore it
12:12:17 <WimLeers> ?
12:12:19 <alexpott> WimLeers: so I think we should just deprecate it and say it is dead and never has been used...
12:12:23 <WimLeers> hahah
12:12:28 <WimLeers> works for me :P
12:13:09 <dawehner> WimLeers: so yeah I try to not change any behaviour
12:13:14 <WimLeers> dawehner: agreed
12:13:15 <WimLeers> dawehner++
12:13:19 <WimLeers> :)
12:13:25 <WimLeers> totally makes sense.

So, like this then.

Berdir’s picture

Uh, not sure.

Yes, we didn't use it before, but this issue is about changing that, if not here then in the follow up block ui issue.

So if we remove the block cache configuration then not respecting cacheable would cache the block. Until you save the view, then it won't be cached?

It's a behaviour change anyway, and respecting it seems to be safer choice imho?

dawehner’s picture

It's a behaviour change anyway, and respecting it seems to be safer choice imho?

Well, but all those cacheable things have been calculated wrong in the past. We assumed that people would opt into caching and all handlers would define cacheable = TRUE,
so In fact there is no view with cacheable: TRUE out there. At least on a random d8 site, there is none:

$ ag "cacheable: true" | wc -l
   0
 ag "cacheable: false" | wc -l
  111

Status: Needs review » Needs work

The last submitted patch, 186: replace-2464427-186.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
77.26 KB
1.47 KB

Rebased + fixed the fails.

#2542748: Automatic entity updates can fail when there is existing content, leaving the site's schema in an unpredictable state removed the update_order_test.install, which this patch was modifying. AFAICT.

Wim Leers’s picture

17:22:47 <WimLeers> dawehner: So… what's the outcome of the disagreement between you and berdir over at https://www.drupal.org/node/2464427#comment-10279633? Green patch for either approach!
17:22:50 <Druplicon> https://www.drupal.org/node/2464427 => Replace CacheablePluginInterface with CacheableDependencyInterface #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface => 190 comments, 31 IRC mentions
17:24:27 <dawehner> WimLeers: well, I thought the changes I prefered didn't introduced any behaviour changes
17:24:44 <WimLeers> dawehner: yep, I see your point
17:25:42 <dawehner> WimLeers: so when it doesn't change on the views layer, how would that change the behaviour on the block level?
17:25:55 <dawehner> i fear berdir is right, but I probably don't get the  point he tried to make
19:43:07 <WimLeers> dawehner: " <dawehner> i fear berdir is right, but I probably don't get the  point he tried to make"
19:43:20 <WimLeers> dawehner: the issue berdir was referring to is https://www.drupal.org/node/2458763
19:43:23 <Druplicon> https://www.drupal.org/node/2458763 => Remove the ability to configure a block's cache max-age #2458763: Remove the ability to configure a block's cache max-age => 48 comments, 6 IRC mentions
19:43:27 <WimLeers> "if not here then in the follow up block ui issue."
19:44:08 <WimLeers> "So if we remove the block cache configuration then not respecting cacheable would cache the block. Until you save the view, then it won't be cached?"
19:44:47 <WimLeers> So Berdir's concern is that Views blocks that are already not being cached because they have 'cacheable: false' set will suddenly be cached.
19:44:53 <WimLeers> But you are saying that that could not have happened.
19:44:59 <WimLeers> So I think what you're saying is fine.
19:45:01 <WimLeers> dawehner: ^^
catch’s picture

Issue tags: +rc deadline

Tagging RC deadline.

This is something we have to do either before RC, or maybe in a minor, or worst case would get punted to 9.x if neither of those, but definitely can't go into a patch-level release.

Berdir’s picture

The change record needs to be updated for the changed behavior (BC for the old keys instead of an upgrade path). Mostly just writing it a bit differently.

So the problem is that I'm saying that we can't just ignore cacheable: false because we end up caching *all* views until they are resaved. Even those sorted by Random because we'd remove the hack that we have added that makes it actually uncacheable.

But @dawehner is pointing out correctly that *all* views have cacheable: false, so by respecting it, we prevent caching for all views.

In both cases, until they are resaved and updated.

So we basically have the option of caching when we shouldn't or never cache. Both are bad.

With #2538108: Add hook_post_update_NAME() for data value updates to reliably run after data format updates, we might be able to add an update function again that does nothing but resave the views, maybe that would help? But that doesn't help with existing default views in contrib/custom, we'd still have to worry about those then again.

Wim Leers’s picture

Note that part of the problem here is that plugins can opt in to implement this interface:

    // Iterate over ordinary views plugins.
    foreach (Views::getPluginTypes('plugin') as $plugin_type) {
      $plugin = $this->getPlugin($plugin_type);
      if ($plugin instanceof CacheablePluginInterface) {
        $cache_contexts = array_merge($cache_contexts, $plugin->getCacheContexts());
        $is_cacheable &= $plugin->isCacheable();
      }
      else {
        $is_cacheable = FALSE;
      }
    }

… so if any of them doesn't implement the interface, the display will not be cacheable. I bet this is why most views actually have cacheable: false.

Also peculiar is that the behavior for handlers is different:

    // Iterate over all handlers. Note that at least the argument handler will
    // need to ask all its subplugins.
    foreach (array_keys(Views::getHandlerTypes()) as $handler_type) {
      $handlers = $this->getHandlers($handler_type);
      foreach ($handlers as $handler) {
        if ($handler instanceof CacheablePluginInterface) {
          $cache_contexts = array_merge($cache_contexts, $handler->getCacheContexts());
          $is_cacheable &= $handler->isCacheable();
        }
      }
    }

Note that there we don't set false if the handler doesn't implement the interface.

jibran’s picture

Berdir’s picture

Status: Needs review » Needs work

Yeah, I expected views block caching to be quite broken now after #2458763: Remove the ability to configure a block's cache max-age but it seems to be working quite well, actually. At least the max-age of time based caching and random being not cacheable is respected thanks to bubbling. Selecting None in views for cache also makes it uncacheable.

Bulk form also results in not being cacheable for both authenticated (makes sense, for the form token if nothing else) and anonymous (not exactly sure what that happens).

So, it's not looking that bad as far as that issue is concerned.

Discussed this also a bit with @dawehner and writing a post update function to resave them should be OK. We should keep the cacheable in the schema I think but we can probably remove the code that checks it at runtime completely if we have the update function.

  1. +++ b/core/modules/user/src/Plugin/views/access/Role.php
    @@ -149,8 +149,8 @@ public function calculateDependencies() {
        * {@inheritdoc}
        */
    -  public function isCacheable() {
    -    return TRUE;
    +  public function getCacheMaxAge() {
    +    return 0;
       }
    

    Why 0, this should be cacheable?

  2. +++ b/core/modules/user/src/Plugin/views/argument_default/User.php
    @@ -119,4 +120,11 @@ public function getCacheContexts() {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getCacheTags() {
    +    return [];
    +  }
    +
    

    We're not adding this method to ArgumentDefaultPluginBase? I know, making them think and stuff, but this makes it a considerably bigger API change.

  3. +++ b/core/modules/views/src/Plugin/views/query/QueryPluginBase.php
    @@ -320,9 +320,9 @@ public function getEntityTableInfo() {
    -  public function isCacheable() {
    +  public function getCacheMaxAge() {
         // This plugin can't really determine that.
    -    return TRUE;
    +    return Cache::PERMANENT;
       }
     
    

    Comment is a bit wird but pre-existing (I don't know but yeah whatever, just cache it ;))

jibran’s picture

writing a post update function to resave them should be OK.

Not planning to work on this at all until next week so please feel free to move it forward. Thanks.

Wim Leers’s picture

  1. Well-spotted, I bet that was an oversight.
  2. Thoughts, @dawehner? I'll let you decide, happy to do what @Berdir suggests.
  3. heh.
dawehner’s picture

Well-spotted, I bet that was an oversight.

So we need test coverage?

We're not adding this method to ArgumentDefaultPluginBase? I know, making them think and stuff, but this makes it a considerably bigger API change.

Well, yeah I think its okay to put it into the base class given how much of a rare case this is you want to override. Currently you need to explicitly specify it which has its own advantages.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
72.45 KB
2.31 KB

First, a rebase. This conflicted quite a bit with #2567183: Re-export all built-in configuration in core. That patch also happens to have done part of the work that this patch needs to do, so this patch actually becomes a bit smaller :)

Interestingly, there still seem to be some things that this patch changes in config files besides the cache_metadata key. It still adds some dependencies. But #2567183 added a test that ensures all exported config is correct. So… I expect this patch to fail. If it doesn't fail, then the test added in #2567183 is incorrect/incomplete/insufficient.

Wim Leers’s picture

FileSize
5.07 KB

Wrong interdiff, sorry.

Berdir’s picture

That the test didn't catch some might be because we don't do those tests on test modules and additionally, many of those test views are in a special folder so that we can only load them on specific tests.

Wim Leers’s picture

FileSize
74.67 KB
8.86 KB

#196:

  1. Done.
  2. Done, but note that this means ArgumentDefaultPluginBase does not implement CacheableDependencyInterface, yet does have a getCacheTags() method. That works, but is rather strange…
  3. Removed that confusing comment.

#199: added test coverage as requested. In doing so, I found a problem that is only an actual problem in a very particular edge case:

+    // @todo Fix this in https://www.drupal.org/node/2551037,
+    // DisplayPluginBase::applyDisplayCachablityMetadata() is not invoked when
+    // using buildBasicRenderable() and a Views access plugin returns FALSE.

The added test coverage does prove it works correctly though, the problem I discovered is unrelated to cacheability metadata.

Wim Leers’s picture

FileSize
74.71 KB
1.37 KB

Oops, forgot some staged bits.

The last submitted patch, 203: replace-2464427-203.patch, failed testing.

Wim Leers’s picture

FileSize
77.5 KB
2.84 KB

#196:

Discussed this also a bit with @dawehner and writing a post update function to resave them should be OK. We should keep the cacheable in the schema I think but we can probably remove the code that checks it at runtime completely if we have the update function.

This adds that post-update hook.

Wim Leers’s picture

#196:

We should keep the cacheable in the schema I think but we can probably remove the code that checks it at runtime completely if we have the update function.

This removes the run-time checks.

Wim Leers’s picture

#196

We should keep the cacheable in the schema I think but we can probably remove the code that checks it at runtime completely if we have the update function

… and in talking to @Berdir at the DrupalCon Barcelona sprint, he now thinks we can actually remove it from the schema.

The last submitted patch, 206: replace-2464427-206.patch, failed testing.

The last submitted patch, 207: replace-2464427-207.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 208: replace-2464427-208.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
77.83 KB
1.46 KB

This fixes the fail in Drupal\system\Tests\Update\UpdatePostUpdateTest::testPostUpdate(), that we can see appearing in #206 (and in subsequent patches). I will need to leave soon, so probably won't be able to figure out why the other test is fataling.

This also reverts #208, because clearly that does not work.

This should then bring it down to one failure.

Status: Needs review » Needs work

The last submitted patch, 212: replace-2464427-212.patch, failed testing.

The last submitted patch, 203: replace-2464427-203.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
74.04 KB
662 bytes

Fixed that test, didn't review yet.

The last submitted patch, 206: replace-2464427-206.patch, failed testing.

The last submitted patch, 208: replace-2464427-208.patch, failed testing.

The last submitted patch, 207: replace-2464427-207.patch, failed testing.

The last submitted patch, 212: replace-2464427-212.patch, failed testing.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - looks great to me!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 400baf1 on 8.0.x
    Issue #2464427 by jibran, Wim Leers, Berdir, borisson_: Replace...
dawehner’s picture

Awesome, thank you!

Wim Leers’s picture

GLORIOUS IS THIS DAY!

Also: I wish I could use emojis on d.o, so I could use the "fire" emoji to signal this issue is now KILLED WITH FIRE. Guess I will have to wait until d.o is on D8… :)

dawehner’s picture

Note: this nearly caused a critical: #2579705: Frontpage fails after update

Status: Fixed » Closed (fixed)

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

larowlan’s picture

+++ b/core/modules/views/src/Plugin/views/style/Table.php
@@ -432,8 +432,8 @@ public function wizardSubmit(&$form, FormStateInterface $form_state, WizardInter
-  public function isCacheable() {
-    return TRUE;
+  public function getCacheMaxAge() {
+    return 0;

This introduced an issue with caching of table displays in views. Resolved in #2932083: Views Table style plugin breaks dynamic cache