Skip to content
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

Merged
merged 6 commits into from
Mar 13, 2024

Conversation

StanJansen
Copy link
Contributor

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.

Copy link
Owner

@Baldinof Baldinof left a 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()
Copy link
Owner

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?

Copy link
Contributor Author

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.


class KvCacheAdapter extends Psr16Adapter
{
public static function createConnection(#[\SensitiveParameter] string $dsn, array $options = []): mixed
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public static function createConnection(#[\SensitiveParameter] string $dsn, array $options = []): mixed
public static function createConnection(#[\SensitiveParameter] string $dsn, array $options = []): self

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied.

use Spiral\RoadRunner\KeyValue\Factory;
use Symfony\Component\Cache\Adapter\Psr16Adapter;

class KvCacheAdapter extends Psr16Adapter
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied.

@StanJansen
Copy link
Contributor Author

@Baldinof Thanks for the review!

I've applied your comments and added tests. I had to add a new configureContainerCallable to the getKernel method in the test, as the generated kv services are not used and thus removed from the container: symfony/symfony#28528

So without this workaround I could not assert that they were configured correctly.

@Baldinof
Copy link
Owner

I had to add a new configureContainerCallable to the getKernel method in the test, as the generated kv services are not used and thus removed from the container: symfony/symfony#28528

I think if you add a cache config that uses the roadrunner adapter it should keep the services. And in fact it would test the integration with the symfony cache layer. WDYT?

@StanJansen
Copy link
Contributor Author

I had to add a new configureContainerCallable to the getKernel method in the test, as the generated kv services are not used and thus removed from the container: symfony/symfony#28528

I think if you add a cache config that uses the roadrunner adapter it should keep the services. And in fact it would test the integration with the symfony cache layer. WDYT?

Very good idea! I've updated the test.

The cache.adapter.roadrunner.kv_foo service is not available, but the cache.foo one is (which is testable as an instance of KvCacheAdapter), which also tests the Symfony cache integration.

@Baldinof
Copy link
Owner

Baldinof commented Mar 13, 2024

Cool!

The tests seems to not pass with lowest dependencies, can you have a look please ?

@Baldinof
Copy link
Owner

PHPStan is already failing on default branch, so it's just about the PHPUnit job

@StanJansen
Copy link
Contributor Author

Cool!

The tests seems to not pass with lowest dependencies, can you have a look please ?

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 cache.app & cache.system, as those two are always loaded, also in older versions.

@StanJansen
Copy link
Contributor Author

PHPStan is already failing on default branch, so it's just about the PHPUnit job

@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 :)

@Baldinof Baldinof merged commit 3b2b421 into Baldinof:3.x Mar 13, 2024
9 checks passed
@Baldinof
Copy link
Owner

You rock!

Thank you :)

I will try to finish #130 and make a release

@StanJansen
Copy link
Contributor Author

You rock!

Thank you :)

I will try to finish #130 and make a release

@Baldinof do you have an ETA on this? As I prefer using your bundle and not my own fork, so I know when to plan on updating this :)

@Baldinof
Copy link
Owner

I did not find the time to finish the other PR so I released 3.1.0 for now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants