Problem/Motivation

To reproduce, make sure you have the smart cache module enabled and your page is actually cached (check headers, possibly user a normal user, not an admin).

Create a view that shows content that is flagged by a certain flag.

Then flag something, go to that view, it should be shown now. (might not work if you already visited the view before that).

Then click on unflag. The link text will change because it is a lazy builder and called on the content cached by smartcache but the entry will still be there.

While at it, we should also check this with multiple users and make sure they each get their own version of the view. For that, we need the user cache context.

Proposed resolution

We need a special cache tag to invalidate views and other lists that show flags for a given user. For example, flags:$uid.

Then add that cache cag and the user cache context if not already there to that view. Tests could be built upon #2562971: Flag bookmark view is broken, once committed.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#9 flag-views-link-2655134-9-interdiff.txt944 bytesberdir
#9 flag-views-link-2655134-9.patch2.07 KBberdir
#5 2638878-05-TEST-ONLY.patch1.97 KBjhedstrom

Comments

Berdir created an issue. See original summary.

martin107’s picture

martin107 commented

To reproduce, make sure you have the smart cache module enabled

Did you mean

https://www.drupal.org/project/smartcache

as it looks like support for Drupal7/8 looks non-existent?

... asking for a friend :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Priority: Major » Normal

As an easier temporary, we could just ensure that such views are uncached, by adding CacheableDependencyInterface and returning 0 in getCacheMaxAge(). Or you can do that manually by disabling views caching.

We could also use the default flagging_list cache tag, but that would invalidate every cached view for all users if any user tags something. So rather pointless I think, better just disable caching.

@dawehner told me that one way to add the cache tag is to manipulate $this->view->element in query().

Changing to normal due to the available workaround.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Sorry, I meant dynamic page cache, I still refer to that as smartcache as it was called like that during development.

But it's actually not even relevant, it's enough to enable views caching.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Active » Needs review
StatusFileSize
new 2638878-05-TEST-ONLY.patch1.97 KB

I am seeing this bug over in the message_subscribe module, but it is hard to reliably reproduce. I decided to start with the bookmark flag test mentioned above, but could not get it to fail. This is the progress on updating that test to reproduce the error (but its passing).

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

I initially stumbled across this issue from one with message subscribe. However, that has been fixed over in https://github.com/Gizra/message_subscribe/issues/50, where the issue was that the tab utilizing the Views' output wasn't using that View's cache tags.

socketwench’s picture

socketwench commented

So what's the call here? We can't reproduce the bug, but I do think the test in the patch has value. Should we commit that, and postpone this issue if we can reproduce it, or just leave the patch out?

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

@Berdir, do you have thoughts on how to reproduce this (or perhaps it is no longer even an issue, in which case as @Socketwench said, we could commit this test).

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue tags: -Needs tests
StatusFileSize
new flag-views-link-2655134-9.patch2.07 KB
new flag-views-link-2655134-9-interdiff.txt944 bytes 
+++ b/modules/flag_bookmark/src/Tests/FlagBookmarkUITest.php
@@ -17,6 +18,7 @@ class FlagBookmarkUITest extends WebTestBase {
   public static $modules = [
+    'dynamic_page_cache',
     'views',

this is always enabled in tests.

The reason this didn't fail is while you gave the two users the same permissions, you did that with two different roles, which results in a different user.permissions hash, so they get different permissions.

This makes it fail. The other approach would be to give the authenticated user role those permissions.

Status: Needs review » Needs work

The last submitted patch, 9: flag-views-link-2655134-9.patch, failed testing.

ivnish’s picture

ivnish
he/him
commented
Status: Needs work » Closed (outdated)

8 years without any activity. I think we can close it as outdated. Please reopen if needed.