Problem/Motivation

There was some discussion about this in #2304987: Don't invalidate cache tags of referenced entities, use entity list cache tags correctly, add test coverage for entity list cache tags. That was going to rename getListCacheTags() to getListCacheTag() but thankfully did not. It does not make sense to be singular as the return value is always an array of tags. It just happens that currently most implementations in core are just returning one.

I am sure this started off life as getCacheTags, so not sure when or why this was changed to the singular version. I guess for similar reasons that the referenced issue above wanted to?

We need to return consistency back to these methods, this makes a better API.

Proposed resolution

Rename this back to getCacheTags().

Remaining tasks

Renaming.

User interface changes

None

API changes

Return to the correctly named method.

CommentFileSizeAuthor
#17 2351847-17.patch31.68 KBdamiankloip
#16 2351847-16.patch30.91 KBdamiankloip
#8 interdiff-2351847-8.txt792 bytesdamiankloip
#8 2351847-8.patch31.68 KBdamiankloip
#6 2351847-6.patch30.91 KBdamiankloip
D8.getCacheTags.patch31.67 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Issue summary: View changes
oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g commented
Status: Needs review » Reviewed & tested by the community

Green.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented

Agree on RTBC. The cacheable interface has getCacheTags IIRC, so entities should be implementing that, which means they would be using this method anyway.

As contrib modules should implement to an interface not to an implementation, it is most likely that contrib will need to change to use cacheable interface anyway.

That said:

Why do entities not yet implement cacheable interface? And is there an issue I can follow?

Can't they have a maxAge and cache contexts?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, D8.getCacheTags.patch, failed testing.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
Issue tags: +Needs reroll
damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2351847-6.patch30.91 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 6: 2351847-6.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
2351847-8.patch31.68 KB
interdiff-2351847-8.txt792 bytes

doh.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
Status: Needs work » Needs review

Bot! Get to work!

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

This was never called getCacheTags() - the getCacheTag() method was added by Issue #2217749 by Wim Leers, Jalandhar, visabhishek, damiankloip: Entity base class should provide standardized cache tags and built-in invalidation.. Seems @damiankloip has changed his mind :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 2351847-8.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Hmm, maybe I'm going mad then!

Either way, having a method called getCacheTag that returns an array of tags is nuts.

I think this originally just returned a string maybe?

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented

Can we please make this implement the CacheableInterface then to standardize its usage?

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

How does that fit in with the getListCacheTags() method that now lives on EntityType classes?

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
2351847-16.patch30.91 KB

Here is a reroll. How about we punt that to another issue? As we will need to implement the all the other methods that live on CacheableInterface.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
2351847-17.patch31.68 KB

#16 contains the same mistake #8 had in.

The last submitted patch, 16: 2351847-16.patch, failed testing.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC then

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Needs a draft change notice.

Agreed this is just wrong and it's unlikely to have much/any contrib impact.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue tags: +API change, +Needs change record
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work

Setting back to needs work for CR

Status: Needs work » Needs review

damiankloip queued 17: 2351847-17.patch for re-testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs review » Reviewed & tested by the community

CN: https://www.drupal.org/node/2360567

catch’s picture

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

Committed/pushed to 8.0.x, thanks!

  • catch committed 985edb1 on 8.0.x 
    Issue #2351847 by damiankloip: Fixed Rename getCacheTag() to...
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue tags: -Needs change record

+1

Published the CR and made minor changes.

Why do entities not yet implement cacheable interface? And is there an issue I can follow?

Because we're gradually making things implement CacheableInterface. I agree that'd be lovely though. The nice thing is that this would merely be an API addition, not an API change.

Long-term, every "thing" (… but most things are entities!) in Drupal should implement CacheableInterface, so that we know for every "thing" whether it's cacheable, and if so, how (how long, by which contexts it should be varied, by which tags it must be invalidated …).

Related: #2329101: CacheableInterface only has a getCacheKeys() method, no getCacheContexts(), leads to awkward implementations.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue tags: +DX (Developer Experience)
damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Yes, that sounds like a nice place to be :)

Status: Fixed » Closed (fixed)

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