Error handling

[nrk]

Error handling

[combatpoodle]

This would be great for us too. The first option is most standard, and for backwards compatibility could be enabled by setting a constant or specifying the desired behavior during initialization.

[nrk]

I like both 1 and 3 but I'd be more inclined to favor 3 since it's the same approach we used for phpiredis_reader. Currently the client and the reader resources are completely separate and don't share any kind of interaction, this is due to the fact that the reader bits were implemented at a later stage to make this library useful for other pure-PHP libraries such as Predis to optionally speed things up a bit with protocol parsing and serialization.

The advantage of 3 is that users can customize the behavior of the client simply by supplying a callable object to make it raise a PHP error (like it happens currently when using the client), return a custom error object (like it happens with Predis that uses only the reader resource) or simply throw an exception (which is the point of the current discussion). We could also make the client raise a PHP error by default to retain the current behavior when there's no custom handler specified, but maybe we should just aim to introduce breaking changes altogether (see later in my comment). I'm not sure about having a distinction between global or per-connection handlers, I'd go only with the second but maybe I'm missing a use case that would make the global ones useful.

But while this is cool to handle errors returned by Redis, it doesn't solve our problem with plain connection errors (e.g. the server goes down while doing stuff)...

Slightly related: I was meaning to finally freeze the library and tag a 1.0.0 release to start making changes (even breaking ones) targeting a new major release sometime in the future. The change described in your issue would be a good start in this direction, and I have some more ideas that I was planning to share in a issue open for discussion. Unfortunately my C skills are not something to be proud of and also a bit rusty since the last time I used it extensively was in high-school (quite a while ago :-)) so literally any kind of help and contribution would be more than appreciated.

[seppo0010]

Hi,

I'm with some free time at the moment, so I can take a stab at this, or review @theintz's implementation but I agree on deciding an interface first.

About implementations, I prefer option 3, adding a two new optional arguments to phpiredis_{multi_}command{,_bs} that would be called on error and on status and return whatever those functions return.

The phpiredis_reader interface might also be used instead and only use one extra new optional argument, but I think most of the users won't prefer to initialize a reader for these functions.

[theintz]

So the debate swings in favor of setting an error handler. However, instead of passing the handler(s) each time a phpiredis function is called, I would favor adding a function phpiredis_set_error_handler(), which attaches a custom error handler to a connection. This error handler could also be used when a connection error happens. Maybe we could define a few constants that represent the type of error: PHPIREDIS_CONNECTION_ERROR and PHPIREDIS_PROTOCOL_ERROR, for example.

[nrk]

@theintz I agree with you and that's what I was referring to when I said that we should use per-connection handlers.

I like @seppo0010's suggestion of using the same resource returned by phpiredis_reader_create() as an optional argument to set custom status and error handlers used to process Redis responses, but instead of passing it each time a function is called I'd pass it only once when we create the connection. The function would look something like this:

phpiredis_connect($host, $port, $reader = null);

As for connection or protocol parsing errors we can indeed add a new function with a signature like phpiredis_set_error_handler($connection, $handler) where $handler is a callable with a signature like function($message, $type) (and where $type is populated with the values of PHPIREDIS_CONNECTION_ERROR or PHPIREDIS_PROTOCOL_ERROR depending on the error)

[seppo0010]

What I don't know is how that works considering the connection might be persistent.

[theintz]

@seppo0010 For all PHP knows, there are no persistent connections. For every request, the connection must be re-initialized by calling phpiredis_pconnect() anyways (in order to get the connection handle), so it might as well be non-persistent (connections are however cached/persisted inside the extension code). The error handler then has to be set on the connection for every request as well.

I am going to get started on adding the function and the constants we talked about, I have to admit however that C is not my native language either. Much of the code for setting an error handler is already in the extension though, so it should not be very difficult. Will create a pull request when I have it working.

[nrk]

In the meanwhile I'm going to create a new v1.0 branch and tag v1.0.0 soon, the new developments regarding this issue will be eventually available in a new major release.