search

Baldinof / roadrunner-bundle

A RoadRunner worker integrated in your Symfony app
MIT License
266 stars 47 forks source link

feat: introduce KV cache adapters #137

Closed StanJansen closed 6 months ago

StanJansen commented 7 months ago

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.

StanJansen commented 6 months ago

@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: https://github.com/symfony/symfony/issues/28528

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

Baldinof commented 6 months ago

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: https://github.com/symfony/symfony/issues/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 commented 6 months ago

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 commented 6 months ago

Cool!

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

Baldinof commented 6 months ago

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

StanJansen commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

You rock!

Thank you :)

I will try to finish https://github.com/Baldinof/roadrunner-bundle/pull/130 and make a release

StanJansen commented 6 months ago

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 commented 6 months ago

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