Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Cache] TraceableAdapter#21082
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
[Cache] TraceableAdapter#21082
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| public static function setupBeforeClass() | ||
| { | ||
| parent::setupBeforeClass(); | ||
| self::$redis = new \Redis(); |
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.
I'd base this on FilesystemAdapter so that more people can run the tests
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.
Thanks... I just want an adapter that can run all tests.
nicolas-grekas commentedDec 28, 2016
👍 |
| $event->end = microtime(true); | ||
| } | ||
| if ($event->result) { |
nicolas-grekasDec 28, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I felt the same. I'll gather this info in the DataCollector instead.
| } | ||
| /** | ||
| * @internal |
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.
Does it really make sense to tag the event class as internal when we return it in thegetCalls() method?
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.
OK for removing the tag then
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.
👍 after this change
xabbuh commentedDec 28, 2016
👍 Status: Reviewed |
| public function getCalls() | ||
| { | ||
| return $this->calls; |
nicolas-grekasDec 29, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 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();}
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.
Sure, makes sense
| * @author Tobias Nyholm <tobias.nyholm@gmail.com> | ||
| * @author Nicolas Grekas <p@tchwork.com> | ||
| */ | ||
| class TraceableAdapter implements AdapterInterface |
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.
Isnt it more a wrapper then a adapter as the class doesnt adapt one api for another?
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.
Strictly seen you are right. But I think it would be weird to change the name as we are implementing theAdapterInterface here, wouldn't it?
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.
I see
nicolas-grekas commentedDec 29, 2016
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/aI moved the TraceableAdapter to a separate PR and added some tests. I found a small bug which was fixed.Commits-------5cc4baf [Cache] TraceableAdapter
Nyholm commentedDec 29, 2016
Thank you for merging |
I moved the TraceableAdapter to a separate PR and added some tests. I found a small bug which was fixed.