Skip to content

[Lock] Make sure RedisStore will also support Valkey #59488

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 13, 2025
Merged

[Lock] Make sure RedisStore will also support Valkey #59488

merged 1 commit into from
Jan 13, 2025

Conversation

PatNowak
Copy link

@PatNowak PatNowak commented Jan 13, 2025 •
edited by nicolas-grekas
Loading

Q A
Branch? 7.2
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #59387
License MIT

As the change happened on Valkey (https://github.com/valkey-io/valkey/pull/617/files#diff-1abc5651133d108c0c420d9411925373c711133e7748d9e4f4c97d5fb543fdd9L1819-R1819), it's hard to consider this a bug when RedisStore was meant only to support Redis ;)

@PatNowak PatNowak requested a review from jderusse as a code owner January 13, 2025 08:38
@carsonbot carsonbot added this to the 7.3 milestone Jan 13, 2025
@nicolas-grekas
Copy link
Member

Thanks for digging this. Let's merge it as a bugfix still. 👍 for 7.2 on my side.

@nicolas-grekas nicolas-grekas modified the milestones: 7.3, 7.2 Jan 13, 2025
@carsonbot carsonbot changed the title Make sure RedisStore will also support Valkey [Lock] Make sure RedisStore will also support Valkey Jan 13, 2025
nicolas-grekas
nicolas-grekas reviewed Jan 13, 2025
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

// $this->redis is just $redis: '@snc_redis.default' from services.yml

OK, so you'd need to fix your connection service in "no exceptions" mode to be compatible with the implementation.

The patch LGTM now, just minor things.

@PatNowak PatNowak mentioned this pull request Jan 13, 2025
Open
@fabpot fabpot changed the base branch from 7.3 to 7.2 January 13, 2025 13:22
@fabpot
Copy link
Member

fabpot commented Jan 13, 2025

Thank you @PatNowak.

@fabpot fabpot merged commit a8a9e0b into symfony:7.2 Jan 13, 2025
7 checks passed
@stof
Copy link
Member

stof commented Jan 13, 2025

If we want to be reliable regarding changes to the wording of error messages, shouldn't we use only NOSCRIPT as prefix in the check, i.e. checking only the error type ?

@johanwilfer
Copy link
Contributor

Yeah, we should check the prefix to be NOSCRIPT instead probably. See here: valkey-io/valkey#1537 (comment)

And they link to the documentation saying only the first word should be matched if possible. So while this fixes the issue with Valkey it can probably be a bit more robust.

@fabpot fabpot mentioned this pull request Jan 29, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@simPod simPod simPod left review comments

@fabpot fabpot fabpot approved these changes

@xabbuh xabbuh xabbuh approved these changes

@jderusse jderusse Awaiting requested review from jderusse jderusse is a code owner

@lyrixx lyrixx Awaiting requested review from lyrixx

@kbond kbond Awaiting requested review from kbond

@chalasr chalasr Awaiting requested review from chalasr

@dunglas dunglas Awaiting requested review from dunglas

@yceruto yceruto Awaiting requested review from yceruto

@OskarStark OskarStark Awaiting requested review from OskarStark

Assignees
No one assigned
Labels
Bug Lock Status: Reviewed
Projects
None yet
Milestone
7.2
Development

Successfully merging this pull request may close these issues.

[Lock] Lock::acquire will always return false when using RedisStore
8 participants
@PatNowak @nicolas-grekas @fabpot @stof @johanwilfer @simPod @xabbuh @carsonbot