RFC: Cache trait #1908
RFC: Cache trait #1908
Conversation
|
-1 as it's unnecessary and only makes the code more confusing. |
src/Cache/CacheTrait.php
Outdated
| { | ||
| public function getOrSave(string $cacheKey, CacheItemPoolInterface $pool, array &$localCache, callable $callback) | ||
| { | ||
| if (isset($localCache[$cacheKey])) { |
There was a problem hiding this comment.
What about introducing the localCache variable inside the trait instead of passing a reference?
|
I'd have done a parent abstract class instead. It'd remove |
|
If we really need to simplify the cache part, we should think about switching to PSR-16 ( |
|
I'm +1 to factorize this code, it's an error prone one, better not duplicate it everywhere. |
|
@dunglas preference on trait vs abstract class? |
|
In that case I'm -1 for something confusing to the reader of the code like |
|
@teohhanhui did you even read the PR description? |
|
Oh, I don't mean -1 for this PR. Sorry for the confusion. |
| } | ||
|
|
||
| return $this->localCache[$cacheKey] = $routeName; | ||
| return $this->getOrSave($cacheKey, $this->cacheItemPool, function () use ($resourceClass, $operationType, $context) { |
There was a problem hiding this comment.
Perhaps getCached? I also agree with @soyuka that we shouldn't need to pass the $cacheItemPool here, it should be in the trait.
| @@ -13,7 +13,7 @@ | |||
|
|
|||
| namespace ApiPlatform\Core\Bridge\Symfony\Routing; | |||
|
|
|||
| use Psr\Cache\CacheException; | |||
| use ApiPlatform\Core\Cache\CacheTrait; | |||
src/Cache/CacheTrait.php
Outdated
| { | ||
| private $localCache = []; | ||
|
|
||
| private function getOrSave(string $cacheKey, CacheItemPoolInterface $pool, callable $callback) |
There was a problem hiding this comment.
naming proposal:
<?php
private function cache(string $cacheKey, callable $getValue) {
}With a parent class we can remove CacheItemPoolInterface from the method signature
There was a problem hiding this comment.
We don't need a parent class. The property could be declared in the trait as we already do elsewhere?
There was a problem hiding this comment.
also works yes, I just think that parent class > trait if we can :) (ability to use instanceof parent etc.)
There was a problem hiding this comment.
I think using inheritance here is... wrong. It's the kind of use case which traits are exactly designed to solve - horizontal code reuse.
src/Cache/CacheTrait.php
Outdated
| use Psr\Cache\CacheException; | ||
| use Psr\Cache\CacheItemPoolInterface; | ||
|
|
||
| trait CacheTrait |
|
comments addressed. |
src/Cache/CachedTrait.php
Outdated
| //do nothing | ||
| } | ||
|
|
||
| $routeName = $getValue(); |
There was a problem hiding this comment.
Do you mean $value? 😆
src/Cache/CachedTrait.php
Outdated
|
|
||
| private function getCached(string $cacheKey, callable $getValue) | ||
| { | ||
| if (isset($this->localCache[$cacheKey])) { |
There was a problem hiding this comment.
Actually, I think we should use array_key_exists here, because we should support storing null values in the cache.
|
Thanks @bendavies! 🎉 |
RFC: Cache trait
cc @dunglas as discussed.
I noticed we were doing the same thing in quite a few places.
Thoughts please?
P.S. i've put very little thought into the naming of the trait and function. Please suggest something better!