If an aggregator page without feed items is requested - e.g. the feed only has a few pages worth of feed items but aggregator?page=10000 is requested - the empty page will be cached by the dynamic page cache without a cache context set. Later requests for aggregator will incorrectly return this empty page.
Pager cache context should be set in all cases, even if there are no items being displayed.
Review patch.
None.
None.
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 3273873.patch | 1.27 KB | mfb |
| TEST-ONLY-WILL-FAIL.patch | 736 bytes | mfb |
Comments
Comment #2
mfbComment #3
larowlanThanks, we're waiting on help to get tests running on d.o, for some reason the composer facade isn't picking up our dependencies
#3272246: Ensure that aggregator doesn't get special core treatment
Comment #4
mfbOk :) BTW, should I also file this as a bug report + patch for Drupal 9, i.e. are aggregator bugs still being fixed there?
Comment #5
mfbWent ahead and filed a bug report on core as well: #3273876: Aggregator page contents could be empty due to missing cache context
Comment #6
larowlanI think its more likely to be fixed here, as soon as its installable in D9, I think folks should be able to move
Comment #7
mfbYes core issues seem to be resolved slowly.... well at least I was able to validate the test and patch over there.
Comment #8
mfbAnyone interested in a reviewing this patch? It's a pretty simple fix for a fairly critical bug (at least for sites that rely on Aggregator module).
Comment #9
larowlanPatch looks good to me, I'll look to commit in the coming fortnight
Comment #12
larowlanWell that was a long fortnight 😆
Fixed thanks, I'll cut a release if the 2.x branch passes tests
Comment #13
larowlanhttps://www.drupal.org/project/aggregator/releases/1.0.2
https://www.drupal.org/project/aggregator/releases/2.0.1
Comment #14
mfb🎉