[WIP] Build custom Redis client based on the installed extension

[phansys]

Q	A
Branch	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#399
License	MIT
Doc PR	n/a

[phansys]

I'm trying to use the reflection API in order to introspect the \Redis class, but it seems to fail to get some method arguments. By instance, with pconnect() it doesn't find any argument. Do you know how could I get a valid representation of the class provided by the extension?
Thank you in advance.

[B-Galati]

Could be an issue similar to phpredis/phpredis#1374

[B-Galati]

See #442 if you want more details ;-)

[B-Galati]

I am not sure why it is necessary to have that in the CI.

IMHO, the class should be generated at cache warmup then dump into var/cache/snc-redis-bundle and then loaded thanks to a custom autoloader. It's basically what doctrine-bundle is doing for entities.

What do you think?

[phansys]

I think that could work only when using DI, but be aware that PhpredisClientFactory doesn't depend on the container.
Also, in my first approach I was thinking to check the generated file in each build in order to detect any change based on the installed Redis version, but I think it doesn't have sense now that the installed versions are set explicitly at Travis config.

[phansys]

Thank you for pointing me in the right direction. I've opened phpredis/phpredis#1526 for the described problem.

[B-Galati]

@phansys Given what was said in the issue, I think we could only support phpredis4 for this feature and have a fallback compatible with phpredis3 only. That's quite easy to do since v3 will not change anymore.
We could provide another one for phpredis2 but it's not important IMHO as it's quite an old version.

[phansys]

Sure. I'll try to work on the proposed fix today. Thank you.

[phansys]

Methods like echo() and eval() are present at Redis 3.1.6, but these are not allowed by PHP < 7.0. Should we remove them from the custom client based on that version or should we restrict the usage of the client only for PHP >= 7?
For BC reasons, I think the first option is more suitable for this case, but I'd like to hear your opinion.

[phansys]

By the other hand, Redis 4.3.0 seems to be not compatible with 4.2.0 (ref):

Declaration of Snc\RedisBundle\Client\Phpredis\Client4_3_0::psubscribe(array $patterns, $callback) should be compatible with Redis::psubscribe(array $patterns)

Should we ship a client by minor version?

[B-Galati]

By the other hand, Redis 4.3.0 seems to be not compatible with 4.2.0 (ref):

Declaration of Snc\RedisBundle\Client\Phpredis\Client4_3_0::psubscribe(array $patterns, $callback) should be compatible with Redis::psubscribe(array $patterns)

Should we ship a client by minor version?

For phpredis4 we could only generate the client class on cache warmup so that it would always fit with the installed phpredis version.

We would have static client class only for older phpredis version, (e.g.) v2 and v3.

Methods like echo() and eval() are present at Redis 3.1.6, but these are not allowed by PHP < 7.0. Should we remove them from the custom client based on that version or should we restrict the usage of the client only for PHP >= 7?
For BC reasons, I think the first option is more suitable for this case, but I'd like to hear your opinion.

For v3, the bundle only accepts PHP 7 so let's say we add this feature for V3 only. What do you think?
Which should be released in less than 2 months (I hope).

[phansys]

I think your proposal is doable. Regarding the release of v3.0.0, should this PR target master?

[B-Galati]

Yes master for upcoming V3.

[B-Galati]

Should we close this PR ?

[phansys]

Should we close this PR ?

Yes, closing since this fix will not be accepted as is for 2.x branch.