Shift responsibility for keeping Date header to ResponseHeaderBag #22124
Conversation
|
I would do that in master as there is no bug fix here. |
|
@fabpot Fine for me. Can you do this when merging, or do you need a new PR? |
|
You can change the target branch on the PR directly. |
|
Must be a new GH feature or it has always been so well hidden I missed it. Changed. |
| ), | ||
| array( | ||
| array('ETag' => 'xyzzy'), | ||
| array('ETag' => array('xyzzy'), 'Cache-Control' => array('private, must-revalidate')), |
There was a problem hiding this comment.
Im not sure you can simply remove the cache-control expectations, perhaps re-add it in a separate test. Which is probably best in the long run.
There was a problem hiding this comment.
If I am not mistaken, there are other tests for them. In this test method that was only noise AFAICT, but please correct me if I'm wrong
There was a problem hiding this comment.
You're right 👍 same situation is covered in testCacheControlHeader
|
If you could also change the target to 3.4, that would be wonderful. |
|
@fabpot changed base to 3.4 – did I get you right? Also, at least in the web interface, GitHub shows no merge conflicts...? |
|
3.4 👍 |
|
Hm, rebasing and fixing up a branch that I had previously rebased in the GH web interface, including merge conflicts, really challenged my git skills... hope it's allright now. |
|
Apparently, tests are broken on deps low. |
The headers returned now also include the "Date", but adding it to the expected outcome would add additional noise. So instead I chose to test that only the given headers would remain present, preserving case. The Cache-Control was not important here, we have other tests covering that aspect.
|
Messed up the test during merge conflict resolution, should be fixed now. |
|
Thank you @mpdude. |
…seHeaderBag (mpdude) This PR was squashed before being merged into the 3.4 branch (closes #22124). Discussion ---------- Shift responsibility for keeping Date header to ResponseHeaderBag | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | This is an improvement over #22036. It shifts responsibility for preserving a `Date` header to the `ResponseHeaderBag`. We already have similar logic there for the `Cache-Control` header. Commits ------- 5d83836 Shift responsibility for keeping Date header to ResponseHeaderBag
This is an improvement over #22036. It shifts responsibility for preserving a
Dateheader to theResponseHeaderBag.We already have similar logic there for the
Cache-Controlheader.