Duplicate HTTP headers should be removed

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

This could use steps to reproduce. I don't notice this on 404s pages or regular pages.

added patch against 10.1.x

#36 applied to 10.1 fine.
Please include an interdiff with files also. Hiding #40

Re #39, I was able to reproduce this issue in 11.x by adding

--- a/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -118,6 +118,9 @@ public function onRespond(ResponseEvent $event) {
     // Set the Content-language header.
     $response->headers->set('Content-language', $this->languageManager->getCurrentLanguage()->getId());
 
+    $response->headers->set('foo', 'bar', FALSE);
+    $response->headers->set('foo', 'bar', FALSE);

in core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php.
This will show 2 entry in header response in network tab.
But I am not sure if this is the correct way to reproduce this issue. Once confirmed IS can be updated.

I will make a PR of the provided patch so that review can be done easily.

Seems like a simple enough change.

Test does fail without the fix

Duplicate HTTP link headers were removed.
Failed asserting that true is false.

Left a comment on the MR.

Assert was added per the thread.

Going to remark it.

I am doing triage on the core RTBC queue → .

I read the issue summary which has a proposed resolution, so reviewers know what they are looking for. I then read the comments. I found everything answered.

I applied the patch and confirmed it fails without the fix. I did not review the MR.

Leaving at RTBC

This Stack Overflow answer implies that duplicate headers are allowed, at least in some cases: https://stackoverflow.com/questions/4371328/are-duplicate-http-response-...

The example given is that

Cache-Control: no-cache, no-store

is equivalent to

Cache-Control: no-cache
Cache-Control: no-store

So are we doing the right thing here?

Also this code seems to deduplicate by value, but without taking the key into account? I haven't tested this but it appears that if you generate two headers with the same value but different keys, they might be treated as duplicates?

#52 did not test but think that would be a good test to include.

@longwave

I haven't tested this but it appears that if you generate two headers with the same value but different keys, they might be treated as duplicates?

Correct, But I am not sure how to reproduce above scenario. Can you please provide some code reference, so that I can try to reproduce and fix it?

If the issue is only with Link headers, perhaps we should only be deduplicating these.

Done

@smustgrave,

#52 did not test but think that would be a good test to include.

Test is already added for duplicate links. Please elaborate what needs to be done here. Thanks.

two headers with the same value but different keys,

Actually think that is covered so nvm

Setting to needs work for unresolved comments on the MR,

Because of @longwave's comment I have added a link to the latest RFC to the Issue Summary.

Appears all threads have been addressed.

I think the MR looks good, only Link headers are considered which was the source of the bug. There is good test coverage. Duplicate link header values are removed. I agree this is good to go.

Hiding all the patches as it is confusing.

I've added some comments on the MR that needs addressing.

There was 1 failure:
1) Drupal\KernelTests\Core\Render\RenderTest::testDuplicateLinkHeaders
Failed asserting that actual size 2 matches expected size 1.
/builds/issue/drupal-2861552/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/builds/issue/drupal-2861552/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/builds/issue/drupal-2861552/core/tests/Drupal/KernelTests/Core/Render/RenderTest.php:92
/builds/issue/drupal-2861552/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 4, Assertions: 4, Failures: 1.

Re ran test-only feature to make sure test coverage was still good with the latest changes.

See all threads have been addressed, believe this is still good.

I have reviewed the MR and left one comment which is probably still not resolved from previous threads.

And also one additional minor comment.

Otherwise I think this looks good!

My points seems to be resolved, so moving back to RTBC. Thanks.

Reviewing this again and I now see that the current solution is different from the one in #18 🐛 Duplicate HTTP headers should be removed RTBC . That solution changes \Drupal\Core\Render\HtmlResponseAttachmentsProcessor::processHtmlHeadLink to prevent the additional of duplicates in the first place. I tested this version with the version of the test in the latest MR and it does work. The current solution started in #30 🐛 Duplicate HTTP headers should be removed RTBC and removes duplicate headers. Unfortunately, there has not been a discussion of which approach is preferred.

So, I am setting this back to Needs Review to decide which is the preferred solution. Sorry that I missed this point in my previous visits to this issue.

Tagging for issue summary to be updated to clear things up.

I am not sure if this still requires IS update as Remaining tasks already mention about decision to be made related to the approach.

Sorry meant can it be clearly noted in the issue summary the pros/cons of each approach.

Can go in the proposed solution as "a)" and "b)"

tagging for submaintainer for make the call though. Unless you know someone else that can and we can ping them.