Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpCache] purge both http and https from http cache#21582
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| } | ||
| returnfalse; | ||
| if (isset($this->locks[$key2])) { |
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.
the actual deleting could be moved to a private function to avoid code repetition.
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.
Having a private method makes sense I think.
| $uri2 =0 ===strpos($uri,'https://') | ||
| ?preg_replace('https','http',$uri,1) | ||
| :preg_replace('http','https',$uri,1); | ||
| $key2 =$this->getCacheKey(Request::create($url2)); |
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.
uri2
| publicfunctionpurge($url) | ||
| { | ||
| $key =$this->getCacheKey(Request::create($url)); | ||
| $uri2 =0 ===strpos($uri,'https://') |
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.
url
dbu commentedFeb 12, 2017
ups, yep url not uri. but i wait for feedback on the general idea before fixing any details here |
fabpot commentedFeb 12, 2017
fabbot reports errors that should be fixed. |
e2cad29 to00420e2Compare| publicfunctionpurge($url) | ||
| { | ||
| $key =$this->getCacheKey(Request::create($url)); | ||
| $http =preg_replace('#^https#','http',$url); |
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.
if $url is already http://, nothing changes.
this was previously
$url2 =0 ===strpos($url,'https://') ?preg_replace('#^https#','http',$url,1) :preg_replace('#^http#','https',$url,1);return$this->doPurge($url) ||$this->doPurge($url2);
but i find the new code more readable. we add one preg_replace instead of a strpos. given that we do multiple filesystem operations, the speed difference should not matter.
dbu commentedFeb 13, 2017
updated. should this go into master or is it a bugfix? |
fabpot commentedFeb 16, 2017
I think this is a bug fix. |
| /** | ||
| * Purges data for the given URL. | ||
| * | ||
| * This method purges both the http and the https version of the cache entry. |
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.
HTTP in caps?
| * @param string $url A URL | ||
| * | ||
| * @return bool true if the URL exists and has been purged, false otherwise | ||
| * @return bool true if the URL existswith either http or https schemeand has been purged, false otherwise |
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.
Same here?
dbu commentedFeb 18, 2017
fixed the spelling. should i close this and open a new pull request against the 2.7 branch? or can the mergers handle that? |
fabpot commentedFeb 18, 2017
Thank you@dbu. |
This PR was submitted for the master branch but it was merged into the 2.7 branch instead (closes#21582).Discussion----------[HttpCache] purge both http and https from http cache| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | => travis| Fixed tickets | -| License | MIT| Doc PR | -The HTTP cache store of HttpCache respects the scheme for cache entries, which is the expected behaviour. however, Store::purge($url) also respects the scheme when invalidating the cache. This seems wrong to me.This PR is rather rough for now, and i did not even look at the tests. Do the maintainers agree with my assumption? Should it be a bugfix against 2.8? Any input on the code?Commits-------15da53c purge both http and https from http cache store
| $http =preg_replace('#^https#','http',$url); | ||
| $https =preg_replace('#^http#','https',$url); | ||
| return$this->doPurge($http) ||$this->doPurge($https); |
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.
Based on the title of this PR, I would have expected this to be an&& instead of an||
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.
my thinking was that when either http or https was purged we report that something was purged. the chances thatboth http and https were in cache are small.
i think the most sane use case for this is when doing cache invalidation, e.g. with FOSHttpCache: you need to send a request to the specific host by ip, rather than the domain name, to target each server instance. but if you do that over https, you get certificate mismatches and thus want to use http.
apart from such special cases, your webserver should probably redirect requests from http to https rather than allow both.
This PR was squashed before being merged into the 2.7 branch (closes#22079).Discussion----------[HttpKernel] Fixed bug with purging of HTTPS URLs| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |I found two bugs in `HttpCache\Store::purge()` with HTTPS URLs:1. `->purge('https://example.com/')` only purges the `http` version not the `https` one.2. If a cache entry exists for both `http` and `https`, only the `http` version gets purged, the `https` version stays in the cache.I think this issues were introduced with#21582.This pull request fixes both issues and adds tests for them.Commits-------f509150 [HttpKernel] Fixed bug with purging of HTTPS URLs
The HTTP cache store of HttpCache respects the scheme for cache entries, which is the expected behaviour. however, Store::purge($url) also respects the scheme when invalidating the cache. This seems wrong to me.
This PR is rather rough for now, and i did not even look at the tests. Do the maintainers agree with my assumption? Should it be a bugfix against 2.8? Any input on the code?