-
-
Notifications
You must be signed in to change notification settings - Fork 48
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: introduce KV cache adapters #137
Conversation
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.
Hello!
Thank you for the PR :)
Some minor comment.
It would be nice if you could add some tests here to validate the bundle configuration registers the cache in the right way.
->arrayNode('kv') | ||
->addDefaultsIfNotSet() | ||
->children() | ||
->scalarNode('rpc_dsn')->defaultValue('tcp://127.0.0.1:6001')->end() |
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.
As RPC could be used for other RoadRunner plugins, I think it's worth it to move the config to the root of the bundle config.
It would require to extract the RPC::create()
call and maybe make a re-usable RPC service.
What do you think?
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 suggestion, upon looking in the code I noticed the bundle already has a RPCFactory
service, I removed the config and used that one instead.
src/Cache/KvCacheAdapter.php
Outdated
|
||
class KvCacheAdapter extends Psr16Adapter | ||
{ | ||
public static function createConnection(#[\SensitiveParameter] string $dsn, array $options = []): mixed |
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.
public static function createConnection(#[\SensitiveParameter] string $dsn, array $options = []): mixed | |
public static function createConnection(#[\SensitiveParameter] string $dsn, array $options = []): self |
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.
Applied.
src/Cache/KvCacheAdapter.php
Outdated
use Spiral\RoadRunner\KeyValue\Factory; | ||
use Symfony\Component\Cache\Adapter\Psr16Adapter; | ||
|
||
class KvCacheAdapter extends Psr16Adapter |
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.
class KvCacheAdapter extends Psr16Adapter | |
/** | |
* @internal | |
*/ | |
final class KvCacheAdapter extends Psr16Adapter |
Unless there is actual use case to allow extension and creation from outside of the bundle.
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.
Applied.
@Baldinof Thanks for the review! I've applied your comments and added tests. I had to add a new So without this workaround I could not assert that they were configured correctly. |
I think if you add a |
Very good idea! I've updated the test. The |
Cool! The tests seems to not pass with lowest dependencies, can you have a look please ? |
PHPStan is already failing on default branch, so it's just about the PHPUnit job |
Ah, older versions of Symfony did not have cache pools public, so it had the same issue as before: they were removed from the container because they weren't used. I fixed it by assigning them to |
@Baldinof Checked the PHPStan error and it was a very small issue (with older Symfony versions). I've updated that one aswell so the pipeline is all green now :) |
You rock! Thank you :) I will try to finish #130 and make a release |
I did not find the time to finish the other PR so I released 3.1.0 for now :) |
This feature would make it possible to create (and automatically generate) Symfony cache adapters for the KV plugin. It leverages Symfony's PSR16 cache adapter.
The changes in the
README.md
include the instructions on how to use this.This should be a non-breaking change that's opt-in.