(bc) problem with 4.0 redis

[jroszkiewicz]

PHP Redis 4.0 was released 17-03-2018 and.. we have some bc:

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

Proposition:

• release new major version for 4.0 php redis
• in current major version fix composer.json and lock to 3.1.x php redis

[curry684]

lock to 3.1.x php redis

That won't work as the bundle doesn't depend on phpredis, since it can also work with predis.

The true solution, which is B/C compatible, is to invoke a different client based on which version is installed.

[curry684]

Actually https://ocramius.github.io/ProxyManager/docs/access-interceptor-value-holder.html might do exactly what is wanted here, and it'd be both backwards and future compatible.

[curry684]

@snc honest and blunt question as I currently have 4 projects failing CI and going to fail to deploy soon-ish because of this issue: are you still maintaining this project?

I'm happy to join in and get stuff fixed, but not if merging the PRs is going to take months, and given the amount of open issues and PRs, and your Github activity as a whole, I'm getting the impression of an abandoned project. Fine if that is the case, but in that case we have no other choice than to fork.

[jroszkiewicz]

@curry684 you can always force to install redis 3.1.x by pecl, buts it's a temporary solution.
However i think symfony/cache is better option. And of course more trusted ;-)

[curry684]

symfony/cache is a caching solution, not a Redis encapsulation. I have projects using hashmaps, pubsub, ordered lists and everything else Redis offers.

Thanks for the PECL hint, I completely overlooked that option as I didn't expect it to be possible with the stock Docker containers, but it is and it will fix some of my CI issues for now hehe.

[Seldaek]

@curry684 If you send a reasonable PR I'll try and get it in, I have write access here because I remember nagging @snc too once upon a time. I think he's too busy overall :)

[curry684]

@Seldaek cheers, I think #405 should be the quick fix. If logging is disabled the wrapper is not used, so I disabled it by default if 4.0.0+ is installed, and issued a warning if explicitly enabled.

By all means do review, I don't have an easy setup to test it properly so all I know for sure is that it doesn't break tests.

[curry684]

Well it didn't break tests on my 3.1.6 install, now also fixed tests with 4.0 installed hehe.

[quentin-st]

@Seldaek Could you tag a release please? :-)

[Seldaek]

@curry684 should get a 2.1.0 out over the weekend or early next week. Feel free to require 2.1-dev in the meantime.

[curry684]

Up for grabs @ https://github.com/snc/SncRedisBundle/releases/tag/2.1.0

[yellow1912]

I ran into the same issue, here is my updated Client.php for the new Redis class, hope it helps:

https://gist.github.com/yellow1912/82acee8afd9e5b83826d35a8e4ff8584

[yellow1912]

I think we can release a new minor version for this, we can create a file named Client4.php for example? I'm not sure. The developer will have to specify in the config which client to use so it won't break with the older code. The only issue is that anywhere this Class is referred to we may have to update the use path.

[curry684]

We can actually detect the phpredis version during container compilation and select the right wrapper there, no manual work needed.

However I'm hesitant to take on the technical debt of adopting this file, as it means we'll be fixing up stuff and have a Client4.3.5.php in the end as Redis upstream keeps adding/changing/removing parameters and functions - if the base class doesn't exactly match compilation fails. I think we really need to generate the proxy class based on RTTI for this to become future proof.

[yellow1912]

@curry684 How would that work if you have to extend the base Redis File? Or is it necessary to extend that file?

[curry684]

You have to extend it so we can still keep passing the \Redis instances around that all other libraries and native code expect. The only thing the wrapper does right now is time calls and log them.

So the only way to both "be" a \Redis instance and have that behavior is to extend the class, and in PHP7 that means every function signature has to exactly match the overridden function literally. So the only way to make that work is to use RTTI to inspect the current \Redis instance, and regenerate all the methods like class RedisWrapper201804111305 extends \Redis { .... }.

At least, I don't see other ways hehe. Good thing is, that's pretty well doable - the Ocramius proxy manager does almost what we need, and Symfony and Composer do comparable things in generating containers and autoloaders.

[curry684]

I'll reopen this btw for the discussion as right now we only have a workaround really.

[yellow1912]

I think that can work, I guess with that the nice thing is that you do not have to do anything when they update the code on phpredis. I will play around with it abit today and see what I can do.

[yellow1912]

@curry684
Okie, I'm writing some minor test code for this, several things come to mind:

1. Is there a need to generate a random class like RedisWrapper201804111305, can we just simply generate the code and write it directly to our Phpredis/Client.php
2. The code to generate this code should be put somewhere that is run before any other SNCRedisBundle code, I'm thinking of putting that directly in SncRedisBundle.php?

Edit:

After further checking, it seems like we can have 2 approaches:

1. Use the SmartReference Proxy (https://ocramius.github.io/ProxyManager/docs/access-interceptor-scope-localizer.html). With this, it's possible to intercept any method call the the base Redis object and do whatever we want (in this case, you can trigger logging or anything)
   Limitation: you initialize this proxy using a real object, something like this:

$proxy = $factory->createProxy(
    new \Redis(),
    ['doFoo' => function () { echo "PreFoo!\n"; }],
    ['doFoo' => function () { echo "PostFoo!\n"; }]
);

2. Generate real wrapper classes for the corresponding Redis version, I don't think we need ProxyManager in this case

Am I missing something?

[curry684]

Is there a need to generate a random class like RedisWrapper201804111305, can we just simply generate the code and write it directly to our Phpredis/Client.php

We definitely should not overwrite vendor code at runtime (in a good environment the vendor folder should even be read-only). We should generate it in %kernel.cache_dir% similar to how Doctrine generate its proxies.

The random class name isn't terribly important but we would need something like that to avoid breaking production in case someone runs apt-get upgrade php-redis as we wouldn't be recompiling the container. This is actually a tough nut to crack well as it raises the question how we detect the change anyway.

The code to generate this code should be put somewhere that isrun before any other SNCRedisBundle code, I'm thinking of putting that directly in SncRedisBundle.php?

It should be generated during container compilation, so either in a compiler pass or the extension. Haven't looked at Doctrine yet how and when they generate the proxies (process should be similar).

The SmartReference proxy almost does what we want, but not entirely as it a) requires wrapping all functions separately and b) needs you to put all available functions in the arrays, thereby still requiring you to enumerate the functions by reflection. If you're doing that anyway it's not that much more effort to generically generate all of it. Which would bring my intended solution to suggestion 2.

If you're uncomfortable about the complexity of the required code don't worry, I can make time for it but all help is of course welcome, just wouldn't want you to waste a lot of time on a solution that's not going to work.

[yellow1912]

Then I think perhaps this will work:

https://github.com/nette/php-generator

If you place it in the CompilerPass, then you need to make sure to delete the cache after doing server software (redis) upgrade (and you cannot use the symfony cli command to delete I think, in this very specific situation). Another option is to use the Bundle boot method, which is always run I think.

For the file name, it may make sense to generate name that match the exact Redis version if possible, that way we can check if that file already exists and skip it.

There will be a few changes in the configuration I believe, because right now you allow specifying the class in the configs, with the file being auto-generated I wonder how that will work.

PS:
It would be cool if you can take over it or at least put the first code for it then I can follow your direction, otherwise I may do something that is not what you want and in the end the PR will not be accepted.

[curry684]

The generator is imo overkill. We're building a bundle here used in thousands of production applications that we'd all be burdening with the extra dependency, while essentially we need to generate only rather simple straightforward code we can also do with some nowdocs.

I've looked into how Doctrine does all this yesterday and it matches up with what I was planning, also it interferes with some other running issues about detecting Redis failure and improving the logging process (I want to make that event based so it's future proof). I think the total change is going to be rather big if taking all that into account.

[yellow1912]

In that case, perhaps it may make sense to deploy a hotfix to make it work with the current version and 4.0 version version instead? I imagine that the change you are talking about will need time for consideration and tests?

I have a working version of the Client.php that works fine with 4.0, perhaps we can push in there in the bundle as something like Client4.php for people who need it while waiting for a proper, official fix.

[idetia]

This is a workaround to fix it:

In your config.yml:

snc_redis:
    clients:
        default:
            logging: false

[curry684]

@idetia that's actually the "fix" we released in 2.1.0, we default that setting to false if running with phpredis 4.0+. So you should just run composer update snc/redis-bundle. If you explicitly specify true a warning will be issued that the setting was overridden because of incompatibility.

[dominikzogg]

@oerdnj thats the reason why i ask for the old package within ppa again on twitter

[curry684]

@dominikzogg a new version of the bundle is already available that circumvents the compatibility issues, so you can also just composer update to grab 2.1.2.

[dominikzogg]

@curry684 but we sill need?

snc_redis:
    clients:
        default:
            logging: false

[curry684]

Nope.

[yellow1912]

@curry684 Would you mind sharing the status of this issue? I'm interested in helping to somehow solve it even if it's just a short term solution.

[curry684]

I'm pretty swamped in "real work" right now. Hoping to have a short holiday soon where I can actually do some work on getting that out of my head into git 😉

[MGatou]

I ran into the same issue. I install "snc/redis-bundle": "3.x-dev" and run redis server v4 but it doesn't solve the issue.

[phansys]

IMHO, If the extension respects BC, I think we can expect that the existing methods are not modified between minor or patch versions.
Given that premise, I think we could ship a static class by supported version, using aliasing to keep the Client class available in a transparent way:

namespace Snc\RedisBundle\Client\Phpredis;

# Client/Phpredis/Client.php
$version = phpversion('redis');
if (version_compare($version, '4.0.0') >= 0) {
    class_alias(ClientRedis4::class, 'Client');
} elseif (version_compare($version, '3.0.0') >= 0) {
    class_alias(ClientRedis3::class, 'Client');
}

If there is some penalty for calling phpversion() and version_compare() at runtime, I think we should provide a task responsible for this in a Composer hook (post-package-install, post-package-update by instance).
I know this approach leaves out any new method that could exist in the local environment if the available Redis version is newer than the shipped by the bundle, but I think this at least a step to solve the main issue.
Regarding the concern about the extension update after the dependency resolution, I think that is out of the scope of this package: dependency resolution must be made before compiling the app in order to guarantee that everything is compatible. So, running an extension update after resolving dependencies is something that never should be contemplated by a sane environment. The same ugly scenario can explode if the PHP package itself is updated.

[B-Galati]

@phansys

We could do something like this but more simple by doing the class swap in the bundle extension.

We could support last minor version of each major version for method compatibility so that it would not be such a big deal to maintain. Only 2 classes to maintain like you proposed.

To manage that effectively phpredis minimum version could be checked by the bundle extension so that we could let user knows as soon as possible that their phpredis version is not compatible anymore with the bundle.

If there is some penalty for calling phpversion() and version_compare() at runtime, I think we should provide a task responsible for this in a Composer hook (post-package-install, post-package-update by instance).

No overhead if it's done in the bundle extension.

I know this approach leaves out any new method that could exist in the local environment if the available Redis version is newer than the shipped by the bundle, but I think this at least a step to solve the main issue.
Regarding the concern about the extension update after the dependency resolution, I think that is out of the scope of this package: dependency resolution must be made before compiling the app in order to guarantee that everything is compatible. So, running an extension update after resolving dependencies is something that never should be contemplated by a sane environment. The same ugly scenario can explode if the PHP package itself is updated.

Sadly, there is no silver bullet if we want to support multiple libraries without requiring them :/

[phansys]

That's fine for me. I can work on a PR based on that approach, but I don't know where can I get the Redis class for each extension version. Is there a builder or something similar which we can trust? IMO, that build process should be embedded at CI build in order to ensure the shipped version is always up to date.

[yellow1912]

Anyone is picking this up? I'm seriously considering doing something about this. We must have better support for Phpredis, the code should detect the current Phpredis and try to switch to that class.

I have several versions of PHP running across several projects and this is causing big issues.

[B-Galati]

@yellow1912 You can give it a try indeed 👍

[ostrolucky]

I don't think this issue is valid anymore. Signatures were updated in #507 so it's compatible now with newer redis versions.

[ostrolucky]

Now logging also works thanks to #618, so closing here