platformsh / symfonyflex-bridge

Bridge library for running Symfony Flex on Platform.sh
MIT License
30 stars 15 forks source link

Redis support #42

Closed ryanolee closed 3 years ago

ryanolee commented 3 years ago

The PR aims to address the issues outlined in #41 by adding the options to map redis ENV vars into a symfony cache pool or a symfony session handler. This PR proposes a way to do that. It allows for 3 new relations to be mapped redis, redis_session and redis_cache. redis_session and redis_cache override the values specified by redis is they are defined so redis can be defined for either purpose (or both) independently of each other if separate stores are preferable.

Unit tests and documentation have been added to test and document this behaviour. Let me know what you think :+1:

Resolves #41

Crell commented 3 years ago

I didn't realize Symfony had a standard Redis session/cache backend now. Or are you just defining these env vars directly yourself?

Also, we don't support multiple relationship names on anything else so let's not do so here. The names should also be rediscache and redissession for consistency with other naming conventions, and with the Laravel bridge (which already maps those relationship names).

ryanolee commented 3 years ago

Hi @Crell, Just to confirm we ideally want to remove support for the redis platform relationship and have a rediscache and redissession platform relation that work in isolation from each other to fill in each of the use cases individually? Also thanks for the quick reply :rocket: Do you know if you able to relate the same service to each other multiple times :thinking: (https://docs.platform.sh/configuration/app/relationships.html). Considering in some cases you might want to use the same redis instance for both cache and session storage hence the generic redis fallback?

Crell commented 3 years ago

Correct. Having 2 relationships that point to the same endpoint works fine.

Have a look at the Laravel bridge, which already does this. Model on that and steal code liberally as needed. :smile:

https://github.com/platformsh/laravel-bridge

ryanolee commented 3 years ago

Applied the feedback @Crell :+1:

ryanolee commented 3 years ago

Not sure if we want to break out the redis stuff into two entirely separate tests but can apply those changes also if preferable :thinking: . In terms of cache backends, Symfony now does have built in support for it but it needs configuring. All that this does is expose env vars that symfony can pick up on and later use in config for the associated redis connection :smile: :tada:

Crell commented 3 years ago

Let's go ahead and split up the tests, yeah. A few minor formatting nits above, nothing drastic. While you're in there it looks like the RabbitMQ header also needs to be ##. Can you tweak that?

ryanolee commented 3 years ago

Hi @Crell , Re added the review changes and split the tests / made them more confirmative to the other tests in there.

Crell commented 3 years ago

Thanks! I'll tag a new release shortly.

ryanolee commented 3 years ago

Thanks @Crell ! 🎉