[HttpCache] purge both http and https from http cache #21582
Conversation
| } | ||
|
|
||
| return false; | ||
| if (isset($this->locks[$key2])) { |
There was a problem hiding this comment.
the actual deleting could be moved to a private function to avoid code repetition.
There was a problem hiding this comment.
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)); |
| @@ -324,20 +324,37 @@ private function getMetadata($key) | |||
| public function purge($url) | |||
| { | |||
| $key = $this->getCacheKey(Request::create($url)); | |||
| $uri2 = 0 === strpos($uri, 'https://') | |||
|
ups, yep url not uri. but i wait for feedback on the general idea before fixing any details here |
|
fabbot reports errors that should be fixed. |
e2cad29 to
00420e2
Compare
| */ | ||
| public function purge($url) | ||
| { | ||
| $key = $this->getCacheKey(Request::create($url)); | ||
| $http = preg_replace('#^https#', 'http', $url); |
There was a problem hiding this comment.
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.
|
updated. should this go into master or is it a bugfix? |
|
I think this is a bug fix. |
| @@ -317,14 +317,30 @@ private function getMetadata($key) | |||
| /** | |||
| * Purges data for the given URL. | |||
| * | |||
| * This method purges both the http and the https version of the cache entry. | |||
| * @param string $url A URL | ||
| * | ||
| * @return bool true if the URL exists and has been purged, false otherwise | ||
| * @return bool true if the URL exists with either http or https scheme and has been purged, false otherwise |
|
fixed the spelling. should i close this and open a new pull request against the 2.7 branch? or can the mergers handle that? |
|
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
|
|
||
| return $this->doPurge($http) || $this->doPurge($https); |
There was a problem hiding this comment.
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.
my thinking was that when either http or https was purged we report that something was purged. the chances that both 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?