As part of #2124957: Replace 'content' cache tag with 'rendered' and use it sparingly, we added extensive test coverage for the most important content entity types' usage of cache tags in #2188565: Test coverage for Comment, Custom Block, Node, Taxonomy Term and User entity cache tags.

Aggregator's Feed and Item are two of the three last content entity types (the last is Shortcut) that don't yet use cache tags yet when rendering content.
#2217749: Entity base class should provide standardized cache tags and built-in invalidation added standardized cache tags to all entities, so we can use those.

Let's use those cache tags for rendered feeds/feed items and add basic test coverage.

This also allows us to cache the AggregatorFeedBlock forever by default :)

CommentFileSizeAuthor
#19 oops-2241229-19.patch1.38 KBWim Leers
#13 interdiff.txt584 bytesWim Leers
#13 aggregator_feed_item_cache_tags-2241229-13.patch22.53 KBWim Leers
#10 interdiff.txt1.1 KBWim Leers
#10 aggregator_feed_item_cache_tags-2241229-10.patch22.52 KBWim Leers
#6 aggregator_feed_item_cache_tags-2241229-6.patch21.47 KBWim Leers
#1 aggregator_feed_item_cache_tags-2241229-1.patch5.75 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
Status: Active » Needs review
FileSize
aggregator_feed_item_cache_tags-2241229-1.patch5.75 KB

Status: Needs review » Needs work

The last submitted patch, 1: aggregator_feed_item_cache_tags-2241229-1.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented 
+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorFeedBlock.php
@@ -180,4 +189,31 @@ public function build() {
+    if ($feed = $this->feedStorage->load($this->configuration['feed'])) {

Why do we have to load the items to get their tags? Shouldn't those be added when the items themselves are rendered?

Wim Leers’s picture

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

#3: I was asking myself that question also. From the discussions I've had with people on the Block cache patch (#2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags), the conclusion was that if at all possible, blocks should always return all the cache tags if they can.

That being said, I worked on #2241291: Regression: menu link-specific cache tags and #2241235: Shortcut/ShortcutSet entity types should use cache tags after this issue, and now I think we should get rid of the Item-specific cache tags, and give them the same treatment as MenuLink: only use the higher-level cache tag (Feed and Menu, respectively).

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented 
+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorFeedBlock.php
@@ -144,7 +152,8 @@ public function blockSubmit($form, &$form_state) {
+      $item_query = clone $this->itemQuery;

@@ -180,4 +189,31 @@ public function build() {
+      $item_query = clone $this->itemQuery;

hmm would probably be better to inject the factory instead. i find it cleaner than cloning.

/me noted it for the next time i inject entity query

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs work » Needs review
FileSize
aggregator_feed_item_cache_tags-2241229-6.patch21.47 KB

Did what I said in #4, and more:

  1. Removed Item-specific cache tags; just like MenuLinks don't get individual cache tags. Use the Feed cache tags instead, just like the Menu cache tags.
  2. Thanks to the above, fixing all remarks in #3 and #4 was trivial.
  3. Updated the EntityCacheTagsTestBase base class to support the "default" view mode that the Feed entity relies upon.
  4. Corrections for the Item entity:
    1. added missing access controller, which prevented references to aggregator feed items from showing up at all
    2. disabled render caching of feed items
    3. added missing URI callback, which prevented entity references to aggregator feed items with the "link" setting enabled to cause a fatal exception
  5. Corrections for the Feed entity:
    1. added missing access controller, which prevented references to aggregator feeds from showing up at all
    2. added cache tags for the "full feed view page", it was missing due to the way "full feeds" are rendered
    3. disabled render caching of feeds, to make it match the implementation, which prevents render caching of feed entities. Until now, it was actually disabled, but not marked as such in the Feed entity's annotation, hence EntityCacheTagsTestBase was trying to verify the render cache entries, even though they would never exist!
    4. Feed::postDelete() didn't call its parent, hence tag invalidation upon deletion was not happening!

In other words: this now also fixes fundamental problems in the Item and Feed entities. I had no choice but to do this, because I couldn't get it to pass the standardized entity test coverage otherwise.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Priority: Normal » Major

Tentatively bumping to major, because this fixes so much of the brokenness in aggregator.module (see #6 for details).

Status: Needs review » Needs work

The last submitted patch, 6: aggregator_feed_item_cache_tags-2241229-6.patch, failed testing.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

The code here looks great.

Are we planning to enable render caching of feeds or items in the future?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs work » Needs review
FileSize
aggregator_feed_item_cache_tags-2241229-10.patch22.52 KB
interdiff.txt1.1 KB

Simple fix for the failing test.

Are we planning to enable render caching of feeds or items in the future?

Render caching of items doesn't make a lot of sense; render caching of feeds does. I hope that we'll enable it, but it's definitely out of scope for this issue.

moshe weitzman’s picture

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

Thanks for test fix.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/aggregator/src/Entity/Item.php
@@ -184,4 +189,40 @@ public function getGuid() {
+  }

I'm confused by this.

The feed items give the Feed cache tag both for getCachetag() and getListCacheTag(), so aren't they handled by Entity:invalidateTagsOnSave() already?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs work » Reviewed & tested by the community
FileSize
aggregator_feed_item_cache_tags-2241229-13.patch22.53 KB
interdiff.txt584 bytes

#12: wow, well-spotted, sir! The flaw was in Item::getListCacheTags() though, it should've returned Feed::getListCacheTags(), not Feed::getCacheTag(). Sadly we don't have test coverage yet for list cache tags yet, or this would've been detected. Solving that is out of scope for this issue though.
Opened an issue for that: #2304987: Don't invalidate cache tags of referenced entities, use entity list cache tags correctly, add test coverage for entity list cache tags.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Assigned: Wim Leers » catch

Assigning to catch, since he already started review in #3 and #12.

catch’s picture

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

Yes this looks fine otherwise. Committed/pushed to 8.x, thanks!

  • catch committed 6c574a1 on 8.x 
    Issue #2241229 by Wim Leers: Aggregator Feed...
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue tags: -sprint

Hurray! :)

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented 
+++ b/core/modules/aggregator/src/Entity/Feed.php
@@ -23,6 +23,7 @@
+ *     "access" = "Drupal\aggregator\FeedAccessController",

+++ b/core/modules/aggregator/src/Entity/Item.php
@@ -21,10 +23,13 @@
+ *     "access" = "Drupal\aggregator\FeedAccessController",

@@ -184,4 +189,40 @@ public function getGuid() {
diff --git a/core/modules/aggregator/src/Entity/ItemAccessController.php b/core/modules/aggregator/src/Entity/ItemAccessController.php

diff --git a/core/modules/aggregator/src/Entity/ItemAccessController.php b/core/modules/aggregator/src/Entity/ItemAccessController.php
new file mode 100644

new file mode 100644
index 0000000..58d3a13

index 0000000..58d3a13
--- /dev/null

--- /dev/null
+++ b/core/modules/aggregator/src/Entity/ItemAccessController.php

+++ b/core/modules/aggregator/src/Entity/ItemAccessController.php
@@ -0,0 +1,43 @@
+namespace Drupal\aggregator;
...
+class ItemAccessController extends EntityAccessController {

the namespace or folder of ItemAccessController is wrong.
But seems this class is dead code anyway, since Item specifies FeedAccessController in the annotation.

I guess we should just rm ItemAccessController.php ?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Fixed » Needs review
FileSize
oops-2241229-19.patch1.38 KB

#18: Oops! You're right — strange that none of us noticed that!

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Title: Aggregator Feed & Item content entity types, and AggregatorFeedBlock should use cache tags » Followup: Aggregator Feed & Item content entity types, and AggregatorFeedBlock should use cache tags
Status: Needs review » Reviewed & tested by the community

Thanks! no problem, thanks for working on this

catch’s picture

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

catch
he/him
Primary language English
CreditAttribution: catch commented
Title: Followup: Aggregator Feed & Item content entity types, and AggregatorFeedBlock should use cache tags » Aggregator Feed & Item content entity types, and AggregatorFeedBlock should use cache tags

  • catch committed 9c86984 on 8.x 
    Issue #2241229 by Wim Leers: Followup: Aggregator Feed...

Status: Fixed » Closed (fixed)

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