Node RSS Views plugin causes wrong entity_view output to be cached

Fix the cache context because now some of the theme will render out author user context.

Patch #7 looks good. Marking RTBC.

If this is supposed to bypass the node template and now doesn't, won't that cause other problems?

looks good. Marking RTBC.

Testbot had a hiccup re-rtbcing

This seems pretty important to have a test for.

I am experiencing a similar problem. The patch #11 resolves the caching issue. Using Drupal 8.5.6.

Reroll for 8.7.x

NW for tests

I am at Seattle Sprint and working on this issue.

Followed steps to reproduce with caching for Page and Feed set to Tag-based (for render caching). Not reproducible with Drupal core 8.7.0-dev environment from Community Tools Download (https://www.drupal.org/tools) using Devel 8.x-2.0 Generate to populate nodes.

Rerolled with code from patch 2885098-18.patch including test cases from drupal-2885098-11-node-views-rss-no-cache.patch.

Triggering bots.

Re-roll patch created for 9.1.x

Please review.

Thanks for the patch. I moved this back to Needs work. I think at this point it makes sense to work on a test that will demonstrate the bug. I think this is still a Novice issue.

Can't reproduce the bug. Tried the following:
Test Case1:
1) Created view as content with type: article [already have 1 node created as article]
2) the page view with Format:Unformatted list | Settings and Show:Content | Full content. Caching: tag based, Path:/global2020
3) Added new view as feed with Format:Unformatted list | Settings and Show:Content | Full content. Caching: tag based, Path:/feed/global2020
Views output

Test Case2:
1) Created view as content with type: article [already have 1 node created as article]
2) the page view with Format:Unformatted list | Settings and Show:Content | RSS. Caching: tag based, Path:/global2020
3) Added new view as feed with Format:Unformatted list | Settings and Show:Content | RSS. Caching: tag based, Path:/feed/global2020
Output:
Both are showing expected results, page view showing title and RSS showing popup to download file.

Process:
1) Visited RSS page first then page
2) Visited page first then RSS page
3) Also tried with different formats still can't get the cached page.

Thank you, @bakulahluwalia. Being able to triage issues is really important too.

I confirm that I tried to reproduce it following the steps and wasn't able to reproduce it.

I changed Caching to something other than None in Administration > Configuration > Development > Performance, and after creating the view, I cleared cache, visited the Feed, noted the fields, visited the page, and I saw the content rendered and the markup was based on the node template.

I think if this issue still exists, we would need to update the Steps to reproduce and write a failing test first.

> I changed Caching to something other than None in Administration > Configuration > Development > Performance

The _only_ affect this setting has is the Cache-Control HTTP header.

Based on the code, this is about the render cache. I didn't try to reproduce yet, but the fix makes sense to me and I can see that this could happen.

Maybe try with a different view mode, there's special special handling around default, although full should work.

Hi,

I followed the steps as described in the issue but could not re-produce the results.

Is there anything we need this issue to get moving forward ?

Hi @gapple,

Sorry I think you're right. I was doing the wrong cache clearing sequence.

I am just uploading GIF here for anyone to see the issue visually.

@berdir the above screenshot is in Full display mode.

Regards,
Amjad

Thank you for finding the solution to this. It's been driving me mad on my blog for months. I spent hours and hours tracking down where the problem is from but only managed to figure it was something to do with RSS feeds.

Essentially, the problem is that when you render the node in a RSS feed as a teaser view mode the default node template is used and that template is added to the render cache. This means that when you come to render the node as a teaser using the default template the RSS teaser cache is returned, which gives you the wrong template in your theme. I'm sure this problem exists with other view modes and entities.

I had a go at creating a test for this situation and managed to figure it out. The original fix for the Rss.php class is good, but change to the CommentRssTest test didn't make sense to me.

The good news is that I managed to create a test situation that demonstrates the problem and the fact that the fix is correct. Hopefully everything is clear in the attached patch.

1. +++ b/core/modules/node/tests/src/Functional/NodeRSSCacheTest.php
   @@ -0,0 +1,97 @@
   +
   +  /**
   +   * Ensure that the RSS cache of the node contains is rendered using the current theme.
   +   */
   +  public function testNodeRSSCacheContent() {
   

   Descriptions must not be longer than 80 characters. I think the comment is a bit confusing anyway. the comment on the class seems much clearer.

2. +++ b/core/modules/node/tests/src/Functional/NodeRSSCacheTest.php
   @@ -0,0 +1,97 @@
   +    $node = $this->drupalCreateNode([
   +      'type' => 'article',
   +      'promote' => 1,
   +      'title' => 'Article Test Title',      ¶
   +      'body' => [
   

   trailing spaces.

3. +++ b/core/modules/node/tests/src/Functional/NodeRSSCacheTest.php
   @@ -0,0 +1,97 @@
   +
   +    // Check the render caches for the node tester.
   +    $cache = $this->container->get('cache.render');
   +    $cid = 'entity_view:node:' . $node->id() . ':teaser:' . implode(':', \Drupal::service('cache_contexts_manager')->convertTokensToKeys(['languages:' . LanguageInterface::TYPE_INTERFACE, 'theme', 'user.permissions'])->getKeys());
   +    $nodeTeaserCache = $cache->get($cid);
   +
   

   not sure if we really need to check the cache directly as well, kind of seems like an implementation detail. Tests like this tend to break if things are refactored.

You should also upload a test-only patch separately and first, to prove that your test fails without the fix.

Hi Berdir,

Thanks very much for taking the time to review. I agree with all of the points raised. I was wondering about the cache issue as I was writing it. I felt like I needed to check and demonstrate that the cache was correct, but I was also thinking "this _might_ need fixing in the future". I'll need to keep an eye out for that feeling in the future :). Apologies about the code formatting issues, I should have spotted those.

I have a new patch with the changes applied and another patch without the original fix in place to show that the problem exists. Good idea about the second patch without the fix, I hadn't thought of doing that.

Thanks,
Phil

Adding interdiff between #40 and #42.

+RTBC

I can confirm patch #42 fixed our long-standing problem, similar to other people have reported here: #2915636: node.html.twig intermittently being ignored

The steps I followed to reproduce was:

1. Clear the cache.
2. Visit a taxonomy term feed, for example: /taxonomy/term/%/feed that will generate the cache for the RSS feed
3. Visit a page with a view that displays the same content as the taxonomy term feed. I was getting the 'broken' rows displaying at this point.

Thank you very much @philipnorton42! You're a legend :)

It seems like this will just completely remove render caching from RSS pages. Could we change the cache key instead of unsetting it?

Trying this out on https://rachelnorfolk.me to see if it resolves https://twitter.com/rachel_norfolk/status/1389905214359146500.

Of course, an intermittent issue needs a few days to see how it goes…

Thanks for the feedback catch.

I've had another go at doing this and rather than remove the keys I have added an additional key that references the view rss feed. After some testing this approach appears to work fine and the unit tests created work well also.

This seems like a better approach than just removing the cache keys. Ready for a review I think!

Looks like the test got tripped up on a coding standards thing that was introduced since the last coding standards check.

Fixed the issues in that file and re-created the fix.

Noticed an issue in the views.view.test_node_article_feed.yml file were the previous and next characters were being corrupted.

Sorted that out to keep things neat.

+++ b/core/modules/node/src/Plugin/views/row/Rss.php
@@ -110,12 +110,14 @@ public function render($row) {
-

Is this change intentional? If doesn't add value we should remove to reduce the diff.

+++ b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_node_article_feed.yml
@@ -0,0 +1,208 @@
+uuid: 39713110-fd07-4d68-9814-f4b56948b659

We should remove the uuid.

Thanks for the feedback nlisgo, much apprecaited.

I think the removal of the line in the render() method is fine. It was flagged as a coding standards issue though phpcs running locally and although it is technically out of the scope of the changes I was making, I think having this change ok.

Good spot about the uuid in the config file, I have removed that.

I have also changed the name of the class NodeRSSCacheTest to NodeRssCacheTest, which is also a coding standards change. Since this is a new class for this change I think that's fine in this case. It does bloat the interdiff a little bit though.

+++ b/core/modules/node/tests/src/Functional/NodeRssCacheTest.php
@@ -0,0 +1,87 @@
+    ViewTestData::createTestViews(get_class($this), ['node_test_views']);

The standard now seems to be static::class or self::class.

Everything else looks good to me.

Also a good suggestion, I've used static::class as that gets around any late static binding issues with self::class.

Thanks again! o/

Views supports both, and while the pseudo-rendered entity approach in my experience never really works out, just straight out removing a feature far more complex than just fixing this bug. we have to deprecate it, possibly convert existing config to a field based configuration instead, people might need to update their default configuration, ...

Can the issue summary be updated as there 2 proposed solutions. Can we highlight which one was chosen. Will help with the review.

Adds issue summary update tag

If I understand correctly, the remaining concerns have all been addressed:

• catch's question in #49, addressed by philipnorton42 in #54
• nlisgo's question in #59, addressed by philipnorton42 in #61
• nlisgo's question in #62, addressed by philipnorton42 in #63
• gapple's question in #64, addressed by Berdir in #65
• smustgrave's question in #67, which I addressed just now by updating the issue summary

…and the patch still applies, so I'll set this issue back to RTBC.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

I converted the patch to a merge request, as instructed by the above needs-review-queue-bot comment.

Left some minor comments. Fine to self RTBC after addressing.

I just applied suggestions for those two comments as they were minor, restoring status

Comment module has the same issue, opened a follow-up #3418181: Comment RSS Views plugin causes wrong entity_view output to be cached

Committed to 11.x

As this is a bug and there is no risk of disruption, backported to 10.2.x