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() from CacheableInterface? 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 add getCacheBin() as a separate method to BlockPluginInterface?

Remaining tasks

TBD.

User interface changes

None.

API changes

Potentially a new interface, TBD.

CommentFileSizeAuthor
#8 interdiff.txt1.98 KBWim Leers
#8 2339373-8.patch3.61 KBWim Leers
#4 2339373-4.patch3.01 KBWim Leers
#1 CacheabilityAffectorInterface-do-not-test.patch1.44 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue summary: View changes
FileSize
CacheabilityAffectorInterface-do-not-test.patch1.44 KB

Names that we've seen so far are CacheabilityAffectorInterface and CacheAffectorInterface.

Attached is a patch that shows what it'd look like.

Crell’s picture

Crell CreditAttribution: Crell commented
Related issues: +#2267843: Split off CacheableTrait

Related, and some prior discussion: #2267843: Split off CacheableTrait

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented

What about just removing getCacheBin() from CacheableInterface? 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 add getCacheBin() as a separate method to BlockPluginInterface?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Active » Needs review
FileSize
2339373-4.patch3.01 KB

#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() from HtmlFragment & HtmlPage entirely, because we don't render cache generic HtmlFragments (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.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

I try to understand why you would ever want to override the cache bin for blocks? Isn't render always enough?

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.

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?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

I try to understand why you would ever want to override the cache bin for blocks? Isn't render always enough?

Good 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".

sorry but yeah I don't see why this is a point in favour of having that swappable for blocks.

That wasn't an explanation/justification for having BlockPluginInterface::getCacheBin(), it was an explanation/justification for not having getCacheBin() on CacheableInterface :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Good 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".

Well, 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.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Title: Consider splitting up CacheableInterface (or at least providing a subset) » Remove getCacheBin() from CacheableInterface —
Issue summary: View changes
FileSize
2339373-8.patch3.61 KB
interdiff.txt1.98 KB

Oh! You're right! And it'd be even simpler: they'd only have to implement the block alter hooks, and that's it.

      if ($plugin->isCacheable()) {
        …
        $build[$entity_id]['#cache'] += array(
          …
          'bin' => $plugin->getCacheBin(),
        );
      }
      else {
        …
      }

      …
      $this->moduleHandler()->alter(array('block_view', "block_view_$base_id"), $build[$entity_id], $plugin);
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Title: Remove getCacheBin() from CacheableInterface — » Remove getCacheBin() from CacheableInterface
effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented

Patch looks good to me, and I agree with #8's answer for how to handle special snowflake blocks that want a different bin.

+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.php
@@ -82,9 +82,6 @@ public function onHtmlPage(GetResponseForControllerResultEvent $event) {
-      if ($bin = $page->getCacheBin()) {
-        $response->headers->set('cache_bin', $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().

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

#10:

  1. that was only added because every single #cache subproperty was being passed like that
  2. the cache_bin header is used *nowhere*
  3. we always want to store cached pages in a cache bin the page caching logic chooses

i.e. there's not a single reason to keep it.

effulgentsia’s picture

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

Works for me. https://www.drupal.org/node/2184553 will need an update after commit.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

CR already updated.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Yay less code! Rationale makes sense as well.

Committed and pushed to 8.x. Thanks!

  • webchick committed 4bd133d on 8.0.x 
    Issue #2339373 by Wim Leers, effulgentsia: Remove getCacheBin() from...

Status: Fixed » Closed (fixed)

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