RSS Feed header reverts to text/html when cached

Possibly this fix should be used for all display plugins, or added in a different layer of the API? See #1312962: Cache HTTP headers sent by view result/output.

After applying the patch there is no Content-Type header set in the response headers.

I ran into this same issue with a cached RSS feed. The patch proposed in #2 does not work. The issue with the patch is that it is assuming $build['#content_type'] exists, which is does not. This results in always nullifying the Content-Type header.

Cause for this bug:
The template_preprocess_views_view_rss() is the only place where the response header "Content-Type" is set to 'application/rss+xml; charset=utf-8'. This works when the theme "views_view_rss" is not cached. Once it is render cached, then the response header "Content-Type" is never set.

The MR posted in #7 provides a way for the style plugins (RSS, OPML) to alter the response object built by the display plugin (Feed). This sets the correct response Content-Type header even when the style's theme is render cached.

Cleanup / Future considerations:
The template_preprocess_views_view_rss() and template_preprocess_views_view_opml() no longer need to set the response headers. This was bad practice to begin which has led to this issue. A theme that can be cached should not set the HTTP response headers. It will eventually cause the issue found here.

Passed tests and validation, so attached a static patch for MR 2063 at commit f49f4274. This can be used for builds that want to use the patch as of now while not receiving future updates to the MR that could break their builds.

Hello,

I applied #9 patch but it display the RSS feeds in HTML format not in XML format with proper structure so

I created MR !2619 for it.
Also, fixed below coding standard warnings.

FILE: /home/binoli/Sites/drupal/core/modules/views/src/Plugin/views/style/Rss.php
---------------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
---------------------------------------------------------------------------------
  29 | ERROR | [x] Missing function doc comment
  55 | ERROR | [x] Missing function doc comment
  63 | ERROR | [x] Missing function doc comment
  78 | ERROR | [ ] Return type missing for @return tag in function comment
 100 | ERROR | [x] Missing function doc comment
---------------------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------

FILE: /home/binoli/Sites/drupal/core/modules/views/src/Plugin/views/display/ResponseDisplayPluginTrait.php
----------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------------------------------------------
 20 | ERROR | Missing parameter type
 22 | ERROR | Missing parameter type
----------------------------------------------------------------------------------------------------------

Please review MR.

Thanks

@Binoli Lalani please do not turn this issue into a code cleanup task. The patch should focus on the bug in this issue only.

@Binoli Lalani - Your new "branch 3272985-change-views-rss-feeds-format" steals the work done in my branch "3272985-views-rss-feed-response-content-type" and claims it as its own. If you want to contribute to make a better patch, it's best practice to branch off of the original branch "3272985-views-rss-feed-response-content-type" and then make your changes. This keeps the commit authors for credit and makes it easier to see what you are changing.

Hello @recrit,

I've not stolen your work. I applied your patch but it seems that feeds are showing in HTML format, I wanted to show in XML format with proper structure same as an attached screenshot so I just extended your patch and change only below line in my MR.

$response->headers->set('Content-Type', 'application/rss+xml; charset=utf-8');

to

$response->headers->set('Content-Type', 'application/xml; charset=utf-8');.

Thank you for providing the patch. I will take care of your point to branch off of the original branch.

Thanks!

@Binoli Lalani The problem is that you did not extend my patch. You applied the patch to a new branch and then committed it is as your own.
Your branch: 3272985-change-views-rss-feeds-format - All commits are authored by you.

To contribute to the patch, the following makes it easier to see exactly what you are changing and it keeps the authors of the original commits.

git clone git@git.drupal.org:issue/drupal-3272985.git
# Checkout the original branch: 
git checkout 3272985-views-rss-feed-response-content-type
# Make your changes to the files
# Commit the changes to the issue branch

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

created a new issue branch "327985-11.x" for 11.x with patch #9 rolled on 11.x. Let's use this for development and then only post static patches based on the MR 4756.

Attached is a static patch of Drupal 11.x MR 4756 at commit c952438a.

Work Pending: Add automated tests for RSS header content-type.

Attached is the static patch for 10.1.x MR 4789.

Pending: Automated tests

Added Step 0 to uninstall page cache modules that could be caching the HTTP response headers.
Added expected HTTP header.
Added new PHP interface.

Attached is a patch for 11.x that adds only the new test. This patch should fail which proves that a fix to core is needed.

FYI - "PHP 8.1 & MySQL 5.7 Not currently mergeable." is for some reason using the closed merge request 4788 without anyway to change it to the correct MR 4789. For that reason, I was unable to run the 10.1 test with PHP 8.1 & MySQL 5.7

Summary up until now:

• 11.x: MR 4756
  • Tests: PASS at commit 8509a53c91
  • Verify that D11.x fails without this fix: VERIFIED - See "tests only" patch on #35
  • Static patch at 8509a53c91: Attached to this post - "3272985-D11.x-MR4756-8509a53c91--20230928.diff "
• 10.1.x: MR 4789
  • Tests: PASS at f56a2633c6
  • Static patch at f56a2633c6: Attached to this post - "
    3272985-D10.1.x-MR4789-f56a2633c6--20230928.diff"

In Slack, #gitlab, smustgrave asked for all MRs except 4756 and 4789 were closed. I have closed MR !2619, MR !2063 and MR !2042,

Cheers

Thanks @quietone!

Tested on a fresh installed of 11.x with standard profile
I used the rss view that is installed
Confirmed the issue following the steps in the IS and that MR 4756 solved the issue.

For the new trait believe we will need a change record

Good example would be https://www.drupal.org/node/3392235

@smustgrave - Draft CR created at https://www.drupal.org/node/3402518

CR reads fine.

I'm pretty sure this can be implemented using #attached, see comment on the MR. This would mean that views.theme.inc wouldn't need to do anything then either, and would save the API addition.

@catch this will not work in any preprocess function once it has been rendered cached. It will work the first time, but any request to the page after that will break. See comment #6 and #8 for more details.

#6 and #8 don't mention using #attached, they just talk about the preprocess function, which is also not using #attached (hence why it doesn't work properly).

#attached does work with the cache system and I think it could be implemented in the style plugin directly rather than preprocess.

@catch thanks .. When I first read your comment, I thought that you meant the preprocess function. I'll try using the #attached approach in Drupal\views\Plugin\views\style\Opml::render() and Drupal\views\Plugin\views\style\Rss::render() to see if the test passes.

@catch Adding the "['#attached']['http_header']" to \Drupal\views\Plugin\views\style\Opml::render() and \Drupal\views\Plugin\views\style\Rss::render() does not work since the Feed display plugin uses returns_response = TRUE which get its own custom response processing. The views HTTPStatusCode area plugin get's added to page displays, so the displays build area gets processed in a way that the "#attached" is propagated to the response in Drupal\Core\Render\HtmlResponseAttachmentsProcessor.

Feed Display Response processing:
The Feed display plugin is the only core Views display plugin that uses the returns_response = TRUE. When the "returns_response" plugin property is set, the \Drupal\views\Routing\ViewPageController::handle() uses the response as built by the display plugin with its buildResponse. The Feed::buildResponse() renders the $build array with a RendererInterface::renderRoot(), adds only the cacheable metadata with CacheableMetadata::createFromRenderArray(), and then returns the \Drupal\Core\Cache\CacheableResponse instead of a build array. With that processing, the attachments do not get added to the response as expected.
The "['#attached']['http_header']" approach could theoretically work if we added the code to process "['#attached']['http_header']" in Feed::buildResponse() similar to how Drupal\Core\Render\HtmlResponseAttachmentsProcessor processes it.

To test this approach, I created the "3272985-attached-approach-10.1.x-DO_NOT_USE-51.patch" for 10.1.x in order to show that it fails the new automated tests added in the MR. The same issue exists for 11.x too.
Test Result: Failed for Drupal\Tests\views\Functional\Plugin\DisplayFeedTest::testFeedCacheability as expected.

For reference \Drupal\views\Routing\ViewPageController::handle():

    ...
    $class = $route->getOption('_view_display_plugin_class');
    if ($route->getOption('returns_response')) {
      /** @var \Drupal\views\Plugin\views\display\ResponseDisplayPluginInterface $class */
      return $class::buildResponse($view_id, $display_id, $args);
    }
    else {
      /** @var \Drupal\views\Plugin\views\display\Page $class */
      $build = $class::buildBasicRenderable($view_id, $display_id, $args, $route);
      Page::setPageRenderArray($build);

      views_add_contextual_links($build, 'page', $display_id, $build);

      return $build;
    }

The "['#attached']['http_header']" approach could theoretically work if we added the code to process "['#attached']['http_header']" in Feed::buildResponse() similar to how Drupal\Core\Render\HtmlResponseAttachmentsProcessor processes it.

Thanks for trying it out. This approach sounds encouraging.

Seems the feedback has been addressed?

@smustgrave - no it hasn't, there haven't been any commits on the MR, only discussion.

@catch I do not have any commits unless you are looking for the approach in #54 to be used instead of the current implementation.

@recrit yes I think we should try the approach in #54 - i.e. make things work with the existing API rather than adding a new one.

@catch I updated the MRs to use #attached['http_header'] instead of a custom trait.

10.1.x MR 4789 (this MR)

11.x MR 4756

That looks much better to me, and great to get rid of that logic from preprocess.

Last question I have is does this still work for the live preview situation?

@catch Yes, it worked in my testing. Now, the HTTP headers are only added when the route is visited via the route callback \Drupal\views\Routing\ViewPageController::handle() instead of a theme function as it was before.

Removed the Data model change from the IS as that no longer seems to be true.

Current CR is outdated and can be deleted.

Not sure if we need one for announcing headers being added but going to assume no.

Appears feedback has been addressed though and it's clear in the IS which MR to be committed.

Asked for an addition to the comment in the MR.

Agreed this doesn't need a change record, just a bugfix now.

Updated both MRs with a comment to reference \Drupal\Core\Render\HtmlResponseAttachmentsProcessor::setHeaders()

Thanks! Moving back to RTBC.

Nearly went to commit this then spotted a couple more small issues, feel free to re-RTBC once these are resolved.

@catch resolved

Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!

thanks! Attached is the final patch for 10.1.x for anyone still on that version.