Updated: Comment #N

Problem/Motivation

There are a handful of calls like: cache()->deleteTags(). This is just not correct. This only works properly at the moment because core happens to only use db cache bins. If you were to switch any cache backends out, these calls will break.

This should not be possible to get wrong, but it is currently. Until we fix the whole bins/tags stuff. This should still be fixed here though.

Proposed resolution

Replace these with calls to Cache:: instead.

Remaining tasks

Do it

User interface changes

None

API changes

None

CommentFileSizeAuthor
#13 interdiff-2175823-13.txt707 bytesdamiankloip
#13 2175823-13.patch20.87 KBdamiankloip
#11 2175823-11.patch20.18 KBdamiankloip
#6 interdiff.txt3.07 KBolli
#6 2175823-6.patch20.19 KBolli
#4 interdiff-2175823-4.txt11.9 KBdamiankloip
#4 2175823-4.patch17.12 KBdamiankloip
d8-dont-use-cache-bin-tag-invalidation.patch5.37 KBdamiankloip

Comments

damiankloip’s picture

damiankloip commented
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
commented
Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community
olli’s picture

olli commented

What about calls like $this->cache->deleteTags() or \Drupal::cache($this->cacheBin)->deleteTags() or cache($this->cacheBin)->deleteTags() ?

damiankloip’s picture

damiankloip commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2175823-4.patch17.12 KB
new interdiff-2175823-4.txt11.9 KB

Yes, totally right. we need to get those too.

I left the backend tests doing what they are doing, just calling ->deleteTags on themselves. I think it's ok to leave that for now?

The last submitted patch, 4: 2175823-4.patch, failed testing.

olli’s picture

olli commented
StatusFileSize
new 2175823-6.patch20.19 KB
new interdiff.txt3.07 KB

Should we rename these two methods in CacheBackendInterface?

damiankloip’s picture

damiankloip commented

You fixed the tests, thanks!

I think we should see how #918538: Decouple cache tags from cache bins goes, but I don't think this issue should try to cover that.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Can we please open a follow up to put this information onto the CacheBackendInterface? I do not see how you should know from reading the CacheBackendInterface.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Assigned: Unassigned » catch

Since catch moved this to major, he's probably in the best position to ensure that it's solved to specification. :)

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

@dawehner we either need to do that, or go ahead with #918538: Decouple cache tags from cache bins which would remove this method from the interface altogether.

Was going to commit it, but it no longer applies (yes, already).

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review
StatusFileSize
new 2175823-11.patch20.18 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 11: 2175823-11.patch, failed testing.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review
StatusFileSize
new 2175823-13.patch20.87 KB
new interdiff-2175823-13.txt707 bytes

Doh

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Thank you

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

YES! THANK YOU!

This confused me enormously when I was first learning about cache tags. This is MUCH better.

Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.