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

[11.x] Use null as default cursor value for PHP Redis #53095

Merged
merged 3 commits into from
Oct 10, 2024

Conversation

jayan-blutui
Copy link
Contributor

Fixes #53087

Additional context: phpredis/phpredis#2562

This PR attempts to fix the current issue with the changes made in PHP Redis 6.1.0 where the default cursor value of '0' is handled differently. With some testing, setting this to null fixes this issue.

I have attempted to only make its fix available to PHP Redis and continue to use the previous value of '0' for PRedis. If this change isn't as per the framework's standard. Happy to make changes or totally understand if this is closed. 😄

@jayan-blutui
Copy link
Contributor Author

Not sure why the Windows builds failed here at the setup PHP step 🤔

Perhaps it needs to be re-run?

$connection = $this->store->connection();

$defaultCursorValue = match (true) {
$connection instanceof PhpRedisConnection => null,

Choose a reason for hiding this comment

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

As above

@@ -292,10 +292,15 @@ protected function currentTags($chunkSize = 1000)
default => '',
};

$defaultCursorValue = match (true) {
$connection instanceof PhpRedisConnection => null,
Copy link

@scottasmith scottasmith Oct 10, 2024

Choose a reason for hiding this comment

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

This probably should also do a version check as well as checking its PhpRedisConnection. That way it doesn't break it for people who have held back on upgrading the extension

if (version_compare(phpversion('redis'), '6.1.0', '>=')) {
    ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea. How about I do something like:

$defaultCursorValue = match (true) {
    $connection instanceof PhpRedisConnection && version_compare(phpversion('redis'), '6.1.0', '>=') => null,
    default => '0',
};

Choose a reason for hiding this comment

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

I'm not a maintainer, but for me this would work 😄

@taylorotwell taylorotwell merged commit 9201b98 into laravel:11.x Oct 10, 2024
31 checks passed
timacdonald pushed a commit to timacdonald/framework that referenced this pull request Oct 15, 2024
* fix: PHP Redis default cursor value

* remove 0 as default value

* add version check for phpredis 6.1.0 or above
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues with latest phpredis breaking RedisTaggedCache
3 participants