Conversation
src/CorsService.php
Outdated
| if (!in_array($header, $existing)) { | ||
| if (count($varyHeaders) < 2) { | ||
| $response->headers->set('Vary', $response->headers->get('Vary') . ', ' . $header); | ||
| } else { | ||
| $response->headers->set('Vary', $header, false); | ||
| } | ||
| } |
There was a problem hiding this comment.
This does seem correct. But it is a bit hard to grok and the count comparison reliance on being in the else seems prone to refactoring bugs.
There was a problem hiding this comment.
It doesn't seem right to pass a manually comma separated string into ::set. That method performs its own internal "is it a string or an array" checking. If attempting to set multiple values, use an array. If setting a single value, use a string?
The Symfony documentation doesn't say anything about the correctness of passing a manually crafted string here. If it happens to work, my estimation is that it works by happy accident.
There was a problem hiding this comment.
I don't Symfony does any string parsing/crafting. You can set an array, and then merges/replaces the arrays, but it doesn't automatically combine them as far as I can tell. Settings an array just results in multiple headers with the same name (which is valid). But getting them back from the HeaderBag just returns the first one (unparsed)
There was a problem hiding this comment.
See also the code from Symfony itself, when handling multiple values for 1 header:
Often using the same implode style
There was a problem hiding this comment.
Hmm looking at the code for the $response->getVary() en $response->setVary() it does seem like maybe we could set the Vary header, but I would rather not change existing behavior in a small release. I did refactor it a bit now.
There was a problem hiding this comment.
It doesn't seem right to pass a manually comma separated string into ::set. That method performs its own internal "is it a string or an array" checking. If attempting to set multiple values, use an array. If setting a single value, use a string?
Yeah sort of. But it's allowing for multiple copies of headers not a header with a list of values which is somewhat different. Each entry in the array is sent as a different header.
The current method is actually strictly follows the RFC of keeping vary in a single CSV value.
A Vary field value consisting of a comma-separated list of names
indicates that the named request header fields...
The new code provides for sending multiple headers but doesn't force it. Changing it entirely rely on the array handling would force it to send multiple which might be unexpected or as barry said it could be an unwelcome surprise in a bug fix that could break code relying on it.
It might even be undesirable. I did i quick at a cursory review all reverse proxies and CDNs seem fine merging up multiple vary headers and that makes sense but I didn't dig too deep.
3 participants
Resolves #105
Replaced #107
When multiple Vary headers are set, Symfony only returns the first one. When replacing the Vary header, it removes the 2nd one. This changes behavior so that in the case multiple headers are used, it check all existing Vary headers and appends it as a new header. This will result in an additional Vary header but only when there were already 2 or more.