Relative URLs in cached feeds should be converted to absolute ones

The response object is deliberately passed down from the Feed display plugin through to the theme layer, to allow the HTTP header to be set. Cache it, so that the headers can then be used for subsequent requests.

Here's a patch to implement this. It will need tests adding, though perhaps we can just add to those written for #88183: Relative URLs in feeds should be converted to absolute ones, to use a render cache and view the feed twice?

I've added steps to reproduce, to the original issue description. I had some thoughts about possible alternative solutions too, in case anyone is interested:

As an alternative, we could remove the current code that sets the header from template_preprocess_views_view_rss(), and add it with a $build['#attached']['http_header'] in \Drupal\views\Plugin\views\style\Rss::render(), but then that still needs processing back up in \Drupal\views\Plugin\views\display\Feed::buildResponse() - possibly requiring a refactor of \Drupal\Core\Render\HtmlResponseAttachmentsProcessor::setHeaders() to avoid duplicating code. I could understand if that is preferred to caching the whole response object.

Another idea I had was to allow arrays to be set as values within the #cache_properties array, to allow caching just a part of a certain property - so then just the headers from $build['#response']->headers could be cached, rather than the whole object. But that would be a bigger API change. I believe the approach in comment 2's patch is the simplest instead, and that caching the whole response object isn't really a problem.

For reference, here's what gets serialized into a cache using a database backend for the response:

s:9:"#response";O:35:"Drupal\Core\Cache\CacheableResponse":7:{s:7:"headers";O:50:"Symfony\Component\HttpFoundation\ResponseHeaderBag":5:{s:23:"*computedCacheControl";a:2:{s:8:"no-cache";b:1;s:7:"private";b:1;}s:10:"*cookies";a:0:{}s:14:"*headerNames";a:3:{s:13:"cache-control";s:13:"Cache-Control";s:4:"date";s:4:"Date";s:12:"content-type";s:12:"Content-Type";}s:10:"*headers";a:3:{s:13:"cache-control";a:1:{i:0;s:17:"no-cache, private";}s:4:"date";a:1:{i:0;s:29:"Fri, 06 Apr 2018 11:44:29 GMT";}s:12:"content-type";a:1:{i:0;s:34:"application/rss+xml; charset=utf-8";}}s:15:"*cacheControl";a:0:{}}s:10:"*content";s:0:"";s:10:"*version";s:3:"1.0";s:13:"*statusCode";i:200;s:13:"*statusText";s:2:"OK";s:10:"*charset";N;s:23:"*cacheabilityMetadata";N;}

I don't believe that's a disastrous additional amount to add to the render cache for feeds, but it's enough to have made me think about alternatives.

Let's put the patch through the existing tests at least...

Here's a really naive attempt at a test, let's see if it fails without the fix...

Hmm, either my test is wrong or the original 'bug' was actually due to something else in my environment...

Here's a working test that should prove the bug - patches with & without the fix included.

For the previous test (comment 4), I hadn't realised that the testing profile enables the page_cache & dynamic_page_cache modules, which actually get around the bug, because the incorrect content is only cached at the render cache layer (the page cache layer is actually correct). So the bug is only present when the page cache module(s) are not installed. So I've created a new test method, rather than dumbly extending the existing one, that will uninstall the dynamic_page_cache module (and rebuild the container etc so that its event subscribers are no longer listening), then I've copied the minimum amount from the existing test that will prove the bug. I hope that's correct - I don't know whether uninstalling modules in this way within a test class is correct or not! (e.g. should the test clear up after itself by re-enabling the dynamic_page_cache module?)

Good idea. Patches updated :-)

That was the expected result :-) The tests-only patch demonstrates the bug, the other patch fixes it.

Nitpicks addressed :-)

That was the expected result :-)