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

Allow for Redis 5.0 #29029

Closed
driesvints opened this issue Jul 2, 2019 · 6 comments · Fixed by #29606
Closed

Allow for Redis 5.0 #29029

driesvints opened this issue Jul 2, 2019 · 6 comments · Fixed by #29606

Comments

@driesvints
Copy link
Member

PhpRedis 5.0 was just released but apparently some changes are needed in Laravel before it can be used with the framework. There's nothing yet in the changelog so unsure what has changed.

We should make sure that no breaking changes are made that would disallow usage of older phpredis versions. Otherwise the changes will have to go to master.

I made a change to the build to only test on the latest 4.x release for now: 0596050

@GrahamCampbell
Copy link
Member

Are you referring to these changes?

1) Illuminate\Tests\Redis\RedisConnectionTest::it_calculates_intersection_of_sorted_sets_and_stores
ErrorException: Function Redis::zInter() is deprecated
/home/travis/build/laravel/framework/src/Illuminate/Redis/Connections/Connection.php:96
/home/travis/build/laravel/framework/src/Illuminate/Redis/Connections/Connection.php:108
/home/travis/build/laravel/framework/src/Illuminate/Redis/Connections/PhpRedisConnection.php:409
/home/travis/build/laravel/framework/src/Illuminate/Redis/Connections/PhpRedisConnection.php:245
/home/travis/build/laravel/framework/tests/Redis/RedisConnectionTest.php:263
2) Illuminate\Tests\Redis\RedisConnectionTest::it_calculates_union_of_sorted_sets_and_stores
ErrorException: Function Redis::zUnion() is deprecated
/home/travis/build/laravel/framework/src/Illuminate/Redis/Connections/Connection.php:96
/home/travis/build/laravel/framework/src/Illuminate/Redis/Connections/Connection.php:108
/home/travis/build/laravel/framework/src/Illuminate/Redis/Connections/PhpRedisConnection.php:409
/home/travis/build/laravel/framework/src/Illuminate/Redis/Connections/PhpRedisConnection.php:261
/home/travis/build/laravel/framework/tests/Redis/RedisConnectionTest.php:295

@driesvints
Copy link
Member Author

Those errors and these failures:

There were 2 failures:
1) Illuminate\Tests\Redis\RedisConnectionTest::test_it_returns_range_in_sorted_set
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'jeffrey' => 1
-    'matt' => 5
+    0 => 'jeffrey'
+    1 => 'matt'
 )
/home/travis/build/laravel/framework/tests/Redis/RedisConnectionTest.php:296
2) Illuminate\Tests\Redis\RedisConnectionTest::test_it_returns_rev_range_in_sorted_set
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'matt' => 5
-    'taylor' => 10
+    0 => 'taylor'
+    1 => 'matt'
 )
/home/travis/build/laravel/framework/tests/Redis/RedisConnectionTest.php:309

https://travis-ci.org/laravel/framework/jobs/553192452

@owenvoke
Copy link
Contributor

owenvoke commented Jul 3, 2019

The changes are listed on PECL at the moment. Unless I've made a mistake?
https://pecl.php.net/package-info.php?package=redis&version=5.0.0

(And in the source package.xml@a5dfff3)

Looks like it's also been added in a slightly nicer format in the CHANGELOG on GitHub. 👍

@owenvoke
Copy link
Contributor

owenvoke commented Jul 9, 2019

Is this possibly related to the breaking change to zRange?
phpredis/phpredis#1555 (phpredis/phpredis@19f3efc)


I tried testing it with the new array syntax (seems to be preferred) which seems to resolve the zRange issues:
https://github.com/pxgamer/framework/runs/164789768 (0f2cb2b)

And the same with the boolean syntax.
https://github.com/pxgamer/framework/runs/164789982 (02699cd)


The deprecated method errors seem to be related to the following commits:

I've added a test to see if these can be resolved by updating the PhpRedisConnection class.
https://github.com/pxgamer/framework/runs/164800716 (6c551c1)

This seems to (when combined with the array syntax fix above) resolve the issues with those warnings.


Note, I personally don't use Redis yet so I have only tested this against the unit tests with Redis 5.0

@ragingdave
Copy link
Contributor

ragingdave commented Jul 18, 2019

There is also still likely this issue, #24222 which was merged but then reverted so it's not actually there, but most of the prefix based functions don't work under phpredis at the moment. I unfortunately haven't had a chance to revisit those issues.

@driesvints
Copy link
Member Author

@pxgamer thanks. That really helped to get the ball rolling. Sent in a PR here: #29606

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 a pull request may close this issue.

4 participants