Invalidate /aggregator page cache when feeds update

I verified that this is still an issue. The steps to reproduce it worked for me.

This test demonstrates the bug by requesting /aggregator, updating feed items, then requesting /aggregator again. The test fails correctly on my local.

Unfortunately, the test is no good because I think it will invalidate the assertions that follow it. Those assertions were added by #3272873: Adjust title → and they depend on the page not being cached. They were a problem for getting a failing test though. Requesting /aggregator?page=2 causes /aggregator to start working properly. I don't know enough to understand why that was happening, but I assume that requesting a different page's context somehow cleared the first page's context too. As a result, it took me a while just to get a failing test. I haven't figured out if the tests for both issues can go into the same method yet. Maybe if I delete all the items and then re-update the feed...

The last submitted patch, 6: 3214630-6-test-only.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Nevermind. This was easy to solve. All it needs is a cache clear before the next assertion. I verified that the cache context assertion is still working properly by undoing the fix from #3273873: Aggregator page contents could be empty due to missing cache context → and testing it locally. It failed just like it's supposed to. Here's a good failing test.

The last submitted patch, 9: 3214630-9-test-only.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

This passes and works for me locally.

I'm hoping to review this tomorrow

+++ b/tests/src/Functional/AggregatorRenderingTest.php
@@ -96,9 +96,20 @@ class AggregatorRenderingTest extends AggregatorTestBase {
+    // Clear the cache so that the paginated cache context can be tested.
+    drupal_flush_all_caches();

Instead of this, we *should* be able to just call $feed->save(), because the Aggregator item includes 'aggregator_feed_list' in its cache tags, saving a feed should invalidate the render cache.

Using drupal_flush_all_caches feels wrong, because effectively we're saying 'if we clear all caches does the correct thing show' which we know is the case. I think we want to instead test that saving a feed invalidates the listing.

Yes, that changed worked.

Looks good to me - thanks!

I'll cut a new release

This can apply to the 1.x branch too. I just haven't fixed its tests yet. We could apply the patch without fixing the tests first. I ran them locally without and with the fix and got the same results that I did for the 2.x branch. But I was trying to do things properly.

Yeah fine to backport we know tests on 1.x are in a bad place

It passed for the 1.x branch too.

This has been committed to contrib Aggregator. The patch in #14 can be rerolled for core, if anyone cares to do it.

Please review.

Verified on Drupal 9.5. Thank you!

9.5 has perhaps seen its last bug fix release already, but for posterity let's get this in anyway.

Committed and pushed 3892aea923 to 9.5.x. Thanks!

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