FlagSimpleTest is failing due to render/page caching

Thanks for filing, I was too tired last night....clearly.

So the narrative of the test is :-

login as various users click unflag and flag and then logout to see what the anonymous users sees.

what the anonymous user receives is incorrect.

I have looked through all the recent D8 change records - and only one jumpes out.

Page caching has been turned on for anonymous users.

Anyway I can't spend much more time today looking at this ... I just thought I would share.

I'm pretty sure that this was working correctly at some point, are we changing the permissions at some point in the test and it was visible before and no longer is? But the anonymous role is a cache tag on the page cache, so that should work.

I've run the test locally, and basically the problem is that the anon user sees the flag link (which they don't have permission to).

Just fleshing out the steps to reproduce...

Give the anonymous user all flag permissions - then the flag/unfag link becomes visible.

From a cold cache

1) Flag the link
2) Observe "Flag this item" changes to "Unflag this item"
3) Click on 'Unflag this item" <--- this breaks no change in text.
4) drush cr and then reload page - see link now read "Flag this item"

I wasn't able to reproduce this locally. render/page cache is enabled. Will trigger a branch retest.

Just providing more information to my steps to reproduce (#4) - I created a flag labelled "flag One" and then from

Home>>Administration>>People ( permissions tab ) [ Under the "Flag" subsection ]

There are two dynamically generated rows with tick boxes for anonymous/authenticated/administrator users

"Flag flag one"
"Unflag flag one"

This is how I gave anonymous users the ability to modify the links I was talking about.
The steps to reproduce are important to me as every iteration of FlagSimpleTest takes over 2 mins.

Just trying to block out one element of the solution - There are many aspects to this and there are going to be lots of small changes in this issues resolution.

ActionLinkTypeBase::renderLink() -- we need to control the cachability of the link-render elements generated here.

on a per role? / per user?/ - this is the bit I am still thinking about.....

Also just making notes for myself on follow up issues- which are not critical.

The links generated by
ActionLinkTypeBase::renderLink(): The links contain an error in that they use the id attribute. In general there can be many of these links to flag a specific item on the same page so we need to use class not id.

I'm just posting early..... I just want to shape the conversation. This patch does not work - refactoring in a critical is not a good thing to do - so this needs justification.

When I look at flag.module - flag_entity_build_defaults_alter(().

Is the sole purpose of the function to identify all views/pages which the flag/unflag links appear and set the appropriate #cache array?

If that is the case -- well why not use a more conventional approach to caching and set the #cache tags on the link directly?

When I look at the flow through hook_entity_build_defaults_alter().
I see that it uses $flag->isFlagged() as an integrity check- I have transferred that protection to ActionLinkTypeBase::renderLink()

Something is wrong with my code.... but I think it just needs a little nudge.

AJAXactionLink::renderLink also needs consideration.

You can't set cache keys inside the code that is being cached. It needs to be known *before* we try to fetch from the cache. So no, your changes are not correct.

hook_entity_build_defaults_alter() ensures that we cache two different versions of the node, one that is flagged and one that isn't (additionally all the other variations, like different user permissions).

This is necessary, at least until we switch to a post render callback, where we could remove that code but with the downside of having to re-calculate the link again. But we already have a to check if the user flagged it or not, so the difference would not be that big .

The cache context used to be set for all cached elements. Now Drupal core does not do it anymore and all modules that have elements that depend on some kind of Context need to set that explicitly. This patch does that for the flag module.

Was debugging this with @LKS90, fix is correct, we shouldn't do anything more than just fix the test here and this is the correct fix, additional things like using #post_render cache should be done in separate issues.

This looks good. I don't what to hold up this critical going forward.

I have filed a minor issue for #2475223: flag/unflag links: Use attribute class not id.

As a separate issue I just want to note that when I repeat my steps to reproduce

Then the flag/unflag cycle still breaks when you give access to anonymous users for a specific flag.

This is troublesome, and may be critical in its own right ... but given the surprise expressed here that this is even a feature then should be resolved in another issue.

Separate issue with a failing patch would be good then, yes.

Because that *should* work. The cached node output is cached by a hash of the permissions, different permissions, different cache. And the page cache should be invalidated if permissions of the anonymous user change, see AnonymousUserResponseSubscriber. If anything added the permissions context, then the page cache entry is tagged with that role and changing it should invalidate it.