Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:7.2fromPatNowak:issue-59387
Jan 13, 2025

Conversation

PatNowak
Copy link

@PatNowakPatNowak commentedJan 13, 2025•
edited by nicolas-grekas
Loading

QA
Branch?7.2
Bug fix?yes
New feature?no
Deprecations?no
IssuesFix#59387
LicenseMIT

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 whenRedisStore was meant only to support Redis ;)

@nicolas-grekas
Copy link
Member

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

PatNowak reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas modified the milestones:7.3,7.2Jan 13, 2025
@carsonbotcarsonbot changed the titleMake sure RedisStore will also support Valkey[Lock] Make sure RedisStore will also support ValkeyJan 13, 2025
Copy link
Member

@nicolas-grekasnicolas-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 reacted with thumbs up emoji
@fabpot
Copy link
Member

Thank you@PatNowak.

PatNowak reacted with thumbs up emoji

@fabpotfabpot merged commita8a9e0b intosymfony:7.2Jan 13, 2025
7 checks passed
@stof
Copy link
Member

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

zuiderkwast reacted with thumbs up emoji

@johanwilfer
Copy link
Contributor

Yeah, we should check the prefix to beNOSCRIPT 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.

@fabpotfabpot mentioned this pull requestJan 29, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@simPodsimPodsimPod left review comments

@fabpotfabpotfabpot approved these changes

@xabbuhxabbuhxabbuh approved these changes

@jderussejderusseAwaiting requested review from jderussejderusse is a code owner

@lyrixxlyrixxAwaiting requested review from lyrixx

@kbondkbondAwaiting requested review from kbond

@chalasrchalasrAwaiting requested review from chalasr

@dunglasdunglasAwaiting requested review from dunglas

@ycerutoycerutoAwaiting requested review from yceruto

@OskarStarkOskarStarkAwaiting requested review from OskarStark

Assignees
No one assigned
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
[8]ページ先頭
©2009-2025 Movatter.jp