Problem/Motivation
For the third or so time, we're having the discussion whether we should have a separate interface for objects that don't make sense to be cached, but that affect cacheability of other objects.
While it's technically not essential to have a separate interface, every time a class implements CacheableInterface
that only affects cacheability, it's very, very awkward to implement the getCacheBin()
method… because you'll just end up writing return 'default';
since there isn't any cache bin actually associated with that object, since that object won't be cached: after all, it only affects the cacheability of another object!
If all this sounds esoteric, here's a practical example for you: menus can be rendered and can be render cached. In principle, they're perfectly cacheable. But, since we only want to show menu links (in rendered menus) that are accessible for the current user, the cacheability of the access check results for those links affects the cacheability of the rendered menu.
This time in #2287071: Add cacheability metadata to access checks, specifically #7 and later and #102 and later.
Proposed resolution
effulgentsia in #3:
What about just removing
getCacheBin()
fromCacheableInterface
? In general, I don't see why the cacheable object should specify its bin; rather, I think the code that's actually caching that object that should make that decision. Perhaps for blocks and some other plugins we want the plugin to specify that, but maybe that just means we want to addgetCacheBin()
as a separate method toBlockPluginInterface
?
Remaining tasks
TBD.
User interface changes
None.
API changes
Potentially a new interface, TBD.
Comment | File | Size | Author |
---|---|---|---|
#8 | 2339373-8.patch | 3.61 KB | Wim Leers |
Comments
Comment #1
Wim LeersNames that we've seen so far are
CacheabilityAffectorInterface
andCacheAffectorInterface
.Attached is a patch that shows what it'd look like.
Comment #2
Crell CreditAttribution: Crell commentedRelated, and some prior discussion: #2267843: Split off CacheableTrait
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedWhat about just removing
getCacheBin()
fromCacheableInterface
? In general, I don't see why the cacheable object should specify its bin; rather, I think the code that's actually caching that object that should make that decision. Perhaps for blocks and some other plugins we want the plugin to specify that, but maybe that just means we want to addgetCacheBin()
as a separate method toBlockPluginInterface
?Comment #4
Wim Leers#3: I was thinking something similar while creating this issue, but the way you described it, it makes a lot of sense.
Did precisely what you suggested and removed
getCacheBin()
fromHtmlFragment
&HtmlPage
entirely, because we don't render cache genericHtmlFragment
s (yet), we cache blocks, and when render caching pages, we have a hardcoded cache bin.@Crell: once this lands, I'll jump back onto #2267843: Split off CacheableTrait.
Comment #5
dawehnerI try to understand why you would ever want to override the cache bin for blocks? Isn't render always enough?
sorry but yeah I don't see why this is a point in favour of having that swappable for blocks. Can you elaborate a bit more in the IS, please?
Comment #6
Wim LeersGood question. I think that in the case of blocks, it's possible to have very complex, very expensive to generate blocks that are specific to your site's special snowflake functionality. I've seen it before that sites want to have a cache bin for their own set of modules, because that makes it easier to distinguish between "core+contrib" and "custom".
That wasn't an explanation/justification for having
BlockPluginInterface::getCacheBin()
, it was an explanation/justification for not havinggetCacheBin()
onCacheableInterface
:)Comment #7
dawehnerWell, those could also override the service which use those cache entries instead or even some custom cache factory implementations. Would it be in the scope of this issue to remove it? Just curious.
Comment #8
Wim LeersOh! You're right! And it'd be even simpler: they'd only have to implement the block alter hooks, and that's it.
Comment #9
Wim LeersComment #10
effulgentsia CreditAttribution: effulgentsia commentedPatch looks good to me, and I agree with #8's answer for how to handle special snowflake blocks that want a different bin.
What about this? Was there a use case for this that we need to have an alternate implementation for? Looks like the issue that added it was #2167039: Regression: page cache tags broken, also: some routes incorrectly use _controller -> No page object in template variables -> Fatal in seven_preprocess_page().
Comment #11
Wim Leers#10:
#cache
subproperty was being passed like thatcache_bin
header is used *nowhere*i.e. there's not a single reason to keep it.
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedWorks for me. https://www.drupal.org/node/2184553 will need an update after commit.
Comment #13
Wim LeersCR already updated.
Comment #14
webchickYay less code! Rationale makes sense as well.
Committed and pushed to 8.x. Thanks!