Update method signatures in Client in order to respect its parent

[phansys]

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

[phansys]

BTW, is there any reason for not using __call() magic method instead of rewrite every parent method? https://3v4l.org/NXUcf

[B-Galati]

@phansys I have no idea ^^

[phansys]

It's fine for you if I implement __call(), removing all the proxied methods? I think it is more scalable since we can forget about the responsibility to keep track for the changes on Redis class.

[B-Galati]

In fact I guess it's because Client class inherits from \Redis so __call would never be called. It is called only on not accessible method.

__call() is triggered when invoking inaccessible methods in an object context.

Perhaps all of these is done on purpose, not sure.

Perhaps @snc or @curry684 @Seldaek know?

[B-Galati]

@phansys I think it would not work but you can try and let me know.

Another option would require not to extend from \Redis and but that is not ok as it would be a major BC break.

[phansys]

You're right, I was omitting that the methods on Redis are accessible. I don't have a better idea then.

[B-Galati]

Ok for me but I would like another feedback.

[curry684]

Yes, __call could not work since it is only invoked for unknown methods. The inheritance model is a compromise between being a drop in replacement and still being able to "hook" into all methods for logging purposes.

The fundamental problem with the approach has caused severe issues the past few months because the inheritance model breaks when function signatures do no match up exactly. We should actually, to achieve the same effect, generate the proxy at runtime based on the actual RTTI from the \Redis class exposed by the module, but that is quite.... 'tricky' to implement, especially given that PHP was apparently bugged in exposing that info right after the phpRedis 4 release.

[curry684]

#399 is the tracker issue for the proxy approach and may contain some useful insights.

[curry684]

Out of curiosity - what issue is this PR trying to solve?

Unless we're really fixing something I'm 👎 on this one as I've just benchmarked my suspicion that using the inline constants is actually about 10% faster than using __FUNCTION__.

[phansys]

The issue which triggered this PR is exposed in the build of #505:

Declaration of Snc\RedisBundle\Client\Phpredis\Client::append() should be compatible with Redis::append($key, $value)

The usage of __FUNCTION__ is just laziness on my side. Given your benchmark, I can change them by the proper method names.

[curry684]

Ah I see, in that case I'm fine with it like this 👍

[B-Galati]

@curry684 Thanks for your enlightenment!

What do you use to benchmark that case? I am really interested :-)