Problem/Motivation
Since #2040135: Caches dependent on simple config are only invalidated on form submissions, simple config (all config that aren't config entities) finally have cache tags!
Now we are finally able to associate cache tags of simple config that is used in the logic to render something.
Proposed resolution
Find all occurrences of using simple config in Drupal core that are used to render something. Every form, every render array that uses it, would ideally associate the appropriate cache tags.
If that's too much work, it'd be okay to only do it for forms that are going to be visible to end users (as opposed to only admins; many forms will only be visible to admins).
Remaining tasks
Find & replace them all!
User interface changes
None.
API changes
None.
Wanna help?
- Find more occurrences; search for
$this->config(
and\Drupal::config()
, check whether it's in something that gets rendered (a form or another render array. - Is this form or render array conceivably going to be shown to an anonymous user? If not, go back to step 1.
- Once you found another occurrence, look at the render array you're dealing with. Let's call it
$build
. Is it guaranteed to have an empty$build['#cache']['tags']
? In that case, just do$build['#cache']['tags'] = $config->getCacheTags()
. Is it possible that there already are cache tags? Then merge the simple config's cache tags with the existing cache tags:$build['#cache']['tags'] = Cache::mergeTags($build['#cache']['tags'], $config->getCacheTags());
. - Re-render the page (note that you might have to clear the render cache; while working on this, you might want to disable the render cache temporarily, see https://www.drupal.org/node/2259531) and verify that the
X-Drupal-Cache-Tags
header now contains the cache tag for the simple config. The cache tags listed in that header are sorted alphabetically. The cache tag you're looking for always begins withconfig:
, plus whatever config is being used. So if the code does\Drupal::config('user.settings')
, you'll want to make sure aconfig:user.settings
cache tag is listed in the header. - Congratulations! You've made Drupal 8 more cacheable :)
- Back to step 1, for more!
Comment | File | Size | Author |
---|---|---|---|
#58 | wherever_simple_config-2407765-58.patch | 20.9 KB | Wim Leers |
#24 | wherever_simple_config-2407765-24.patch | 21.31 KB | borisson_ |
#20 | interdiff-18-20.txt | 378 bytes | borisson_ |
#20 | wherever_simple_config-2407765-20.patch | 22.37 KB | borisson_ |
#18 | interdiff-16-18.txt | 550 bytes | borisson_ |
Comments
Comment #1
Wim LeersTo get started, here's an example; for
AccountForm
, which contains the user registration form, which is end-user facing. I've also included it inCommentViewBuilder
, which renders comments, and is hence also end-user facing.I've also included further info in the IS, in case you want to help.
Comment #2
Wim LeersComment #4
Wim LeersThis should fix at least the majority of the test failures, if not all.
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #6
borisson_Worked on this together with pjonckiere at the sprint in Ghent. Changed cache tags in:
- core/modules/aggregator/src/Controller/AggregatorController.php
- core/modules/contact/src/Controller/ContactController.php
- core/modules/forum/src/Controller/ForumController.php
- core/modules/search/src/Controller/SearchController.php
- core/modules/user/src/Form/UserCancelForm.php
- core/modules/user/src/Form/UserLoginForm.php
Comment #8
Wim LeersPatch looks great; thanks! :)
12 simple failures in a single test; could you fix those? :)
Unnecessary change.
Comment #9
borisson_Fixed unneeded changed line, test now passes for me locally.
Comment #10
Berdir@borisson_: Please provide interdiffs when you make changes to patches, so that it is easier to review. See https://www.drupal.org/documentation/git/interdiff
Comment #11
Wim LeersI think it'd be better if you did
$config = $this->config('system.site')
before it's used to build this form, then use it in the form, and then use$config
here too. That prevents us leaving this here by accident when thesystem.site
config is no longer needed.Same in these places.
Comment #12
borisson_Fixed remarks in #11, added interdiff this time, so Berdir is happy.
Comment #13
Wim LeersAwesome! Looking great :)
Now we still need the remaining ones to be converted. E.g. in
CommentForm
,TermForm
, and more.Comment #14
borisson_Comment #15
borisson_Added cache tags for the following files as well, couldn't find other forms where this is needed.
- core/modules/block/src/BlockForm.php
- core/modules/book/src/BookManager.php
- core/modules/book/src/Form/BookSettingsForm.php
- core/modules/comment/src/CommentForm.php
- core/modules/contact/src/ContactFormEditForm.php
- core/modules/language/src/Form/NegotiationSelectedForm.php
- core/modules/language/src/Form/NegotiationUrlForm.php
- core/modules/locale/src/Form/LocaleSettingsForm.php
- core/modules/taxonomy/src/TermForm.php
Comment #17
Wim LeersIndeed, all remaining forms are admin-facing forms.
The only one that I can still find is
UserController::resetPass()
. That's also end-user facing. Other than that, this is ready, I think :)Comment #18
borisson_Added password reset form. Will have alook at why the tests are failing in the meantime
Comment #19
aspilicious CreditAttribution: aspilicious commentedIs it a good idea to add a mergeConfigCacheTags($form, $config_tags) in formBase?
Comment #20
borisson_Found the problem that made alle tests fail.
Comment #22
Wim Leers#19: I've been asking myself the same question. Alternative idea: make
FormBase::config()
accept an optional$form
parameter, which then automatically merges the cache tags. No extra hassle. But still easy to miss.(It's always going to be easy to miss in the current architecture; we can only avoid that if we re-architect how forms are built; but that's too late at this point. All we can do is make it as simple/painless as possible.)
Assigning to Berdir to get his thoughts on this.
#20: hah, awesome fix :)
Comment #23
BerdirThanks for all your work on this.
A lot thinking out lout below, wondering which cache tags we really need.
Did not read everything, my sprint day is over now :)
I'm not sure if we should do it here, but considering that we're now doing this, should we consider to go one step further and actually cache this, with a list cache tag for aggregator items? Possibly in a separate issue.
The cache tag makes sense on a theoretical level, but if you think about it practically, this is the site name we are talking about. If we change the site name, then we have to invalidate every page anyway, because which page doesn't contain the site name? :)
(Also wondering why this is not a view already :))
I'm not sure I see why we need this in entity forms like this one?
I can not think of a scenario where we would be able to cache this form, why bother with cache tags?
Adding cache tags/code means we need test coverage usually, so we need a test scenario that makes sense (That's my answer to "Adding it doesn't hurt anything", it delays getting this issue in with the parts that we do need :)
This is also stuff that is only used in node/backend forms, but it being a service could theoretically mean that it will be used publicly ;)
Uhm. This is a ConfigForm, and $config is the config we are editing. If this would make sense, then we would have to add this by default to all config edit forms (Hint: I do not think this makes sense :))
CommentForm is something we might display to anonymous users, so this makes sense...
This one makes sense, wondering if the test below is enough test coverage for this, possibly?
Same here, can't see how we would ever cache this.
Comment #24
borisson_Removed Cache tags for BlockForm.php, BookSettingsForm.php & ContactFormEditForm.php. I'll see if I can change the test for CommentViewBuilder.
Comment #25
Wim LeersHrm, right, I should've explained it that way in the IS. Only if the form could conceivably be seen by an anonymous user, which means it might end up in the page cache/Varnish, only then do we need this. Updated IS.
Technically, it doesn't hurt to have cache tags set for uncacheable forms. It doesn't hurt. But this is indeed a good reason to only add cache tags where we can envision it being shown to an anonymous user: otherwise, we'll have to write a whole lot of tests for little value.
Sorry, borisson_ :(
As of #24, the changes in these files can also be reverted.
Comment #26
BerdirI'm not sure about AggregatorController, that is actually frontend, it's just the fact that it is the site name that made me think about it. (which is displayed everywhere, so everything would need to be invalidated I guess, do we need to worry about that?)
CommentDefaultFormatterCacheTagsTest is actually correct I think, that's from the comment view builder, which is frontend and cached, no?
Also note that I didn't get past "c" (in the module/file list of the patch) in my review above, so reviewed the changes and rest of the file as well.
Those changes are no longer necessary.
Same.
Same :)
Those are also backend forms.
This too.
Note that the configuration here does not affect the output, only if we log or not, so not relevant for the render cache.
Same for the term form, then?
This is also an entity form.
This too
=> There won't be much left, which is fine :)
Comment #27
borisson_I kept the changes in CommentDefaultFormatterCacheTagsTest, removed all other changes.
Comment #28
Wim LeersWhitespace-only change, should be reverted.
The changes here are unintentional.
Turns out this is basically the "one time login form", hence I failed when trying to write a test for it. This is *never* cached. So can be removed.
AccountForm
was removed as per #26.8, but that's wrong: it's also visible to the end user atuser/register
.Comment #29
Wim LeersWhat this now really needs, is tests.
Comment #31
Wim Leers#2403485: Complete conversion of comment form validation to entity validation, committed 5 minutes before #28, conflicted with #28. Straight reroll.
Comment #33
BerdirNeeds work for tests then.
Comment #34
Wim LeersBelow, a note for every single file touched in this patch, to ensure we don't miss any test coverage.
Added test for this one in
BookTest
.Added test for this one in
CommentAnonymousTest
.Test coverage: done.
Added test for this one in
ContactSitewideTest
.Added test for this one in
ForumTest
.We show the login block to anonymous users by default, hence this is indirect test coverage for
UserLoginForm
.Added test for this one in
UserRegistrationTest
.Added test for this one in
UserLoginTest
.Removed this now useless hunk.
Comment #35
dawehnerIt still feels wrong to have a
getCacheTags
method on ConfigBase, but well, we decided to go the couple everything route.ooh ooh ooh
It would be nice to have a one line comment why we expect it to be there. Its not obvious, really.
Comment #36
Wim LeersComment #37
Wim LeersAddressed both points in #35.
Comment #38
dawehnerThank you for addressing my feedback.
Comment #39
alexpottShouldn't we be adding the system.site cache tag to SystemBrandingBlock::getCacheTags()?
It looks like the patch in #37 does not contain the changes that the interdiff says it does.
Comment #40
Wim LeersIndeed, I uploaded the same patch as before :/ Stupid — sorry!
And indeed, that cache tag should be added there. Great catch.
Comment #41
Wim LeersFrom IRC:
Comment #43
Wim LeersTestbot--
Comment #44
BerdirLast few interdiffs look good, verified that those changes are actually in the patch ;)
Back to RTBC.
Comment #45
Fabianx CreditAttribution: Fabianx commentedRTBC + 1
Comment #46
amateescu CreditAttribution: amateescu commentedThis
mergeTags()
with ternary logic as an argument is used a lot and it's quite cumbersome to write, how about adding aCache::mergeRenderArrayTags()
(or similar) that receives a render array and does all the isset logic for you?It could even receive the render array by reference to make it even shorter to write.
A lot of these new test methods are added in web tests so they're doing a full Drupal install just for a single assertion. That's.. not very nice of them :)
This logic is also repeated a lot, how about introducing a method on the base test classes for it? It could receive the cache tag as the first argument and an optional path or Url object for the
drupalGet()
part.Comment #47
alexpottre #46.1 I agree a method here would be helpful - I don't think the render array should be passed in by reference though.
re #46.2 but are there obvious places to add this?
re #46.3 yep maybe an assertCacheTag method on WebTestBase
Comment #48
Wim LeersComment #49
Wim LeersSo, regarding #46.1 & #47.1: I agree in principle with #47, but OTOH I wonder if it's problematic to be copying potentially huge render arrays? I think it might actually be better in this case to work by reference because of that.
Also: if we add a method to do this for cache tags, we also want to do it for cache contexts.
So, in this first attempt, I'm adding a method that does work by reference. Looking forward to your feedback :)
But while working on that approach (let's call the above option A), I thought of another (let's call it option B), which I think is actually better, because it doesn't force us to add another method per type of bubbleable metadata.
Once #2429257: Bubble cache contexts lands (currently RTBC), it'd then allow us to easily absorb the cacheability metadata from cacheable objects (objects that implement
CacheableInterface
) and apply it to a render array. Even when more cacheability metadata is added to the render system in the future (like max-age and min-age), code won't have to change.Comment #51
Fabianx CreditAttribution: Fabianx commentedI agree with #49, but I am 100% for option B).
The reason being, that those are internal properties that no user of render arrays or CacheableInterface should know about.
Comment #52
amateescu CreditAttribution: amateescu commentedAbout #46.2, I don't know... any existing method in those test classes? :P
And #49 option B looks great.
Comment #53
Wim LeersGlad to see the agreement for #49 option b :)
Oops, config objects don't implement
CacheableInterface
, it's just thatConfigBase
has agetCacheTags()
method. (Neither do entities, there it is similarly just theEntityInterface
that has agetCacheTags()
method.)(See the relevant CR: https://www.drupal.org/node/2406455.)
So… that actually raises the interesting point… that in fact we do want config objects as well as entities to implement
CacheableInterface
, because that was always intended to be the sole generic interface to implement. But doing that here would probably be out of scope for this issue.Therefore I wonder if it wouldn't be better to address #46.1 using #49 option b in a follow-up issue?
Comment #54
Fabianx CreditAttribution: Fabianx commentedAgree, but we should probably then not introduce a new option a) in between, lets create the follow-up and back to RTBC for the original patch?
Comment #55
Wim LeersFollow-up filed: #2444231: Fix CacheableInterface so it makes sense (was: "Make Config objects & Entities implement CacheableInterface + add BubbleableMetadata::createFromCacheableObject()"). Re-uploading #48. Back to RTBC per #54.
Comment #56
amateescu CreditAttribution: amateescu commentedHere's an interdiff for #46.2, feel free to apply or discard it if you don't like it :)
Otherwise, RTBC++.
Comment #57
alexpottI like #56 - let's do it. amateescu++
Comment #58
Wim LeersThis must be the easiest patch I ever rolled.
Thanks, @amateescu :)
Comment #59
catchCommitted/pushed to 8.0.x, thanks!
Comment #61
Wim LeersYay! This leaves #2381217: Views should set cache tags on its render arrays, and bubble the output's cache tags to the cache items written to the Views output cache as the only big gap in cache tag (test) coverage :)