Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel] Fixed bug with purging of HTTPS URLs#22079
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
fabpot commentedMar 20, 2017
LGTM. |
| $purgedHttp =$this->doPurge($http); | ||
| $purgedHttps =$this->doPurge($https); | ||
| return$purgedHttp ||$purgedHttps; |
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.
Why changing this? The old version was better IMO as it doesn't execute the second purge if the one is properly done.
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.
That was the problem. Both versions of the cache entry – http AND https – should get purged. Before this change only the http version was purged.
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 bad. I was disturbed by the: change in the regex..
| $this->assertTrue($this->store->purge('http://example.com/foo')); | ||
| $this->assertEmpty($this->getStoreMetadata($requestHttp)); | ||
| $this->assertEmpty($this->getStoreMetadata($requestHttps)); | ||
| } |
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.
Am I wrong or the non-regression test is missing?
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.
What do you mean by non-regression test? I added two tests, one for each bug described in the initial issue comment.
fabpot commentedMar 21, 2017
Thank you@ausi. |
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
I found two bugs in
HttpCache\Store::purge()with HTTPS URLs:->purge('https://example.com/')only purges thehttpversion not thehttpsone.httpandhttps, only thehttpversion gets purged, thehttpsversion stays in the cache.I think this issues were introduced with#21582.
This pull request fixes both issues and adds tests for them.