Problem/Motivation
Before Drupal 8 ships, we want to ensure that all entity cache tags work correctly. Every content entity (well, except contact_message
, which is never stored or rendered) already has test coverage. They're arguably also the most complex ones.
But, config entities are also very important. Without proper cache tag bubbling and test coverage, it's extremely likely that many bug reports will be filed about things "not working", due to necessary cache tags not being associated.
Worse, yet: without proper cache tag invalidation, it will seem like CMI deployments are broken! When changing config ("things in CMI") through the UI, the invalidation is performed by the UI, not by the config system!
Proposed resolution
Add test coverage for all config entities that don't already have test coverage.
Remaining tasks
Entity type | Cache tag bubbling | Test coverage |
---|---|---|
Action |
TBD | TBD |
Block |
Already done. | |
BlockContentType |
TBD | TBD |
CommenttType |
TBD | TBD |
ConfigurableLanguage |
TBD (a non-comprehensive integration test was added at #2428837: Adding/updating interface translations should invalidate page & render caches) | TBD |
ContactForm |
✓ | ✓ |
DateFormat |
Already done: #2241275: DateFormat cache tag: don't set the cache tag, but invalidate the entire render cache? | |
Editor |
✓ | TBD |
EntityDisplayMode |
TBD | TBD |
FieldConfig |
TODO | TODO |
FilterFormat |
✓ | ✓ |
FieldStorageConfig |
TODO | TODO |
ImageStyle |
Already done. | TODO |
Menu |
Already done: #2179083: Rendered menus (e.g. menu blocks) should set cache tags to inform the page cache. | |
Migration |
Not applicable. | |
NodeType |
TBD | TBD |
ResponsiveImageMapping |
Already done. | TODO |
Role |
TBD | TBD |
SearchPage |
Already done: #2241249: First step in making search results pages cacheable: add the associated SearchPage's cache tag. | |
ShortcutSet |
TBD | TBD |
Tour |
Already done: #2241267: Make tours set cache tags. | |
ViewStorage |
Already done: #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 | |
Vocabulary |
TODO | TODO |
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#16 | 2342651-16.patch | 9.46 KB | Wim Leers |
Comments
Comment #1
Wim LeersComment #2
Wim LeersBetter title. No need to explicitly mention those config entities that already have test coverage. The issue summary shows them.
Comment #3
Wim LeersThe
Role
entity apparently usesentity_render_cache_clear()
to clear all render caches when permissions change.Druplicon: WimLeers: 5 hours 16 min ago <berdir> tell WimLeers wondering if we should remove entity_render_cache_clear() and use a cache tag delete on rendered instead for Role? possibly with the same pattern as we do in DateFormat?
We can't and shouldn't use the same pattern as
DateFormat
. The difference is that changing date formats only ever is going to affect *render* cache items. But role changes can also affect non-render cache items. We already have cache items that cache things per role. If those cache items want to be invalidated whenever a role's permissions change, they should be tagged with that role's cache tag. If theRole
entity were to use therendered => TRUE
cache tag, then that wouldn't work, or rather: we'd invalidate far too much.e.g.
_toolbar_get_subtrees_hash()
already usesRole
's list cache tag (user_roles
).Comment #4
Wim LeersComment #5
Wim LeersComment #6
BerdirNitpick: They are rendered (in the e-mail) and stored (in a test and with https://www.drupal.org/project/contact_storage). But not in the frontend and therefor irrelevant for cache tags, that part is correct :)
Why? It shouldn't depend on it. When you want to add configurable fields there, enable the module in the test.
Why do we need the cache tag here exactly?
Comment #7
Wim LeersRE: nitpick: true :)
Comment #8
Berdir1. Yes because the test adds a field? Then the test should depend on field.module, not contact. Contact should work fine as long as you don't add fields.
2. Can/should we make some of that more generic then? It's a normal entity form, what happens on a cached user registration form for example, and it's also valid to have public forms for creating certain node types. Should an entity form include the relevant tags by default?
Comment #9
Wim LeersComment #10
Berdir1. Ah, the calculateDependencies() thing. Yeah, sorry :) I would suggest to just fix that:
in there. Then it is green.
See #2079427-13: Core/Entity depends on classes / functions from field.module, where I found this, @swentel agreed with making that conditional.
2. Maybe EntityFormDisplay::buildForm() to add that part? It has $form and there we add the field widgets. Not sure about the contact_form/bundle entity. What exactly is the use case there? What can change in it that affects that page? I don't think it's something generic (needed for all content entity forms with bundles?)? node types have similar stuff, but that now works through field overrides, so maybe we should somehow cover them?
Comment #11
xjmComment #14
catchComment #15
xjmComment #16
Wim LeersStraight reroll, to see if it passes.
(One hunk conflicted, because it became obsolete, yay!)
Comment #18
Wim LeersComment #19
Wim LeersComment #20
BerdirIs this really a critical?
This issue is about adding test coverage for things like changing node types and verifying that everything that needs to be updated is invalidated. Yes, we might find bugs there.
But I would say that we do not anticipate that we have to make any API changes to fix those, just add cache tags in the right places, and I also don't think that those bugs are release blocking. I'm sure that we have dozens of bugs that are much more problematic than some page not immediately being updated when you change something in a config entity.
I'm all for improving test coverage and I will help and review this issue to get done, but I don't see why it is blocking a release?
Comment #21
catchComment #22
Wim LeersIn short: I'm fine with this becoming major, as long as #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 remains critical.
Comment #23
catchOK I don't think the Views issue is in any danger of being demoted.
If we discovered something really bad as a result of this issue (or if we end up with dozens of bug reports about cache staleness) we could bump this back up again or open a critical sub-issue for it.
But I think we're far enough along with cache tags generally that the remaining config entities here don't need to block anything. Obviously we should still do this, and it's the sort of change that could go into any patch-level release.
Comment #24
Wim LeersAlex Pott asked me to update this issue, but as far as I know, not much has happened in this area. We've done lots of cacheability fixes, and added lots of test coverage for specific cases. But this issue is about making sure that each config entity type has its own dedicated test coverage ensuring that the specific operations on that config entity type invalidate cache tags as necessary. We haven't done any work in that area all this time AFAIK.
To be able to be 99% confident it's all working, the remaining rows still need to be fixed, I'm afraid. The good thing is that we can do this post-8.0.0: the risk is already sufficiently contained (thanks to the integration tests that we have and the implicit manual testing we do), and fixes are highly unlikely to require API changes.
Comment #28
Wim LeersThis is still not done.
Comment #36
geek-merlinThis is fixed now, isn't it?