avoid duplicate requests to the cache proxy. #126
Conversation
dbu
force-pushed
the
no-duplicate-proxy-requests
branch
from
ddd3d18
to
a76b98d
Compare
| @@ -347,6 +347,8 @@ function ($requests) use ($self) { | |||
| $varnish | |||
| ->purge('/c') | |||
| ->purge('/b') | |||
| ->purge('/b') | |||
| ->purge('/b') | |||
There was a problem hiding this comment.
you should add a test using headers
dbu
force-pushed
the
no-duplicate-proxy-requests
branch
from
a76b98d
to
ec7540e
Compare
|
thanks stof. sorting the headers now, and added a separate explicit test which includes the headers. |
| @@ -130,7 +130,10 @@ public function flush() | |||
| */ | |||
| protected function queueRequest($method, $url, array $headers = array()) | |||
| { | |||
| $this->queue[] = $this->createRequest($method, $url, $headers); | |||
| $signature = md5($method . "\n" . $url . "\n" . implode("\n", ksort($headers))); | |||
There was a problem hiding this comment.
ksort modifies the array by reference, it does not return a sorted array.
And you should probably move the generation of the signature to a private method, or use a different variable for the sorted array. We should try to keep the submitted headers in the provided order if possible
dbu
force-pushed
the
no-duplicate-proxy-requests
branch
from
ec7540e
to
a0ffbaa
Compare
|
good thing with the test, actually found a bug. alright, moved that to a method and did the sorting correctly. the header order should only matter to the user debugging what is going on. if we have 2 requests with different order, we will only see one of them being sent, so if order would technically matter there would be a problem. |
| * | ||
| * @return string A hash value for this request. | ||
| */ | ||
| private function getSignature($method, $url, array $headers) |
There was a problem hiding this comment.
this means that if somebody overwrites queueRequest, he will either have to copy this method or not use a hash. should we make it protected?
There was a problem hiding this comment.
the queue is private, so you have to call the parent method anyway if you overwrite it. queueRequest is not designed to be overwritten in child classes, but to be called in child classes
dbu
force-pushed
the
no-duplicate-proxy-requests
branch
from
a0ffbaa
to
a2c253f
Compare
| { | ||
| ksort($headers); | ||
|
|
||
| return md5($method . "\n" . $url . "\n" . var_export($headers, true)); |
There was a problem hiding this comment.
now respecting the headers too.
i opt to not play around with upper/lowercase as it complicates the code (and makes it slower) for no value unless people do really weird code, in which case they just get duplicate requests but no errors.
|
@ddeboer ok to merge this? |
| @@ -42,7 +42,7 @@ | |||
| }, | |||
| "extra": { | |||
| "branch-alias": { | |||
| "dev-master": "1.0.x-dev" | |||
| "dev-master": "1.1.x-dev" | |||
There was a problem hiding this comment.
this is a new feature but no BC break (the behaviour changes, but not sending duplicate requests sounds like it should not break anything) - so i bump the minor version.
…ests Avoid duplicate requests to the cache proxy
fix #125