Feature/psr16 support #791
Conversation
| /** | ||
| * Wrap the PSR simple cache implementation into the legacy interface | ||
| */ | ||
| class common_cache_PsrWrapperCache extends ConfigurableService implements common_cache_Cache |
There was a problem hiding this comment.
Let's not create new classes in legacy namespace
There was a problem hiding this comment.
This class provides the legacy interface and should be discouraged from being used, as is present for backward compatibility only.
There was a problem hiding this comment.
should be discouraged from being used
In that case it has to be marked as deprecated.
As we are going to get rid of legacy autoloader it's better to not create new classes in the legacy namespace
There was a problem hiding this comment.
Introduce in the PR a new deprecated class and then configure it to be used by default does not seem to me like a good practice!
How difficult will be to change it's usage to not catch common_cache_NotFoundException? I found 14 catches of that exception in three extensions:
tao
taoAct
taoMpArt (that one can be omitted)
I could help with cleaning that up.
There was a problem hiding this comment.
I removed all references to old cache from generis, so that we can move quicker when it comes to cleaning up old references, but I still don't want to make this feature backward incompatible, even if we could fix all other known extensions quickly
| { | ||
| $success = true; | ||
| foreach ($values as $key => $value) { | ||
| $success = $this->set($key, $value, $ttl) && $success; |
There was a problem hiding this comment.
It may confuse developers. In case of failure all already set values have to be deleted.
The same for delete. All already deleted items have to be restored.
There was a problem hiding this comment.
I could not find any indication in the PSR on how a partial success/failure should be handled
There was a problem hiding this comment.
In that case this behavior is up to implementation.
I think rolling back in that case corresponds to best practices.
There was a problem hiding this comment.
Unfortunately I have no idea how to implement this rollback as filesystems are not transactional
|
I don't like approach as it allows developers to use old cache interface. |
|
Unfortunately our legacy get() method is not compatible with the PSR interface, so having dual support would not be possible |
Signature of get() method is compatible:
Just add |
|
Our current implementation of get() throws an exception on cache miss, the PSR implementation returns the default value, so unfortunately this is not possible |
|
Throwing an exception when default value was not given does not seem like an interface violation. |
|
Looks good! |
|
Please fix phpdoc block ( |
|
I could fix the issues, but it is in code that is going to be deleted, so I did not bother. Are there incorecct tags in the part of the code that will not be deleted? |
Support for SimpleCache implementations in the context of the cached functional access control