[Cache] add CacheInterface::delete() + improve CacheTrait #28718
Conversation
a8cb1b4 to
d53c126
Compare
| @@ -116,17 +121,51 @@ public function testExceptionOnNegativeBeta() | |||
| $this->expectException(\InvalidArgumentException::class); | |||
| $cache->get('key', $callback, -2); | |||
| } | |||
|
|
|||
| private function getPool() | |||
There was a problem hiding this comment.
good catch, removed
| * | ||
| * @author Nicolas Grekas <p@tchwork.com> | ||
| */ | ||
| trait CacheContractsTrait |
There was a problem hiding this comment.
I don't believe names of classes containing *Contracts* under Symfony\Contracts namespace have enough value.
Also, such poor name of this trait suggests it has excessively broad responsibility. Any cache thing falls in. Actually, this time period of contracts introduction could be used as opportunity to improve name of CacheIterface as well. PSR did it better with its pool.
There was a problem hiding this comment.
The responsibility is mostly given by the interface this allows implementing.
The specific added responsibility here is providing an implementation of get+delete using PSR-6 methods, so that implementing the contracts interface is a oneliner.
Something like CacheInterfaceImplementationForCacheItemPoolInterfaceTrait would be more precise, but I fear that's not really better. At least, from a consumer pov (ie a PSR-6 implementer), having use CacheContractsTrait; might be a good enough hint to quickly remind one about what this is doing.
But I'd happily listen to better suggestions.
opportunity to improve name of CacheIterface as well
Sure, naming is hard :) Any suggestions? If we really want an alternative name, my proposal would be CacheStorageInterface. This has the benefit of not colliding with PSR-16 CacheInterface (if we should care.)
There was a problem hiding this comment.
Yep I like CacheStorage* :)
Isn't tight coupling of this trait to interface sign it would be better to replace these with abstract class?
There was a problem hiding this comment.
I'm fine renaming CacheInterface to CacheStorageInterface and the trait to CacheStorageTrait. It could also be CachePool*. Any preference anyone (and why?) That's 3 alternatives.
Isn't tight coupling of this trait to interface sign it would be better to replace these with abstract class?
I don't see any tight coupling here, just an implementation. Making it an abstract class would make it impossible to reuse. .e.g it wouldn't fit in the cache component. Inheritance is a bad extensibility point.
There was a problem hiding this comment.
CacheInterface is simpler. Im not sure how many times I've been fighting with PSR-6 interfaces' complicated names.
I vote for no rename of the interface.
The trait however. Not sure... Dont care much =)
d53c126 to
8dceb51
Compare
383cbb7 to
221cbb5
Compare
|
Thank you @ostrolucky and @Nyholm for the review. |
221cbb5 to
ffc71fc
Compare
| use Psr\Cache\InvalidArgumentException; | ||
|
|
||
| /** | ||
| * Gets/Stores items from/to a cache. | ||
| * Covers most simple to advanced caching needs. | ||
| * | ||
| * On cache misses, a callback is called that should return the missing value. |
There was a problem hiding this comment.
This section should be moved next to get method
ffc71fc to
c6cf690
Compare
Im not convinced this PR is needed. Doing fetch, unserialize before save is an implementation detail. It might be easier to use though... |
yes, I used that as a hint, but my main reasoning is that it should be a common use case to invalidate by deletion. I consider I've been misled by what we do in core: we never delete. But that's because we deal with append-only pools. Working with data from models, deleting is just usual business. |
|
Thank you @nicolas-grekas. |
…ait (nicolas-grekas) This PR was merged into the 4.2-dev branch. Discussion ---------- [Cache] add CacheInterface::delete() + improve CacheTrait | Q | A | ------------- | --- | Branch? | 4.2 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - I've been hesitating a lot on this topic, but I think we should add a `delete()` method to `CacheInterface`. Deleting is a very common invalidation strategy and invalidating by passing `$beta=INF` to `get()` has the drawback of requiring a fetch+unserialize+save-with-past-expiration. That's complexity that a delete removes. This PR fixes an issue found meanwhile on `get()`: passing an `ItemInterface` to its callback makes it harder than required to implement on top of PSR-6. Let's pass a `CacheItemInterface`. Lastly, the early expiration logic can be moved from the component to the trait shipped on contracts. Here we are for all these steps. Commits ------- c6cf690 [Cache] add CacheInterface::delete() + improve CacheTrait
I've been hesitating a lot on this topic, but I think we should add a
delete()method toCacheInterface.Deleting is a very common invalidation strategy and invalidating by passing
$beta=INFtoget()has the drawback of requiring a fetch+unserialize+save-with-past-expiration. That's complexity that a delete removes.This PR fixes an issue found meanwhile on
get(): passing anItemInterfaceto its callback makes it harder than required to implement on top of PSR-6. Let's pass aCacheItemInterface.Lastly, the early expiration logic can be moved from the component to the trait shipped on contracts.
Here we are for all these steps.