Hello, thanks for the PR. Here is the options reference:
https://github.com/nrk/predis/wiki/Client-Options

To understand better, which options are useful in your case?
E.g. "profile" is not needed in the context of a cache pool.
For "cluster", is this the canonical way to connect to a cluster? From the doc "redis" is not a legal value, unless I missed something. I don't know predis well so I'd be happy to have more details. Thanks.

Hi@nicolas-grekas,

No problem, I'd be happy to provide some more info! The predis docs are a little off in the wiki. The readme.md specifies there are 3 options, the one mentioned in the wiki, plus eitherredis orpredis. Theredis option specifies that predis should let the redis servers manage the clustering themselves (for clustered redis). If this is set aspredis or a callable, this is for client based redis clustering where you just give it a bunch of non-connected redis servers the client handles the sharding/clustering/replication and so on.

See:https://github.com/nrk/predis#client-configuration
See:https://redis.io/topics/cluster-tutorial

Theprofile option was only included as an example of passing multiple options to the client.

Thanks for the links.

Right now, Predis is skipped when ext-redis is installed.
When one setspredis_options, I suppose one expects Predis to be used.
I'd suggest to throw an exception when that's not the case.

Do you reckon it would be better to throw an exception in that case, or to enforce the use of predis instead? This could be could be done expanding uppon the if statement that checks for the existence of ext-redis. It could check for ext-redis as well as predis options and default to predis if they are present.

It's possible someone could have ext-redis available, but would prefer to use predis for the clustering capability. But still use ext-redis for other redis instances that are not clustered. Allowing thepredis_options to force predis could work out better in this use case.

Setting theclass option toRedisCluster is another way to connect to a cluster directly with the extension.
About your question, I'm not sure. I think I would prefer automatic configuration: when a predis option is used, we could forceclass accordingly, and fail with a clear error message when the package is not installed. WDYT ?

I haven't got much experience with the redis extension, does theRedisCluster class extend theRedis class?

I agree, I think having the code work out what you're trying to use and fail on an invalid configuration setup is ideal.

I've updated the code to reflect the more automated configuration approach as well as added tests to check this config.

If a predis option is specified and predis is not available it will throw aCacheException. If a predis option is specified and a custom class is specified, it will check that the custom class is an instance of\Predis\Client and throw anInvalidArgumentException if not. What are your thoughts on this?

Great, I've made those suggested changes now and cleaned up a few of the exception messages a bit!

I've made the suggested changes in the code and theCHANGELOG.md file now.

Was the line doing$predisOptions = $params['predis_options'] ?? array(); something you wanted changed as well, or is that part ok?

I'm not sure what's caused the tests to fail for travic-ci there. Any ideas? Seems to be something to do with the Process component?

@fabpot Were the changes made suitable for what you mentioned?

See proposal in#28300 (comment)

Having done some more debugging on this, I do not believe my merge is yet complete so please do not accept it yet.

There is an issue where passing in a single host to predis causes it to ignore the clustering setup completely, using the default stream connection instead of creating a cluster. (For example, aws elasticache redis clusters that have a single entry endpoint.)

I'm not yet quite sure on how to approach this, possibly if clustering is expected, to pass in an array of hosts (an array of 1 host works), instead of a single string to the factory.

It also appears the predis factory does not support creating a redis cluster connection.

The current tests pass only because when calling the code to get thecluster option actually causes the creation of a cluster connection, giving a false positive. When passing in a dsn as a single string and not an array of dsn's, redis clustering is never used.

I think this changes the scope of changes required to theRedisTrait to get clustering to work, it may be that#28300 provides a better solution to go with overall instead of my attempt here.

Closing in favor of#28713
Please review/test this PR and see if this works for you.
It'd be awesome if you could contribute some doc for it :)