Feature error handler #23

wants to merge 29 commits into

1 participant


I have moved all the work I did for setting an error handler onto a separate branch. This pull request includes all the commits. It replaces #19.

It extends the original pull request by allowing the error handler to be called from all functions, not just the non-multi functions. It is further called for connection errors as well.

theintz added some commits Feb 27, 2014
@theintz theintz added test case 27 for testing error handlers b17c2aa
…ts, added phpiredis_set_error_handler() function with copied code
@theintz theintz added error callback to _phpiredis_connection struct 482cb39
@theintz theintz use a proper callback in the test 5ee3e9c
@theintz theintz expanded test, checks for function present and does a faulty operation e7751e1
@theintz theintz proper function declaration for free_error_handler(), use the right m…
…acros for accessing arguments
@theintz theintz removed superfluous returns d6c4be9
@theintz theintz removed TODO from phpiredis_set_error_handler() 5ffb663
@theintz theintz call error handler if it is set 0274f21
@theintz theintz expanded test to check the *_bs functions as well ae60f14
@theintz theintz extend test with checks for some error conditions 057220a
@theintz theintz tabs to spaces fb1b28f
@theintz theintz refactored error handler calling code into separate function 184cf6b
@theintz theintz free error callback when destroying the connection 6ae98d6
@theintz theintz need to include the thread safety parameters be0f219
@theintz theintz removed merge error marks....... again bfb4de1
@theintz theintz modified signature of handle_error_callback() to accept a string inst…
…ead of an redisReply
@theintz theintz call error handler for connection errors also 3965dbb
@theintz theintz refactored callback calling again to reduce some duplicate code 18c2183
@theintz theintz set an empty error handler in test 008 c175422
@theintz theintz added test case for corrct error handling with dead connections using…
… QUIT command
@theintz theintz set an empty error handler in test 009 ee28f42
@theintz theintz added test case 029 for testing error handler invocation when using m…
…ulti commands
@theintz theintz call error handler in multi functions as well 892ada1
@theintz theintz added test for proper invocation of error handler when using multi me…
…thods on connection errors
@theintz theintz fix segfault which occurred when an error handler was set and unset a…
…gain (which was done automatically when setting a new error handler) and then called from outside.

the reason why this happened is that the actual php object was unset unintentionally. the commit also contains a test to check for this behavior. i am unsure if the implementation is 100% correct, there may be a memory leak.
@seppo0010 seppo0010 commented on the diff May 12, 2014
+ ZVAL_LONG(arg[0], type);
+ MAKE_STD_ZVAL(arg[1]);
+ // only set second argument when msg is given
+ if (msg != NULL && len > 0) {
+ ZVAL_STRINGL(arg[1], msg, len, 1);
+ }
+ MAKE_STD_ZVAL(return_value);
+ if (call_user_function(EG(function_table), NULL, ((callback*) connection->error_callback)->function, return_value, 2, arg TSRMLS_CC) == FAILURE) {
+ // return FAILURE to signal something went wrong with the error handler
+ retval = FAILURE;
+ }
+ // TODO: we could also return whatever the error handler returned to allow for more flexibility
seppo0010 added a line comment May 12, 2014

Isn't this relatively easy to address?

theintz added a line comment May 14, 2014

@seppo0010 yes, it would be a simple change, but I didn't see a use case for that. we could implement it, if you think it's useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nrk nrk referenced this pull request Sep 3, 2014

Tag a release #29

@theintz theintz closed this Mar 26, 2015
@theintz theintz deleted the theintz:feature-error-handler branch Mar 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment