-
Notifications
You must be signed in to change notification settings - Fork 76
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
POST request strategy key support #171
Conversation
@Kevinrob any feedback? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As explained in the issue linked, the cache should not intercept POST queries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As body should be ignored for GET requests, I don't think it's okay to add this to the library.
You can keep a custom strategy on your codebase or maybe add a way to the library that let customise this on-demand.
PS: for the Docker related modifications, would you like to make a different PR with it?
Sure I will created a separate SR with only docker commit. |
Here: #179 |
It is strange that you have a way to set the cache methods https://github.com/Kevinrob/guzzle-cache-middleware/blob/0e12dccf3c811a18bb2f6c93882c5b5727e1b859/src/CacheMiddleware.php#LL98C21-L98C35 If you think the library should not allow to set cache for post method and others why not drop this method to prevent users from misusing it? |
Yeah, that's because this lib can be extended with custom strategies. The strategies that are bundle with it respect the RFC, but people can use it as they want. |
I am not longer interested in this change. |
Will solve: #170