8000 [HttpCache] fixed if-modified-since header default to an empty string if the last-modified header was not set by akalineskou · Pull Request #34016 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpCache] fixed if-modified-since header default to an empty string if the last-modified header was not set #34016

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[HttpCache] fixed if-modified-since header default to an empty string if the last-modified header was not set #34016

wants to merge 1 commit into from

Conversation

akalineskou
Copy link
Contributor

… if the last-modified header was not set

The problem I had with this was that the psr http factory would create the request, and would throw an exception because the null value isn't an allowed header value
https://github.com/symfony/psr-http-message-bridge/blob/8564bf76630423ced21bbbee189947b90677dcde/Factory/PsrHttpFactory.php#L72
https://github.com/Nyholm/psr7/blob/55ff6b76573f5b242554c9775792bd59fb52e11c/src/MessageTrait.php#L180

Q A
Branch? 3.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

@akalineskou akalineskou force-pushed the fix-http-cache-validate-last-modified-value branch from 020aae3 to 13b34a7 Compare October 17, 2019 17:26
@nicolas-grekas nicolas-grekas changed the title [HttpCache] fixed if-modified-since header default to an empty string… [HttpCache] fixed if-modified-since header default to an empty string if the last-modified header was not set Oct 18, 2019
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Oct 18, 2019
nicolas-grekas
nicolas-grekas reviewed Oct 18, 2019
View reviewed changes
@nicolas-grekas
Copy link
Member

Could you please add a test case?

@akalineskou
Copy link
Contributor Author

Could you please add a test case?

I thought about adding a test for, but I don't how I would go about testing this particular case

@stof
Copy link
Member
stof commented Oct 18, 2019

shouldn't we actually omit the if-modified-since header in case there is not last-modified header instead of setting an empty string ?

@akalineskou
Copy link
Contributor Author

I've created this repo from scratch to reproduce it
https://github.com/alex2005git/symfony-cache-bug

start the server, open the route, after 1 minute or 2 it should throw Header values must be RFC 7230 compatible strings.

@akalineskou akalineskou force-pushed the fix-http-cache-validate-last-modified-value branch 2 times, most recently from e30f93a to d907bb0 Compare October 21, 2019 19:34
@akalineskou akalineskou force-pushed the fix-http-cache-validate-last-modified-value branch from d907bb0 to c64e5cb Compare October 21, 2019 19:35
@akalineskou
Copy link
Contributor Author

shouldn't we actually omit the if-modified-since header in case there is not last-modified header instead of setting an empty string ?

I'm fine with whichever you guys think is best

@akalineskou
Copy link
Contributor Author

@nicolas-grekas any update on this issue?

@akalineskou
Copy link
Contributor Author

ping!

@nicolas-grekas
Copy link
Member

Omitting the header looks the best yes in this case. With a test case also :)

@nicolas-grekas
Copy link
Member

pong @alex2005git :)

@mpdude
Copy link
Contributor
mpdude commented Jan 8, 2020

This has been fixed in #34385 already.

@akalineskou akalineskou deleted the fix-http-cache-validate-last-modified-value branch February 4, 2022 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

Assignees
No one assigned
Labels
Bug Status: Needs Review
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
5 participants
@akalineskou @nicolas-grekas @stof @mpdude @carsonbot
0