Skip to content
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.

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

[6.x] Fix last_modified option in SetCacheHeader #30335

Merged
merged 4 commits into from
Oct 21, 2019
Merged

Conversation

aponscat
Copy link
Contributor

Fix problem with last_modified option

The SetCacheHeaders Middleware accept a parameter last_modified, the problem is that the middleware passes only strings as parameters and the setCache needs a DateTime compatible object.

This fix parses the string passed in the last_modified option in two ways:

  1. if the parameter is a number, a unix timestamp is assumed, parsed with Carbon and replaced in the options array
  2. if the parameter is a string, it's send to the Carbon parse function in order to create a date and then is replaced in the options array

Two new tests created

  1. testLastModifiedUnixTime
  • Gets the current unix timestamp, passes to the last_modified option of the CacheHeaders Middleware and checks that the response contains the Last-Modified header with that exact date.
  1. testLastModifiedStringDate
  • Passes my birthdate to the last_modified option of the CacheHeaders Middleware and checks that the response contains the Last-Modified header with that exact date.

@driesvints driesvints changed the title fix last_modified option in SetCacheHeader [6.x] Fix last_modified option in SetCacheHeader Oct 18, 2019
@driesvints
Copy link
Member

@aponscat
Copy link
Contributor Author

Sent a new commit

@aponscat
Copy link
Contributor Author

Hope that now it's ok.

Sorry for the first low quality pull requests.

And thanks again to you and Taylor for your great job!

@taylorotwell taylorotwell merged commit 737427f into laravel:6.x Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants