Clean-up render cache when permission changes

lookout kid, its somethin you did

Damn. I thought Parmesan was involved in this ticket somehow.

:)

Ouch, having a maintenance constant check in there is pretty ugly. And needs to have an if defined() check.

#6 doesn't use maintenance mode constants

Tagging.

Why not tag every render cache entry with the user role for which it is cached? That seems to be a lot less ugly, and a lot more accurate?

Because we're trying to not have too many tags attached to a cache entry.

#17: I feared you were going to say that :) But do we have numbers? What's the maximum number of tags we should strive for? To me, it feels like we're preventing ourselves from exploiting the advantages of having cache tags by restricting ourselves from using it where it can be useful.
For example, over at #2094241: Cache tag the page cache we will inherently have a lot of tags. We're not preventing that in any way. Heck, you even RTBC'd that patch! :)
So what is the problem of having an additional cache tag here if it's not considered a problem in #2094241?

I have to admit that I don't have any numbers, I just know @catch said this in the entity render cache issue :)

The difference (I'd prefer to not call it a problem) is between "needed" and "not really needed", imo.

So I'm slightly wary of adding cache tags in the following cases:

1. The tag is never cleared (i.e. we're tagging pre-emptively).

2. The tag is cleared very, very rarely - to the point where just invalidating the entire cache is probably OK in that case.

3. The tag is similar to the 'content' tag now where clearing it is going to wipe out a hefty percentage of cache entries because it's overly generic.

For #1 I think we should absolutely avoid it - if a tag is needed we can add it when we need it. For #2 it's going to be tricky to come up with criteria for that case, so I'd be OK with adding the tags then looking at rationalising them later, probably better to be consistent at least at the moment.

The page cache patch is a bit different - the extra tags are the culmination of all the ones added on the page, but we're not adding any extra new tags in general there are we?

#20: we're not adding new tags in the page cache, but in a lot of cases, there's going to be a lot of tags — especially if you have a relatively complex content type with some entity references, and multiple such entities rendered on the same page. Which is not all that uncommon.

I do see the rationale for "it's just cheaper to clear everything when it affects the majority of cache entries". So I think you're saying that's what we should do, then, right? (Which would mean we should just continue to review the existing patch in #7.)

#21. There'll be a lot of tags on items in the page cache, but especially for the page cache if there's 100 tags, we're saying that's a better trade-off to clear when one of 100 items is updated, compared to any item on the site at all - even if there's a run-time cost on tag collection.

I'm mixed on the consistency of using tags for everything, vs. explicitly clearing cache bins when there's a massive configuration change - there's arguments in favour both ways.

#22: so how do we decide how to continue then?

For those specific tags where we're not sure I'm fine with either of these:

1. Use tags for everything, then have a tracking issue/@todos to evaluate if they're adding too much of a runtime cost.

2. Explicitly clear in these cases, then have a tracking issue/@todos to evaluate whether it'd be more consistent to just use tags.

Probably lean closer to #1 since that's harder to get wrong for people who haven't spent months thinking about cache tags.

Ok, thanks for the guidance. That means this patch needs to be rerolled, then.

Let's try this.

Test fails make sense, those just need to have the entity module enabled now.

Here we go.

I've seen that same failure in a different issue too, on a different testbot.

It should go away on re-test but it seems to be a pattern...

Pasting it here for now so that we don't lose the info:

Drupal\image\Tests\ImageFieldDisplayTest

The test did not complete due to a fatal error. Completion check ImageFieldDisplayTest.php 229 Drupal\image\Tests\ImageFieldDisplayTest->testImageFieldDefaultImage()

POST http://drupaltestbot1848/checkout/admin/structure/types/manage/article/fields/node.article.hukpahps/field returned 0 (0 bytes). Browser ImageFieldDisplayTest.php 251 Drupal\image\Tests\ImageFieldDisplayTest->testImageFieldDefaultImage()
array_flip(): Can only flip STRING and INTEGER values!
array_flip(Array)
Drupal\Core\Entity\FieldableDatabaseStorageController->loadMultiple(Array)
Drupal\Core\Entity\FieldableDatabaseStorageController->load(NULL)
entity_load('file', NULL, )
file_load(NULL)
Drupal\image\Tests\ImageFieldDisplayTest->testImageFieldDefaultImage()
Drupal\simpletest\TestBase->run()
simpletest_script_run_one_test('387', 'Drupal\image\Tests\ImageFieldDisplayTest')

Warning FieldableDatabaseStorageController.php 212 Drupal\Core\Entity\FieldableDatabaseStorageController->loadMultiple()

36: 2099105-36.patch queued for re-testing.

Thanks, Berdir :)

The comment as field patch went in with a few @todo's related to this and did a manual cache clear. This shouldn't be necessary anymore, can we remove those and see if that passes now? Search for the nid..

That said, we should also have explicit test coverage for this and not just implicit in some comment tests.

So far, so good.

Also noticed a similar case in EntityViewControllerTest, there's about rdf mapping configuration.

    // Enable the RDF module to ensure that two modules can add attributes to
    // the same field item.
    \Drupal::moduleHandler()->install(array('rdf'));
    // Set an RDF mapping for the field_test_text field. This RDF mapping will
    // be turned into RDFa attributes in the field item output.
    $mapping = rdf_get_mapping('entity_test', 'entity_test');
    $mapping->setFieldMapping('field_test_text', array(
      'properties' => array('schema:text'),
    ))->save();
    // Browse to the entity and verify that the attributes from both modules
    // are rendered in the field item HTML markup.
    \Drupal::entityManager()->getViewBuilder('entity_test')->resetCache(array($entity));
    $this->drupalGet('entity_test/' . $entity->id());

Should we put the same snippet in the RdfMapping entity?

Hm, looks like RdfMapping already has that, it's just broken right now. This fixes it and removes the manual cache clear, which gives us test coverage of that part...

Also re-roll, Role already has a postSave() method now, moved the cache clear thing into the same not syncing/installing condition. No need to do that there.

That said, looks like that issue (#2005644: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for in-place editing metadata, introduce drupalSettings.user.permissionsHash) *did* add a per-role cache tag (and check it on *every* page), which I still think is overkill* considering the number of times we re-save roles *and* that we usually save all of them, but now that we have it, wondering if we want to rely on it here as well...

Edit: * Oh, and it's mixing logic in a replaceable service and a not-so-replaceable Role::postSave() method? Oh well..

Grr. That view_mode stuff in the view builder is unecessary, we load them in the constructor, moved that into a method that is only called if we need it, given that we now use the view builders also to clear the render cache, that seems to make sense and allows to simplify the actual code.

I don't think that's the same, it says could not be stored, not is never stored, so it's theoretically possible that it's there and not TRUE.

Hm, that's even more unrelated to this than my change, which just changed the implementation but not the behavior of it. But don't care much other than we need someone else to RTBC it now ;)

Also, no idea on how to write unit test coverage for this, it calls out to functions so we can't use phpunit and not sure how to test it in a DUBT test. We already have web test coverage in comment.module, which is the only thing that i know is affected by this. Same for RdfMapping.

The comment.module tests are enough imo (having struggled with getting them to pass in the first place, I'm certain they cover this functionality).

48: 2099105-48.patch queued for re-testing.

Re-roll.

Tests passing again, no relevant changes, so back to RTBC.

1. +++ b/core/modules/user/lib/Drupal/user/Entity/Role.php
   @@ -140,7 +140,11 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
   -    Cache::invalidateTags(array('role' => $this->id()));
   +    if (!$this->isSyncing()) {
   +      Cache::invalidateTags(array('role' => $this->id()));
   +      // Clear render cache.
   +      entity_render_cache_clear();
   +    }
   

   It feels strange to only conditionally invalidate tags. Why is that necessary? A comment would be useful here, and in RdfMapping also.

   And if this must happen for user role entities, does that mean it should also happen for other config entities?

2. +++ b/core/modules/entity/lib/Drupal/entity/Form/EntityDisplayModeFormBase.php
   @@ -109,6 +109,10 @@ public function form(array $form, array &$form_state) {
       *   TRUE if the display mode exists, FALSE otherwise.
   ...
      public function exists($entity_id, array $element, array $form_state) {
   +    // Do not allow to add internal 'default' view mode.
   

   "TRUE if exists"
   vs.
   "do not allow to add", yet still returns TRUE.

   I think the comment here is wrong?

It feels strange to only conditionally invalidate tags. Why is that necessary?

Presumably there's a full cache clear after sync? But yeah could use a comment and fixing up the other comments.

Not exactly sure why the isSyncing() was added, let's see if it's necessary.

The comment is correct, this is the exists() callback, by returning TRUE, we prevent a new entity with that name being created.

Edit: If you have a better idea on how to write the comment, feel free to update the patch :)

#57: Thanks! And upon rereading, I now understand the comment. But I still think it's pretty confusing. I think the same comment as in isViewModeCacheable() is more clear:

// The 'default' is not an actual view mode.

rather than

// Do not allow to add internal 'default' view mode.

What do you think? (I'll reroll the patch if you agree.)

Hm, not sure, I still think that it should mention that we prevent that a view with that name can be created with that code. The first comment doesn't explain the code there. Note that the snippet there is in the display mode base class and applies to form and view modes.

Also "not an actual view mode" does not explain *what* it is :)

I tried several things now and can't come up with a good sentence, but I think it should mention that default is a reserved name and we do not allow to create a view mode with that name, so we always consider it to be already existing.

#59: interesting, I find the "do not allow to ADD" (emphasis mine) statement very confusing because it's in a method that does not add anything, but merely checks existence! If it were in a addFooBar() method, it'd be fine.

However, all this time I thought that this was a method that could be called by any code, but it turns out it exists just for this form… I wanted to reroll this patch to explicitly state that the exists() method was in fact a #machine_name callback. But then I noticed that this is how it's done all across Drupal core.

Sorry for holding this patch up a bit longer than needed, but I was genuinely confused.

Tentatively RTBC: the patch itself is good to go, but I think we may want to add explicit test coverage for permission changes affecting the render cache? Then again, the implicit test coverage in CommentNonNodeTest is possibly sufficient. I'll leave it to the core committer to judge that.

According to @amateescu, there are also more fails in #2151459: Enable node render caching that will be fixed by this, we're just not seeing those fails yet as those tests use nodes.

I tried but couldn't come up with an easy way to write explicit tests for this, we can't write unit tests as too many functions involved and a DUBT test would require something like extending the memory counter cache backend to track cache tags too, but that would also need a factory to instantiate them through a service as we can't directly inject it somewhere..

Yeah I think the implicit test coverage is enough for this. Committed/pushed to 8.x, thanks!