SncRedisBundle: 2.1.5 (lazy loading) somehow breaks set method on session client

Sounds reasonable, we can swap it in and out in a compiler pass. That way we could disable it if the Phpredis version is fixed, for example. 👍

Thanks @nicolas-grekas and @dkarlovi! I will try to do something in coming weeks / months.

Here’s some additional intel on the matter of the lazy loading problem:

Phpredis was recently upped to 4.2.0. A change in 4.2.0RC2 introduces correct reflection for the Redis::multi() method - one optional argument. The problem arises from the fact that reflection can not retrieve default values of an optional argument if the method is internal, and lazy loading creates a virtual proxy class with information retrieved via reflection. That means the multi() method in proxy is defined as:

public function multi($mode = null)

whereas the real default value for $mode is Redis::MULTI.

This means when the service is lazy loaded, and we use the method without arguments, the actual method is called like multi(null) (instead of without arguments, or with the correct default value for the argument).

I understand we can create our own stub and (re)introduce default values in a user defined class which would result in a perfect reflection info and in turn a 1:1 proxy class. But, there are many versions of Phpredis to support. How to handle many cases? Whose responsibility would it be to write a correct stub for (every) Phpredis release? I assume there would be a way to have this bundle use the correct stub, based on ext-redis version, but this seems like a heavy undertaking with synchronicity on both sides.

Further, with older versions of Phpredis (ie. 4.0.2) this particular method did not have any ARGINFO present so based on the reflection information, the optional argument was never forwarded from the proxy class to the real class. One could literally call the method like multi(new \DateTime()) and the actual method would still recieve the call without arguments, thus (unintentionally) using its own default.

This is a general issue with proxies for internal classes, maybe Symfony should not create proxies for them at all? Even if everything is done right on the side of Phpredis (which is not, but let’s assume) - still there is the fact that reflection does not have the ability to get default values for optional arguments of internal methods. This change would render your lazy loading technique futile though.

All this said, I think the most immediate solution would be to add a lazy parameter to client configurations… this was already proposed but somehow dismissed. I hope I’ve given enough more insight why this should actually be implemented.

Partially, this is discussed in phpredis/phpredis#1476 too.

@ondrejfuhrer No I don’t think so because Predis client is lazy by default: it does not try to connect to redis unless something is asked. With PHPRedis you have two method to connect: pconnect and connect. To support both we call the choosen one on service creation, thus it tries to connect to redis. To avoid useless connection we added laziness to the service itself. It’s the easiest solution. So IMHO, it would be useless to add laziness to Predis client. Consistency is not a strong argument in that case.

@ondrejfuhrer Thank you

I think we could do two things:

• Add an option to opt-out from lazy loading to let one do as he likes
• Do what is mentioned in this comment https://github.com/snc/SncRedisBundle/issues/442#issuecomment-409152553

Nice, I was wondering why the application cashes when updated a minor version of this bundle 😄 Just an idea: is there a particular reason why the definition is forced as lazy? When I updated that it worked fine also with older phpredis version.

So maybe having the option to disable the lazy loading of the client via the configuration would fix that regardless the phpredis version?

Most likely typical PHP extension fuckery.

In PHP extensions, it is possible to have an optional parameter that also considers the missing parameter NULL, even if the parameter is not nullable. This means that a PHP userland signature for this cannot exist.

Example:

function foo(array $bar = []){}

In PHP core, you can decide that foo() is equivalent to foo(null), even though such a call would be illegal in userland (it would be foo([]) for us mere mortals).

The other possibility is that the method is completely mis-documented, and you will need to experiment with combinations of allowed parameters.

By writing it? O_o