Deleting a flag should clear entity caches

We need to make sure that the cache tag for the flag is added to the render array in hook_entity_view().

We need to make sure that the cache tag for the flag is added to the render array in hook_entity_view().

I think the problem is a layer deeper than that

ActionLinkTypeBase:buildLink() is it better that we declare the appropriate cache tags every time we build a link?

In that case we need a cache tag for both the flag and the entity.

I may be missing something!

Thank for the response...

I need to lay out the chain of thought .... I am missing something.

The reason for using the place holder :-

To separate the thing subject to rapid change ( the flag link )
away from the thing that would otherwise be cached ... the article - or whatever.

I get that but

By linking the #lazy_biulder element to the rapidly changing flag -
does that not invalidate the separation argument and force the article to become dependant on the flag anyway?

Regardless --- I tried implementing the suggestion....and note it does fix the problem!

I think I may have made a mistake .... it is easy to screw up the keys in a render array..
but I am going to post away just in case the problem is deeper than our understanding.

The lazy builder exists because whether the flag shows flag or unflag varies per user (and if he can actually access it). We don't want to cache a version of the whole node for every user, so we use a lazy builder to calculate only that link.

Changing the flag doesn't happen often. If you do that, then might have results that we can't handle. For example, if you change the field to show not as an extra field but as a link? That's not something we can handle in a lazy builder.

So if that happens, we want to invalidate all currently cached nodes that show that flag.

The code looks OK to me. There are API's to collect and merge cacheability metadata but for something that's as simple as this, doing it directly is fine I think.

I went looking for a reason why the patch has no effect.

\Drupal\Core\Render\Renderer:doRender()

Line 308 is where some #lazy_builder integrity checks begin

Here is a code fragment from line 323, if I read it correctly our new #cache key is automatically be overridden.

      $supported_keys = [
        '#lazy_builder',
        '#cache',
        '#create_placeholder',
        // These keys are not actually supported, but they are added automatically
        // by the Renderer, so we don't crash on them; them being missing when
        // their #lazy_builder callback is invoked won't surprise the developer.
        '#weight',
        '#printed'
      ];

If I'm reading this right, this issue is stuck at "needs work"?

Yep, we have a patch that has no effect.

Speaking for myself ... I am still mulling things over ...

lazy builder can become stale for all sorts of reasons, which are particular to the unique circumstance.

The canonical way of lazy builder invalidation seems to assume things which don't work as soon as you want to say I know best - this is the way you optimise for our case..

As far as I have got ... it sounds like we need to provide a good trail blazing example of why and how core need to become more flexible.

Sorry if it sound like a council of despair ... I am sure we will get there :)

I added a test for this.

Fixed moving out the lazy_builder.
Moved before checks so it will work also for disabling flag, so I removed the cache resets and added disable test also.

#2617384: missing test coverage for flag deletion has been committed. But I'm not sure we should merge with that. That test is in AdminUI, it's about testing the UI. This is specifically to test render cache invalidation when editing/deleting a flag.

That said, looking through the tests, enable/disable is apparently already tested in FlagEnableDisableTest, so I think we don't need that. Given that, if only delete is remaining, then merging it into that test would work for me.

@joachim: ?

1. +++ b/flag.module
   @@ -168,6 +169,8 @@ function flag_entity_view(array &$build, EntityInterface $entity, EntityViewDisp
      foreach ($flags as $flag) {
   +    $build['#cache']['tags'] = Cache::mergeTags($build['#cache']['tags'], $flag->getCacheTags());
   +
        // Do not display the flag if disabled.
   

   Note that we moved it before we check if it's enabled and its setting and have it outside of the lazy builder. It has by design a different effect in there and doesn't bubble up.

2. +++ b/src/FlagService.php
   @@ -231,12 +231,6 @@ class FlagService implements FlagServiceInterface {
   -    $this->entityManager
   -      ->getViewBuilder($entity->getEntityTypeId())
   -      ->resetCache([
   -        $entity,
   -      ]);
   

   We went through all resetCache() calls and removed all that we could. You might want to move some into separate issues.

   This one is pointless since we switched to a lazy builder. invalidating the entity cache after flagging doesn't help us in any way and is unneeded.

3. +++ b/src/Form/FlagDisableConfirmForm.php
   @@ -91,11 +91,6 @@ class FlagDisableConfirmForm extends ConfirmFormBase {
   -    // Invalidate the flaggable render cache.
   -    \Drupal::entityManager()
   -      ->getViewBuilder($this->flag->getFlaggableEntityTypeId())
   -      ->resetCache();
   

   this is duplicated anyway by the code in pre (or post) Save and double so by the new cache tag that we add to the output.

   I think it would be nice to remove the one in preSave() too but that would require that we also add a list cache tag to the view alter. Which would be a small overhead since we'd invalidate everything unless we define a per-entity type list cache tag for flags that we invalidate. Also, other ways of displaying the flag would need that cache tag too.

4. +++ b/src/Tests/AdminUITest.php
   @@ -246,7 +246,6 @@ class AdminUITest extends FlagTestBase {
        // Load the vocabularies from the database.
        $flag_storage = $this->container->get('entity.manager')->getStorage('flag');
   -    $flag_storage->resetCache();
        $updated_flags = $flag_storage->loadMultiple();
   

   This was never needed, config entities have no static cache. Also a wrong comment above this.

5. +++ b/src/Tests/FlagSimpleTest.php
   @@ -219,6 +221,7 @@ class FlagSimpleTest extends FlagTestBase {
    
   +    $this->drupalLogout();
        $user_1->delete();
   

   deleting the user we're currently logged in with confuses the test system. I still thinks that we are logged in as that (and has no way of knowing otherwise). on the next drupalLogin(), it then tries to log out first, which gives an access denied.

Merged the tests and fixed the comment of the point 4.

Wrong name.

1. +++ b/src/Tests/AdminUITest.php
   @@ -258,6 +257,12 @@ class AdminUITest extends FlagTestBase {
   +    // Flag node.
   +    $this->drupalGet('node/' . $this->nodeId);
   +    $this->clickLink('Flag this item');
   +    $this->assertResponse(200);
   +    $this->assertLink('Unflag this item');
   

   I don't think we need to flag. Just update the test to look for Flag this item instead.

2. +++ b/src/Tests/FlagSimpleTest.php
   @@ -233,6 +233,7 @@ class FlagSimpleTest extends FlagTestBase {
    
        // Delete the user.
   +    $this->drupalLogout();
        $user_1->delete();
   
   @@ -316,6 +317,7 @@ class FlagSimpleTest extends FlagTestBase {
        $this->assertEqual(1, $count_flags_before);
    
        // Delete  user 1.
   +    $this->drupalLogout();
        $user_1->delete();
    
   

   you can remove those.

Done.

+++ b/src/Tests/AdminUITest.php
@@ -257,12 +257,6 @@ class AdminUITest extends FlagTestBase {
   public function doFlagDelete() {
-    // Flag node.
-    $this->drupalGet('node/' . $this->nodeId);
-    $this->clickLink('Flag this item');
-    $this->assertResponse(200);
-    $this->assertLink('Unflag this item');

Now you deleted too much IMHO. We want to have a positive test, and make sure that we look for a specific string (like Flag this item) before deleting and then the same after deleting.

assertNo* assertions are always problematic. E.g. the string you're not expecting could change and then you wouldn't assert anything anymore.

Ok, I readded the positive test and a test that actually fails.

I think this is good to go then...

Thanks everyone!