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] add CacheInterface::delete() + improve CacheTrait#28718
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
a8cb1b4 tod53c126Compare| $cache->get('key',$callback, -2); | ||
| } | ||
| privatefunctiongetPool() |
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.
unused 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.
good catch, removed
| * | ||
| * @author Nicolas Grekas <p@tchwork.com> | ||
| */ | ||
| trait CacheContractsTrait |
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 don't believe names of classes containing*Contracts* underSymfony\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 itspool.
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.
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 likeCacheInterfaceImplementationForCacheItemPoolInterfaceTrait would be more precise, but I fear that's not really better. At least, from a consumer pov (ie a PSR-6 implementer), havinguse 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 beCacheStorageInterface. This has the benefit of not colliding with PSR-16CacheInterface (if we should care.)
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm fine renamingCacheInterface toCacheStorageInterface and the trait toCacheStorageTrait. It could also beCachePool*. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 =)
Uh oh!
There was an error while loading.Please reload this page.
383cbb7 to221cbb5Comparenicolas-grekas commentedOct 6, 2018
Thank you@ostrolucky and@Nyholm for the review. |
| *Gets/Stores items from/toa cache. | ||
| *Covers most simpletoadvanced caching needs. | ||
| * | ||
| * On cache misses, a callback is called that should return the missing value. |
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.
This section should be moved next to get 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.
moved
Nyholm commentedOct 6, 2018
Im not convinced this PR is needed. Doing fetch, unserialize before save is an implementation detail. It might be easier to use though... |
nicolas-grekas commentedOct 6, 2018 • 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.
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. |
fabpot commentedOct 10, 2018
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
Uh oh!
There was an error while loading.Please reload this page.
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.