Adding support for RedisCluster (phpredis extension)

[JacobBrownAustin]

related pull request:
colinmollenhour/php-redis-session-abstract#62

[colinmollenhour]

Thanks for the PR! I think this should probably just replace Credis_Cluster since it was never completed and would need a lot of work and having both would be confusing. Would you mind removing that, updating the README and add a basic PHPUnit test? Also, I think the constructor should probably throw an exception if the phpredis extension is not present.

[infowolfe]

@JacobBrownAustin thank you for this! Magento has been missing support for redis 6 for 4 years.

[JacobBrownAustin]

Thanks for the PR! I think this should probably just replace Credis_Cluster since it was never completed and would need a lot of work and having both would be confusing. Would you mind removing that, updating the README and add a basic PHPUnit test? Also, I think the constructor should probably throw an exception if the phpredis extension is not present.

Those all sound great. I'll update this soon.

[JacobBrownAustin]

@colinmollenhour , the Credis_Sentinel also uses Credis_Cluster. What should be done about that?

https://github.com/colinmollenhour/credis/blob/master/Sentinel.php#L312
https://github.com/colinmollenhour/credis/blob/master/Sentinel.php#L268

[JacobBrownAustin]

@colinmollenhour , because the new class is basically just an adapter to the php redis extension, there's not much logic to test in the class itself as a unit test, so I'll add a Redis cluster to the docker compose to test it more of an integration test.

[colinmollenhour]

@colinmollenhour , the Credis_Sentinel also uses Credis_Cluster. What should be done about that?

Both of those methods are marked as deprecated so they can be removed. The new version will get a major version bump already.

[colinmollenhour]

@colinmollenhour , because the new class is basically just an adapter to the php redis extension, there's not much logic to test in the class itself as a unit test, so I'll add a Redis cluster to the docker compose to test it more of an integration test.

Sounds good, thanks!

[infowolfe]

@JacobBrownAustin any movement on this?

[JacobBrownAustin]

@infowolfe
Sorry, I had to fix some bugs in a different project last couple of weeks, but I'm working on this again now.

[JacobBrownAustin]

@colinmollenhour @infowolfe
Okay, I've updated my pull request.

It now replaces the previously @deprecated Credis_Cluster class.

I'm running the same tests as CredisTest, but I've had to skip 25 of them due to incompatibilities, known issues, and some bugs I've found with RedisCluster. A lot of them are due to the fact that some of the methods require an additional parameter to specify which node in the cluster to run on like flushDb and save. We could write adapters for this methods to get the most recent cluster information and then run the same command on all nodes of the cluster, like flushDb for example; but this is beyond the scope of my project as I'm not currently needing any of these methods.
I've added PHP 8.4 testing container, in addition to the existing one which is PHP 7.3.
I've noticed that this project still says it supports PHP 5.4, but there's no container that is actually testing this, so I have no idea if that actually works on PHP 5.4.

I've updated the documentation for this new class.

And yeah, I added an Exception thrown in the constructor if the RedisCluster class from the 'redis' extension is missing.

[colinmollenhour]

We can drop PHP 5.4, I think 7.4 should be the lowest to support. I'll try to take a closer look soon. @infowolfe if you have a chance to do some code review and test that would help a lot as well!