-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Error handling #18
Comments
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. |
I like both 1 and 3 but I'd be more inclined to favor 3 since it's the same approach we used for 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. |
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 The |
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 |
@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_connect($host, $port, $reader = null); As for connection or protocol parsing errors we can indeed add a new function with a signature like |
What I don't know is how that works considering the connection might be persistent. |
@seppo0010 For all PHP knows, there are no persistent connections. For every request, the connection must be re-initialized by calling 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. |
In the meanwhile I'm going to create a new |
We are using phpiredis on a high-traffic site and are quite impressed with its performance and ease of use. One thing thats missing though is proper error handling. Currently, when the server replies with an error message (for example when a command is issued against a key of the wrong type), phpiredis will raise a PHP error, which must be handled by the user application.
Unfortunately PHP does not offer any form of classification for errors, all errors must be handled by the same error handler function. This makes it very hard to properly encapsulate the Redis functionality.
We would like to handle the errors coming out of phpiredis separately from other errors that may be happening elsewhere. I can think of three possible strategies for that, all of which don't raise PHP errors:
phpiredis_last_error()
. This special return value could be eitherfalse
ornull
. As these values can also be valid return values, the user application, upon seeing the value, should call the error accessor and determine whether an error really occurred. (This is how phpredis does it)false
.phpiredis_reader
from PHP code, and a function to set an error handler on a reader also exists. I could however not find a way of tellingphpredis_command_bs()
for example, to use this specific reader. Is there a way of doing this? It seems like these readers are never used.I am willing to implement the option that is decided on, and provide it via a pull request. However, I wanted to first open an issue to provide the possibility for a discussion.
The text was updated successfully, but these errors were encountered: