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

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

Merged
merged 1 commit into from Sep 12, 2022

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Aug 16, 2022

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.

@nicolas-grekas nicolas-grekas requested a review from jderusse as a code owner Aug 16, 2022
@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 Aug 16, 2022
/**
* @internal
*/
class RedisCluster6Proxy extends \RedisCluster
Copy link
Member

@stof stof Aug 16, 2022

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

@nicolas-grekas nicolas-grekas Aug 16, 2022

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

@stof stof Aug 16, 2022

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

@nicolas-grekas nicolas-grekas Aug 16, 2022

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 Aug 17, 2022
@nicolas-grekas nicolas-grekas force-pushed the cache-proxy branch 3 times, most recently from 39fa090 to b67509c Compare Aug 28, 2022
@nicolas-grekas nicolas-grekas force-pushed the cache-proxy branch 2 times, most recently from 0f57be2 to df78694 Compare Sep 3, 2022
@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Sep 3, 2022

(PR rebased and ready)

@nicolas-grekas nicolas-grekas force-pushed the cache-proxy branch 4 times, most recently from 99c13cc to 9e703c6 Compare Sep 6, 2022
@nicolas-grekas nicolas-grekas merged commit ee78099 into symfony:6.2 Sep 12, 2022
7 of 9 checks passed
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.

4 participants