Extending StreamingClient without rewriting Factory

[dmarkic]

Hello!

How would one extend StreamingClient and use extended class without rewriting the whole Factory class?

It seems to be easy to implement the Factory in such way that it would create a custom (extended) StreamingClient by just providing the class to use (https://github.com/clue/reactphp-redis/blob/master/src/Factory.php#L69) somewhere in the Factory.

Maybe something like:

$factory->setClientClass(...);

Which would override the default Clue\React\Redis\StreamingClient.

That way we can easily use Factory and build upon (extend) StreamingClient. It just seems such a waste to rewrite (and maintain) Factory class just to change the returned StreamingClient object.

I can make PR for this if the idea is feasible.

Thanks for great software!

Cheers,
Dejan

[clue]

@dmarkic Thanks for bringing up this idea. I'm not sure I follow what exactly you're trying to achieve with this method, perhaps you can elaborate on what you're trying to build with this hypothetical API? Why do you need a custom Client implementation and why do you think the existing Factory still provides value for this use case?

[dmarkic]

@clue For instance, I want hgetall() method to return [key => value, ...] array, where now it returns (as Redis) [key,value,key,value,...] array. It would be easy if I could extend StreamingClient (Factory would use my extended class) and tap into hgetall() call.

Hope this makes sense.

Kind regards,
Dejan

[clue]

@dmarkic I understand where you're coming from and agree that there's value in having native assoc array types (see #17 and others).

Unfortunately, the suggested API would break the LSP (the L in SOLID) as your class would provide a different API than the interface.

With Redis 6 as discussed in #98, the client will have native assoc array support anyway, so this will very likely introduce a BC break anyway.

As such, I'm not sure how much value this suggest change would bring in the meantime.

[dmarkic]

@clue Don't know if we understand each other. The Factory would still enforce that the returned object would be a subclass of StreamingClient so no Api would be changed.
Another use case would be that I would for instance like to log every StreamingClient::__call(), or some other StreamingClient method.

So I would just like to extend StreamingClient and make it possible for Factory to return that extended class.

Kind regards,
Dejan

[clue]

For instance, I want hgetall() method to return [key => value, ...] array, where now it returns (as Redis) [key,value,key,value,...] array. It would be easy if I could extend StreamingClient (Factory would use my extended class) and tap into hgetall() call.
…
The Factory would still enforce that the returned object would be a subclass of StreamingClient so no Api would be changed.

I think the suggested changes make perfect sense and we're looking into updating the API to use more idiomatic values in the upcoming version as discussed in #17 and #98. However, because it currently returns a list of [key, value, key, value, …] and will return an assoc array [key=>value, key=>value, …] in the future, this is a BC break. Accordingly, I've set this for the upcoming v3.0 release milestone.

Other than that, I'll assume this is resolved and will close this for now, please feel free to report back otherwise 👍