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
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. |
Comment | File | Size | Author |
---|---|---|---|
#215 | replace-2464427-215-interdiff.txt | 662 bytes | Berdir |
#215 | replace-2464427-215.patch | 74.04 KB | Berdir |
#212 | replace-2464427-212.patch | 77.83 KB | Wim Leers |
Comments
Comment #1
dawehnerComment #2
Wim LeersHere's an initial patch. Didn't run any tests. But this should capture most of the grunt work.
Comment #3
dawehnerMH, 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?
Comment #4
Wim LeersNo, we want to use it for exactly the same things. The intent is still exactly the same. Just consolidating on a single interface.
Comment #5
dawehnerYeah, sure, but previously we had a dedicated interface, which obviously help to document it implicit to be something else.
Comment #6
Wim LeersTrue.
Comment #8
Wim LeersThought: If we're worried about documenting the meaning, we could have
CacheablePluginInterface
implementCacheableDependencyInterface
and use{@inheritdoc}
+ extra comments to document the precise meaning.Comment #9
dawehnerWell 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.
Comment #10
Wim LeersComment #11
BerdirSo, 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...
Comment #12
borisson_Comment #13
borisson_This should fix most of the errors in the patch from #2.
Comment #14
borisson_Comment #15
Wim LeersHah, nice catch :)
Comment #17
borisson_Fixed a test, let's see if testbot agrees that this is a good thing.
Comment #19
borisson_Made the patch from the right directory now, this should apply.
Comment #21
borisson_Does it apply now?
Comment #23
borisson_This fixes the last test failure.
Comment #24
Fabianx CreditAttribution: Fabianx for Acquia commentedThis should operate on a CacheableMetadata object.
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 ...
Comment #25
Fabianx CreditAttribution: Fabianx for Acquia commentedComment #26
borisson_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. :)
Comment #27
Wim LeersNow 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.
Comment #28
Wim LeersReroll, 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.
Comment #30
Wim LeersTestbot--
Comment #33
Fabianx CreditAttribution: Fabianx for Acquia commented+1 to #27, yes working on that issues clearly showed a lot of confusion around that.
Comment #36
Wim LeersEh, #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.)
Comment #39
Wim LeersStill WSOD :( This will be a PITA to debug then…
Comment #40
Wim LeersThis blocks #2458763: Remove the ability to configure a block's cache max-age. See comments 27, 28, 29 there.
Comment #43
BerdirFound 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 :)
Comment #45
BerdirI 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?
Comment #47
Wim LeersThanks so much, berdir! :)
:D
Yes.
max-age = 0.
Comment #48
BerdirYes, 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.
Comment #49
Wim LeersIndeed, 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?
Comment #50
Wim LeersI said in #2 I only did the grunt work. That explains the failures: nothing was actually already using max-age + tags :)
Comment #52
Wim LeersThe 2 failures from #45 have been fixed. The 62 new failures are likely due to CIDs/cache tags being different.
Comment #53
Wim Leers#24:
With that, all feedback is now addressed.
Comment #54
Wim LeersComment #55
Wim LeersThis will fail — am fixing locally.
Comment #56
XanoComment #58
Wim LeersThis 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.This is the code in HEAD. Note that it's equivalent.
So why didn't HEAD prevent every single view from being cached?
The culprit. Rather than looking at the calculated max-age, it was merely looking at the cache plugin's max age.
Comment #59
Wim LeersI think that due to #59, this is critical. Tentatively marking as such.
Comment #60
dawehnerWell 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.
Are we sure that it makes sense for all of them here to set max-age 0, I kinda doubt that
Comment #61
dawehnerI doubt wim can set those d8 critical tags.
Comment #62
Wim LeersDemoting again.
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.)
Comment #63
Wim LeersThis 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.
Comment #65
Wim LeersThat'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:No fun ;) And that's also why the patch is suddenly so enormous.
Comment #67
Wim LeersOops, a debug statement made it in there.
Comment #68
Wim LeersThis should be green.
Comment #69
Fabianx CreditAttribution: Fabianx for Acquia commentedRTBC - if green. Do we need a change record?
Comment #70
jibranThis changes the existing views so should we write an update hook to re-save all the existing views?
Comment #71
catchYes we should probably have that and not update the db dump at this point.
Comment #72
Wim LeersSo … 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?Comment #73
Fabianx CreditAttribution: Fabianx for Acquia commented#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.
Comment #74
Wim LeersThis 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.
Comment #75
Wim LeersForgot the interdiff. Note that the DB dump change was reverted, but having that in the interdiff is rather… pointless/painful.
Comment #76
jibranWhy not just load enabled views?
Comment #77
Wim Leers@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
Comment #78
jibran@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.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.
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.
Comment #79
jibranUpdate hook works just fine. Wrote some tests unable to run those locally feel free to move forward. :)
Because we want to upgrade all views present in the storage.
Comment #81
jibranFixed some copy paste errors but it's still failing.
Comment #83
dawehnerHere is some suggestion how to fix the tests:
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.
Comment #84
jibran@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.
./core/scripts/dump-database-d8-mysql.php > core/modules/system/tests/fixtures/update/drupal-8.0.0-beta12.bare.standard.php
.Comment #86
jibranRemoved unused test module.
Comment #88
jibranYAR(yet another reroll) and fixed the test fails now we have a working upgrade path with tests.
Comment #89
jibranMissed the schema file changes.
Comment #90
jibran:/
Comment #92
Wim LeersNo, this patch specifically removed that!
Comment #93
jibranI know but update test doesn't pass without this so maybe we can remove this in follow up.
Comment #94
BerdirThe 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.
Comment #95
alexpottComment #96
alexpottSo 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()
.Comment #97
jibranNow it's blocked on #2535082: Allow hook_update_N() implementations to run before the automated entity updates. Rolled a patch on top of #2535082.
Comment #99
jibranIt 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.
Comment #100
catch#2535082: Allow hook_update_N() implementations to run before the automated entity updates is in now.
Comment #101
Wim LeersRe-testing #97, per #100.
Comment #104
Wim LeersComment #105
jibranYay!!! #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.Comment #106
Wim Leers#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:
s/cacheable/'cacheable'/
s/form/from/
Comment #107
jibranHere we go. Thanks @Wim Leers for the review.
Comment #108
alexpottConsidering 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.
Comment #109
Wim LeersComment #110
Wim LeersComment #111
Wim LeersFrom IRC:
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!
Comment #112
Wim LeersCR 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.Comment #113
almaudoh CreditAttribution: almaudoh commentedSo I have been doing the wrong thing all this while...didn't know about
drush config-export
. Is this documented anywhere?Comment #114
Wim LeersI also didn't know that. IDK about documentation about that.
Comment #115
alexpottWhat 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.
Comment #116
jibranMade 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.
Comment #118
jibran#2507727: Adding an "All taxonomy terms" field results in "Invalid parameter number" error. added a new view. Simple reroll.
Comment #121
jibranDoesn't seem relevant.
Comment #123
jibranRe-uploading the same patch for CI to test.
Comment #125
jibranAhh! schema validation hits one more time. :(
Comment #126
BerdirOk, 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?
Comment #127
BerdirI think we don't use @see in inline docblocks? See block_update_8001().
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...
Comment #128
Wim Leers#127.1: I think we do.
#127.2: good point!
Missing docblock.
Comment #129
jibranThanks @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?If these don't fix anything here then why not move these to the follow up issue?
RE: #127
RE: #27
Fixed doc.
Comment #130
Fabianx CreditAttribution: Fabianx for Acquia commentedWe should be good to go again! :)
Comment #131
jibran#2455125: Update EntityViewsData use of generic timestamp to use Field API formatter has landed so here is trivial reroll.
Comment #132
catchShouldn't this also assert that the cacheable metadata has been replaced correctly?
Comment #134
jibranFixed the tests and #132.
Comment #136
nlisgo CreditAttribution: nlisgo commentedGoing 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.
Comment #137
jibranWhy is it confusing?
Comment #138
nlisgo CreditAttribution: nlisgo commentedConfusing 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.
Comment #139
jibranOh I see. Ignore the interdiff in #131 it was just a reroll. It removes the conflict in views.install. Working on this right now.
Comment #140
jibranThis 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
locallyGot following updates.
After running updates got the following error.
I think I can run my updates on
drupal-8.bare.standard.php.gz
instead ofdrupal-8.0.0-cdd12a9.standard.php.gz
.Comment #141
jibranThis is how the full update function looks now.
Comment #143
Wim LeersThe second screenshot says:
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 undercache_metadata
:So… huh? I think we need @amateescu or @Berdir to figure this out.
Comment #144
BerdirSo, 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?
Comment #145
BerdirOpened #2550407: Strict config schema breaks update tests, switch to testing the configuration at the end instead to do exactly that, this has to be postponed on fixing that.
Comment #146
jibranI 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.Comment #147
Wim LeersI think it's mostly @dawehner that has to give his approval for that, not me.
Comment #148
Wim Leers#2550407: Strict config schema breaks update tests, switch to testing the configuration at the end instead landed, this is now unpostponed again.
Comment #151
Wim LeersJust a reroll. Had to resolve one conflict.
Comment #152
jibranAll 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.
Comment #153
Wim LeersWoot! 60% patch size reduction :D
But also 3 fails :(
Comment #154
jibranReverting #2550615: Update the DB dump to have a leading slash for the frontpage. fixes the test for me locally. Something went wrong over there.
Comment #156
BerdirNo, 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.
Comment #157
BerdirThis passes for me but as I said, I'm not sure that test is a good idea.
Comment #158
dawehnerHow does that exactly happen?
Comment #159
jibran@Berdir are we going to fix it in a follow up?
@dawehner Could you please review the
views_update_8002()
?Comment #160
Wim LeersWow, 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
Comment #161
jibranThat is why we have to get this in ASAP so that we can uncover more in issue queue with failing tests/patches.
Comment #162
dawehnerIt 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.
Here is a question. For code we use a "-" instead, see
Just to make life a bit more readable i'd suggest to define the two vars outside of the foreach
Comment #163
jibranThanks @dawehner for the review.
Comment #164
Wim Leers#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 :)
Comment #165
jibranCreated #2551579: Don't access frontpage before running updates in UpdatePathTestBaseTest::testDatabaseLoaded() for #156 fix.
Comment #166
jibranRemoved useless asserts those can be problematic in future and a minor doc fix.
Comment #167
jibranIn #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
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?
Comment #168
Wim Leers#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.
Comment #169
dawehner+1 for the RTBC
Comment #170
alexpottThis 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?
Comment #171
jibranI 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.
Comment #173
jibranIgnore #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.
Comment #174
Wim Leers@jibran So, does this then not address Alex Pott's feedback in #170?
Comment #175
jibranRE: #170
From #141
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 createViewExecutable
orDisplayPluginBase
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. :(Comment #176
jibranSee you after #2538108: Add hook_post_update_NAME() for data value updates to reliably run after data format updates.
Comment #177
Wim LeersThis was discussed on last Friday's EU criticals call. The conclusion was:
cacheable
key (and if it'sfalse
we map that tomax-age: 0
), but that as Views are modified/saved, they migrate naturally. In other words:cacheable
is deprecated (but still supported) in favor ofmax-age
.This reroll implements that.
Comment #178
moshe weitzman CreditAttribution: moshe weitzman at Acquia commentedWim will reroll this due to test fails.
Comment #179
Wim LeersComment #181
Fabianx CreditAttribution: Fabianx for Acquia commentedBack to RTBC
Comment #182
dawehnerThis is
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.
Comment #183
alexpott@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?
Comment #184
dawehnerExactly.
Comment #185
dawehner.
Comment #186
Wim LeersSo, like this then.
Comment #187
BerdirUh, 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?
Comment #188
dawehnerWell, 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:
Comment #190
Wim LeersRebased + 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.Comment #191
Wim LeersComment #192
catchTagging 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.
Comment #193
BerdirThe 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.
Comment #194
Wim LeersNote that part of the problem here is that plugins can opt in to implement this interface:
… 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:
Note that there we don't set
false
if the handler doesn't implement the interface.Comment #195
jibran#2458763: Remove the ability to configure a block's cache max-age is in now so what needs to be done here?
Comment #196
BerdirYeah, 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.
Why 0, this should be cacheable?
We're not adding this method to ArgumentDefaultPluginBase? I know, making them think and stuff, but this makes it a considerably bigger API change.
Comment is a bit wird but pre-existing (I don't know but yeah whatever, just cache it ;))
Comment #197
jibranNot planning to work on this at all until next week so please feel free to move it forward. Thanks.
Comment #198
Wim LeersComment #199
dawehnerSo we need test coverage?
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.
Comment #200
Wim LeersFirst, 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.Comment #201
Wim LeersWrong interdiff, sorry.
Comment #202
BerdirThat 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.
Comment #203
Wim Leers#196:
ArgumentDefaultPluginBase
does not implementCacheableDependencyInterface
, yet does have agetCacheTags()
method. That works, but is rather strange…#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:
The added test coverage does prove it works correctly though, the problem I discovered is unrelated to cacheability metadata.
Comment #204
Wim LeersOops, forgot some staged bits.
Comment #206
Wim Leers#196:
This adds that post-update hook.
Comment #207
Wim Leers#196:
This removes the run-time checks.
Comment #208
Wim Leers#196
… and in talking to @Berdir at the DrupalCon Barcelona sprint, he now thinks we can actually remove it from the schema.
Comment #212
Wim LeersThis 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.
Comment #215
BerdirFixed that test, didn't review yet.
Comment #220
Fabianx CreditAttribution: Fabianx for Acquia commentedRTBC - looks great to me!
Comment #221
catchCommitted/pushed to 8.0.x, thanks!
Comment #223
dawehnerAwesome, thank you!
Comment #224
Wim LeersGLORIOUS 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… :)
Comment #225
dawehnerNote: this nearly caused a critical: #2579705: Frontpage fails after update
Comment #227
larowlanThis introduced an issue with caching of table displays in views. Resolved in #2932083: Views Table style plugin breaks dynamic cache