[Cache] TraceableAdapter #21082
[Cache] TraceableAdapter #21082
Conversation
| public static function setupBeforeClass() | ||
| { | ||
| parent::setupBeforeClass(); | ||
| self::$redis = new \Redis(); |
There was a problem hiding this comment.
I'd base this on FilesystemAdapter so that more people can run the tests
There was a problem hiding this comment.
Thanks... I just want an adapter that can run all tests.
|
👍 |
| $event->end = microtime(true); | ||
| } | ||
|
|
||
| if ($event->result) { |
There was a problem hiding this comment.
thinking twice about this, I'd prefer removing this if/else now :) $event->result already allows to infer it, and it could be controversial to map true==hasItem to a hit. wdyt?
There was a problem hiding this comment.
I felt the same. I'll gather this info in the DataCollector instead.
| } | ||
|
|
||
| /** | ||
| * @internal |
There was a problem hiding this comment.
Does it really make sense to tag the event class as internal when we return it in the getCalls() method?
There was a problem hiding this comment.
OK for removing the tag then
|
👍 Status: Reviewed |
|
|
||
| public function getCalls() | ||
| { | ||
| return $this->calls; |
There was a problem hiding this comment.
What about resetting $this->calls here? This would allow reusing the same adapter several times in a long running process with no mem leak.
$calls = $this->calls;
$this->calls = array();
return $calls;or
try {
return $this->calls;
} finally {
$this->calls = array();
}| * @author Tobias Nyholm <tobias.nyholm@gmail.com> | ||
| * @author Nicolas Grekas <p@tchwork.com> | ||
| */ | ||
| class TraceableAdapter implements AdapterInterface |
There was a problem hiding this comment.
Isnt it more a wrapper then a adapter as the class doesnt adapt one api for another?
There was a problem hiding this comment.
Strictly seen you are right. But I think it would be weird to change the name as we are implementing the AdapterInterface here, wouldn't it?
|
Thank you @Nyholm. |
This PR was squashed before being merged into the 3.3-dev branch (closes #21082). Discussion ---------- [Cache] TraceableAdapter | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | Related to #21065 | License | MIT | Doc PR | n/a I moved the TraceableAdapter to a separate PR and added some tests. I found a small bug which was fixed. Commits ------- 5cc4baf [Cache] TraceableAdapter
|
Thank you for merging |
I moved the TraceableAdapter to a separate PR and added some tests. I found a small bug which was fixed.