Add on demand phpredis connections

[borisbabic]

This extends the wrapper to allow connections to be seamlessly created only when necessary.

[curry684]

I'm sorry but this PR is currently by definition broken, as it depends on the connection wrapper class which is fundamentally broken right now (#399).

Also I'm not sure I see the use case for the ton of complexity this PR is adding. The connection itself is already lazily instantiated at the Symfony service level, so afaics there would only be a difference if you actually inject the connection somewhere and not actually use it. In all other cases it would at best be a micro-optimization, at worst a ton of tech debt waiting to become a problem for no discernible performance win.

Could you perhaps elaborate on the use case and the real world problem you intend to solve?

[borisbabic]

I'm sorry but this PR is currently by definition broken, as it depends on the connection wrapper class which is fundamentally broken right now (#399).

I wasn't aware of that issue, that's very problematic

Also I'm not sure I see the use case for the ton of complexity this PR is adding. The connection itself is already lazily instantiated at the Symfony service level, so afaics there would only be a difference if you actually inject the connection somewhere and not actually use it. In all other cases it would at best be a micro-optimization, at worst a ton of tech debt waiting to become a problem for no discernible performance win.

Could you perhaps elaborate on the use case and the real world problem you intend to solve?

This wasn't really for performance but rather error handling. It came up at work where we want to make a critical monolithic service more robust. We have many situations where we can continue on if there are any redis related issues. If the client is injected as is we don't get any chance to handle connection errors and continue depending on what was attempted.

The following is a simplistic example of what we'd like to do:

<?php

class Cache
{
    private $redis;

    public function __construct(\Redis $redis)
    {
        $this->redis = $redis;
    }

    public function get($key)
    {
        try {
            return $this->redis->get($key);
        } catch (\Throwable $_) {
            // if there are any redis issues, including connection issues, we want to continue
            return null;
        }
    }
    
    public function set($key, $value)
    {
        // we don't want to allow the old value to silently remain so we want an error here
        $this->redis->set($key, $value);
    }
}

[curry684]

So your issues are essentially similar to #105 and #253 ?

[borisbabic]

Yes, it's similar. Basically we also want to somehow handle redis connection issues.

To me there seems to be a difference in the approach; we want to have the ability to decide how to handle issues ourselves rather than having the bundle do it.

[curry684]

For the time being I'm highly hesitant to merge this PR as it's really invasive, a highly specific implementation for your case, and currently downright broken 😉

However what you may not be aware of is that you can implement this yourself - the bundle config allows you to override the wrapper class yourself (https://github.com/snc/SncRedisBundle/blob/master/DependencyInjection/Configuration/Configuration.php#L53). This should allow you to implement your solution locally without effort while we implement a more generic solution in v3 of the bundle, which will also be fully phpredis 4 compatible out of the box then.

[borisbabic]

This PR actually started as an override to the wrapper class. At first I wanted to just extend the wrapper but I had to resort to basicaly copy/pasting it and in the end it seemed better to make this configurable and integrate it upstream.

I don't think the invasiveness is that big of an issue. The really invasive part is opt in and, IMHO, can be sufficiently documented.

I also don't think it's actually too specific. Those involved in the other issues may have wanted to solve their problems differently but it seems to me that they could have used this to solve their issue.

However I do understand not merging this because of #399, the wrapper seems broken.

If this won't be merged would you be open to making it possible to extend the wrapper with this functionality? Duplicating all the redis methods in a new class in our project seems like a headache waiting to happen.

[curry684]

I'll keep it around for reference when we get around to structurally fixing #399 and after that investigating how best to approach #105 and #253. I don't really foresee yet which route we'll take on this in the end.

[B-Galati]

Perhaps this can be solved by using Symfony lazy services : https://symfony.com/doc/current/service_container/lazy_services.html

[curry684]

Closing in favor of #440