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.

Files: 
CommentFileSizeAuthor
#16 2342651-16.patch9.46 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,949 pass(es), 190 fail(s), and 78 exception(s). View

Comments

Wim Leers’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
10.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
Wim Leers’s picture

Title: Bubble+test cache tags for *all* config entities except DateFormat, Menu, SearchPage and Tour » Bubble+test cache tags for *all* config entities

Better title. No need to explicitly mention those config entities that already have test coverage. The issue summary shows them.

Wim Leers’s picture

The Role entity apparently uses entity_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 the Role entity were to use the rendered => TRUE cache tag, then that wouldn't work, or rather: we'd invalidate far too much.

e.g. _toolbar_get_subtrees_hash() already uses Role's list cache tag (user_roles).

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Title: Bubble+test cache tags for *all* config entities » Cache tags for *all* config entities
Berdir’s picture

well, except contact_message, which is never stored or rendered

Nitpick: 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 :)

  1. +++ b/core/modules/contact/contact.info.yml
    @@ -5,3 +5,5 @@ package: Core
    +dependencies:
    +  - field
    

    Why? It shouldn't depend on it. When you want to add configurable fields there, enable the module in the test.

  2. +++ b/core/modules/contact/src/Controller/ContactController.php
    @@ -104,6 +105,7 @@ public function contactSitePage(ContactFormInterface $contact_form = NULL) {
    +    $form['#cache']['tags'] = drupal_merge_cache_tags($contact_form->getCacheTag(), entity_get_form_display('contact_message', 'feedback', 'default')->getCacheTag());
    

    Why do we need the cache tag here exactly?

Wim Leers’s picture

RE: nitpick: true :)

  1. The tests fail without Field module!
  2. Because the contact page may be stored in a reverse proxy (Drupal's page cache/Varnish/…) and we want it to be invalidated if the configuration for it has been modified.
Berdir’s picture

1. 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?

Wim Leers’s picture

  1. The tests don't add a field. They only load the default form display for the contact form entities and save it.
  2. Glad you asked :) I was thinking about this, and spent a few minutes trying, but couldn't find a way to do it generically. I had to switch to another patch, but since you also point this out, I will definitely do that as part of this patch. I absolutely agree!
Berdir’s picture

1. Ah, the calculateDependencies() thing. Yeah, sorry :) I would suggest to just fix that:

      if ($this->entityManager()->hasDefinition('field_config')) {
        $field = FieldConfig::loadByName($this->targetEntityType, $this->bundle, $field_name);
        if ($field) {
          $this->addDependency('entity', $field->getConfigDependencyName());
        }
      }

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?

xjm’s picture

Berdir queued 1: 2342651-1.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: 2342651-1.patch, failed testing.

catch’s picture

Component: cache system » configuration entity system
xjm’s picture

Issue tags: +Triaged D8 critical
Wim Leers’s picture

Status: Needs work » Needs review
FileSize
9.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,949 pass(es), 190 fail(s), and 78 exception(s). View

Straight reroll, to see if it passes.

(One hunk conflicted, because it became obsolete, yay!)

Status: Needs review » Needs work

The last submitted patch, 16: 2342651-16.patch, failed testing.

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue tags: -beta target
Berdir’s picture

Is 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?

catch’s picture

Wim Leers’s picture

12:13:33 WimLeers: berdir: I see arguments in favor and against. In favor: I agree that this doesn't need to block release. Against: it's necessary to ensure everything is working well, it's necessary to enable page caching by default.
12:13:57 WimLeers: I've uncovered many caching problems in relation to this in the past year
12:14:14 WimLeers: But I agree that the remaining config entities are less important; with the exception of Views
12:14:38 WimLeers: i.e. if we choose to keep https://www.drupal.org/node/2381217 critical but to make https://www.drupal.org/node/2342651 major, then I think that makes sense
12:14:40 Druplicon: https://www.drupal.org/node/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 #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 => 35 comments, 7 IRC mentions
12:14:40 Druplicon: https://www.drupal.org/node/2342651 => Cache tags for *all* config entities #2342651: Cache tags for *all* config entities => 20 comments, 4 IRC mentions
12:15:34 catch: I think the Views issue is definitely critical.
12:15:59 catch: 'cos that's not just adding a cache tag, that's the actual bubbling being broken.
12:17:06 WimLeers: yeah that too
12:17:19 WimLeers: and that's the *true* blocker for having page caching enabled by default

In 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.

catch’s picture

Priority: Critical » Major
Issue tags: -Needs D8 critical triage

OK 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.

Wim Leers’s picture

Issue summary: View changes

Alex 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.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Category: Task » Plan

This is still not done.