Skip to content

[Cache] Mark RedisProxy as non-@internal? #47267

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

Closed
spaze opened this issue Aug 12, 2022 · 4 comments · Fixed by #47292
Closed

[Cache] Mark RedisProxy as non-@internal? #47267

spaze opened this issue Aug 12, 2022 · 4 comments · Fixed by #47292
Labels

Comments

@spaze
Copy link

spaze commented Aug 12, 2022

Description

Hi,
I'm using RedisAdapter in an app that already uses Redis (the server) heavily and I'd like to reuse the same connection in the adapter. The connection has several non-default options set and I could open it and pass the \Redis object to the adapter but that would mean the connection is non-lazy. I'd like it to be lazy, to be opened when needed.

There's an option for lazy connections when using a DSN which creates a RedisProxy object and passes it instead of the Redis object but that means another connection in my case. The lazy option is here https://github.com/symfony/cache/blob/5cf8e75f02932818889e0609380b8d5427a6c86c/Traits/RedisTrait.php#L247-L251

The proxy class is very useful for my case, I can set it up and pass a Redis instance and create an initializer closure that would call my connect method which sets options I need, when needed.

I've noticed the RedisProxy class is marked as @internal. I'm a bit nervous when using internal classes so I was wondering whether it would be possible to drop the tag and if I can send a PR and what else it should contain. Or what would be the best way forward to reuse connections in a lazy way.

Thanks!

Example

 vendor/symfony/cache/Traits/RedisProxy.php | 1 -
 1 file changed, 1 deletion(-)

diff --git c/vendor/symfony/cache/Traits/RedisProxy.php w/vendor/symfony/cache/Traits/RedisProxy.php
index ec5cfabb3..5df60c179 100644
--- c/vendor/symfony/cache/Traits/RedisProxy.php
+++ w/vendor/symfony/cache/Traits/RedisProxy.php
@@ -14,7 +14,6 @@ namespace Symfony\Component\Cache\Traits;
 /**
  * @author Nicolas Grekas <p@tchwork.com>
  *
- * @internal
  */
 class RedisProxy
 {
@ro0NL
Copy link
Contributor

ro0NL commented Aug 12, 2022

See also #42428

@nicolas-grekas
Copy link
Member

With #47236 we should be able to make Redis lazy without RedisProxy. Worth a try after merging. Then we might consider deprecating RedisProxy instead.

@spaze
Copy link
Author

spaze commented Aug 12, 2022

Looks good but not sure it would fit my scenario, I'm using symfony/cache as a standalone lib, not with DI.

@spaze
Copy link
Author

spaze commented Aug 14, 2022

Seems that RedisProxy is indeed an internal thing and as such might be changed or removed as needed. Making it non-internal doesn't seem like the best thing to do at least for now. I've used lazy features of the DI container I'm using to make my cache adapters (and the Redis connection) lazy, so I'm fine.

Thanks everyone for the hints and comments!

@spaze spaze closed this as not planned Won't fix, can't repro, duplicate, stale Aug 14, 2022
nicolas-grekas added a commit that referenced this issue Sep 12, 2022
This PR was merged into the 6.2 branch.

Discussion
----------

[Cache] make `Redis*Proxy` extend `Redis*`

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #47267, #42428
| License       | MIT
| Doc PR        | -

`Redis*Proxy` did not extend native classes because we missed the code infrastructure to generate appropriate proxies.
With #47236, we now have it. This PR adds generated proxies to the cache component to keep it lazy-able out of the box.

Commits
-------

cfb46ed [Cache] make `Redis*Proxy` extend `Redis*`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants