RSS Feed header reverts to text/html when cached

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 MR 4756 at commit c952438a.

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

recrit → closed merge request !4788

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 🐛 RSS Feed header reverts to text/html when cached Fixed
  • 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"

quietone → closed merge request !2042

quietone → closed merge request !2063

quietone → closed merge request !2619

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 🐛 RSS Feed header reverts to text/html when cached Fixed 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.

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 🐛 RSS Feed header reverts to text/html when cached Fixed 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.

Hooray!

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