Tests are not working because Dynamic page cache was committed in core.

Hm.

We should try to fix the tests properly, the module eventually needs to work with dynamic page caching. I guess we could commit this as a workaround but we should have an issue open to fix it.

Very likely, the problem is in flag_entity_build_defaults_alter(). That alters how e.g. a node is cached and it caches two different versions of it, one with a flag link and one with an unflag link.

The problem is that the page cache doesn't know of those two variants and which version to use when.

Likely the best problem to solve this is to replace this completely and instead use a lazy_builder to generate the link. There's already a related issue open that needs that anyway: #2466703: Flag reload and AJAX link types are missing a token. So we could commit this and revert this change again there.

See NodeViewBuilder for an example of this, you just register a #lazy_builder instead of actually doing something.

+++ b/src/Tests/FlagConfirmFormTest.php
@@ -66,6 +66,8 @@ class FlagConfirmFormTest extends WebTestBase {
+    \Drupal::service('module_installer')->uninstall(['dynamic_page_cache']);

If we commit this as a workaround, we need to add a @todo here, referencing the core CR and our own issue for the proper fix.

Upping to critical since this is blocking tests.

I haven't looked at the code nor the test failures.

But in general, failures with Dynamic Page Cache enabled indicate that something is being cached across authenticated users when it should not be. I.e. a certain cache context is missing that tells Drupal something is actually per user, per session, per something.

I think we should maybe fix this now rather than postpone it. There aren't any pressing issues that need work on or big patches that would get broken by this, so we should focus our attention on this.

I think we should maybe fix this now rather than postpone it.

I think you may be right, but I have the sinking feeling that to fix this we have to disable the reload link type if caching is enabled, and then load the flagging state over AJAX.

I chatted with larowlan about this earlier tonight. His suggestion was to try one of two paths:

1. Use #lazy_builder similar to what's done in CommentDefaultFormatter to render the flag links after the rest of the page.
2. Use cache tags on the flag entity for the user: CacheableMetadata::createFromObject($flag)->applyTo($elements);

He suggested talking to Wim Leers for ideas.

Can you point to the relevant pieces of code in Flag? Happy to help here.

Confused, isn't 1. exactly what I described fairly detailed in comment #3?

I don't think cache tags help, unless we'd cache per-user in the first place, but that's a bad idea.

One example that needs to be updated is flag_entity_view(). Instead of actually displaying a link, we just need to register a lazy builder and then display the actual link in there.

#3 and #13 definitely sound good.

We need something like this, basically.

I found a bug in the lazy builder logic, it doesn't work correctly for top-level #type elements, so I put it in a sub-element. I think we need to re-enter the whole render process, to also consider #access on the top-level element and possibly other logic that happens before calling it. I guess Wim will open an issue for this, I need to go for now.

Did run just one test but that passed. Needs work for documentation and stuff, of course.

Admittedly, I was trying to summarize what I didn't quite understand well past my bedtime.

Ok, a lazy builder must return an array, not so great DX but easy to fix. This passes for me locally.

RTBC - Looks great to me.

I love how this isolates the things and makes it unnecessary to tamper with the entities #cache.

The below is for discussion with berdir / Wim and not meant for this patch, just giving additional context from the 'core' side of things.

1. +++ b/flag.module
   @@ -183,56 +182,53 @@ function flag_entity_view(array &$build, EntityInterface $entity, EntityViewDisp
   +    // The actual render array must be in a nested key, due to a bug in
   +    // lazy builder handling that does not properly render top-level #type
   +    // elements.
   +    return ['link' => $link];
   

   It is a little on purpose, because that way #type can specify it wants to be lazy builded IIRC.

2. +++ b/flag.module
   @@ -183,56 +182,53 @@ function flag_entity_view(array &$build, EntityInterface $entity, EntityViewDisp
   +  // Lazy builders must always return a render array.
   +  return [];
   

   Yeah, should probably cast NULL into [].

Yay! Tests pass!

Do we need render cache tags? I don't see that we have any at the moment, so that can be taken care of in a follow-up if we should have them.

> Ok, a lazy builder must return an array, not so great DX but easy to fix.

Same way a form builder has to return at least an empty array. But yeah, casting NULL to [] would be a little bit of DX sugar.

+++ b/flag.module
@@ -183,56 +182,53 @@ function flag_entity_view(array &$build, EntityInterface $entity, EntityViewDisp
+  $flag_service = \Drupal::service('flag');

I keep looking at this and thinking, "Oh, that's why this shouldn't be a straight function..." It clearly works as one, but what I saw in CommentDefaultFormatter was in a separate service. I'm not sure what the implications are for not using proper DI here, but that can be in a follow up issue.

@Fabianx: We need to find a different place (aka core issue) to continue discussing this. Yes, #type's should be allowed to have lazy_builder's too, but that shouldn't stop us from at least loading the defaults again after calling it based on getInfo(). We'd just do it twice. Or, i was wondering if we shouldn't just do $this->doRender($new_elements) at that point or something like that. What if you specifiy top-level #access on a lazy-builder return value? That won't be checked either right now.

@joachim:

We don't need cache tags to fix this. What we do need to do is invalidate the {$entity_type_id}_view cache tag ($view_builder->resetCache()) whenever we create, change or delete a flag, somehow trough the type plugin. I was thinking about adding a cache tag for the flag, but the problem is that we can't solve it for *new* flags anyway, so those need to invalidate all existing render cached nodes. To reproduce, create a node first, then create a flag, or create a second flag, that won't be shown until a cache clear happens. I think.

But that's a separate issue and pre-existing situation.

@socketwench: Yeah, I was just too lazy to create a service and all. We could put it in flag service i guess that would be an easy change.

Committed, with just a couple of minor fixes to typos in comments.

Several follow-ups / related issues:

- move this to the flag service? #2568129: Move lazy builder callback into a service.
- add cache clearing #2568131: clearing entity caches on flag changes