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.
Comment | File | Size | Author |
---|---|---|---|
#17 | 2351847-17.patch | 31.68 KB | damiankloip |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedComment #2
oriol_e9gGreen.
Comment #3
Fabianx CreditAttribution: Fabianx commentedAgree 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?
Comment #5
Fabianx CreditAttribution: Fabianx commentedComment #6
damiankloip CreditAttribution: damiankloip commentedRerolled.
Comment #8
damiankloip CreditAttribution: damiankloip commenteddoh.
Comment #9
Fabianx CreditAttribution: Fabianx commentedBot! Get to work!
Comment #10
oriol_e9gComment #11
alexpottThis was never called
getCacheTags()
- thegetCacheTag()
method was added byIssue #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 :)Comment #13
damiankloip CreditAttribution: damiankloip commentedHmm, 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?
Comment #14
Fabianx CreditAttribution: Fabianx commentedCan we please make this implement the CacheableInterface then to standardize its usage?
Comment #15
damiankloip CreditAttribution: damiankloip commentedHow does that fit in with the
getListCacheTags()
method that now lives on EntityType classes?Comment #16
damiankloip CreditAttribution: damiankloip commentedHere 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.
Comment #17
damiankloip CreditAttribution: damiankloip commented#16 contains the same mistake #8 had in.
Comment #19
Fabianx CreditAttribution: Fabianx commentedBack to RTBC then
Comment #20
catchNeeds a draft change notice.
Agreed this is just wrong and it's unlikely to have much/any contrib impact.
Comment #21
catchComment #22
alexpottSetting back to needs work for CR
Comment #24
damiankloip CreditAttribution: damiankloip commentedCN: https://www.drupal.org/node/2360567
Comment #25
catchCommitted/pushed to 8.0.x, thanks!
Comment #27
Wim Leers+1
Published the CR and made minor changes.
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.
Comment #28
Wim LeersComment #29
damiankloip CreditAttribution: damiankloip commentedYes, that sounds like a nice place to be :)