Skip to content

[Cache] make Redis*Proxy extend Redis* #47292

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

Merged
merged 1 commit into from
Sep 12, 2022

Conversation

nicolas-grekas
Copy link
Member

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.

@carsonbot carsonbot added this to the 6.2 milestone Aug 16, 2022
@nicolas-grekas nicolas-grekas force-pushed the cache-proxy branch 4 times, most recently from a6b5aad to 269791d Compare August 16, 2022 15:59
/**
* @internal
*/
class RedisCluster6Proxy extends \RedisCluster
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't those committed proxies break if Redis has a new method in a minor version ?

Copy link
Member Author

Choose a reason for hiding this comment

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

They would. That's why I added a test case to ensure we're in sync.

Copy link
Member

Choose a reason for hiding this comment

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

but then, this means that all existing releases of Symfony become broken when ext-redis releases a new version, until we release a version with the new methods. I find that quite bad DX for our users.

Copy link
Member Author

Choose a reason for hiding this comment

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

They are not really broken until you start using the new methods. I think this is acceptable since ppl using the new methods can wait a new release. The window should be pretty short.

@nicolas-grekas nicolas-grekas force-pushed the cache-proxy branch 3 times, most recently from 73b05fe to e21267e Compare August 17, 2022 09:52
@nicolas-grekas nicolas-grekas force-pushed the cache-proxy branch 3 times, most recently from 39fa090 to b67509c Compare August 28, 2022 17:11
@nicolas-grekas nicolas-grekas force-pushed the cache-proxy branch 2 times, most recently from 0f57be2 to df78694 Compare September 3, 2022 12:35
@nicolas-grekas
Copy link
Member Author

(PR rebased and ready)

@nicolas-grekas nicolas-grekas force-pushed the cache-proxy branch 4 times, most recently from 99c13cc to 9e703c6 Compare September 6, 2022 07:11
@nicolas-grekas nicolas-grekas merged commit ee78099 into symfony:6.2 Sep 12, 2022
@nicolas-grekas nicolas-grekas deleted the cache-proxy branch September 27, 2022 16:55
chalasr added a commit to chalasr/symfony that referenced this pull request Oct 23, 2022
…` (nicolas-grekas)"

This reverts commit ee78099, reversing
changes made to 0b32537.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Cache] Mark RedisProxy as non-@internal?
4 participants