# [RFC] Enable error_handler callback parameters to be passed by reference - Thread: https://externals.io/message/81193 - Original archive: https://news-web.php.net/php.internals/81193 - Started by Thomas Bley on 2015-01-27T02:45:21+00:00 - Messages: 20 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #81193 — Thomas Bley — 2015-01-27T02:45:21+00:00 URL: https://externals.io/message/81193#81193 Here is the rfc: https://wiki.php.net/rfc/error_handler_callback_parameters_passed_by_reference Thanks to reeze for coding and putting it on the wiki. Regards Thomas reeze wrote on 23.01.2015 10:59: > Yeah, seem other want it it too, so I just updated the PR to allow the first four parameters been passed by reference. > > > > > […] --- ## #81194 — Yasuo Ohgaki — 2015-01-27T03:08:56+00:00 URL: https://externals.io/message/81193#81194 · In reply to: #81193 Hi Thomas, > Here is the rfc: > > https://wiki.php.net/rfc/error_handler_callback_parameters_passed_by_reference > > Thanks to reeze for coding and putting it on the wiki. > It looks good to me. Future Scope `set_error_handler()` callback might be able to handle `E_ERROR` to be able to append additional information to memory_limit exhaustions (or others). I really want to catch E_RROR by user defined error handler. The only reason why this was disabled is abuse of error handler. We may document abuse of error handler with `E_ERROR` may result in undefined behavior including memory leak/crash and allow it. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #81235 — Xinchen Hui — 2015-01-27T12:26:58+00:00 URL: https://externals.io/message/81193#81235 · In reply to: #81194 Hey: > Hi Thomas, > > > > > […] > > It looks good to me. > > Future Scope > `set_error_handler()` callback might be able to handle `E_ERROR` to be able to > append additional information to memory_limit exhaustions (or others). > > I really want to catch E_RROR by user defined error handler. The only > reason why this was disabled is abuse of error handler. We may document > abuse of error handler with `E_ERROR` may result in undefined behavior > including memory leak/crash and allow it. I feel it's kindof wrong direction. if you want custom logs, maybe you should use custom logger. thanks > > Regards, > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net -- Xinchen Hui @Laruence http://www.laruence.com/ --- ## #81250 — Yasuo Ohgaki — 2015-01-27T18:09:46+00:00 URL: https://externals.io/message/81193#81250 · In reply to: #81235 Hi Xinhcen, > if you want custom logs, maybe you should use custom logger. It requires to catch all errors, I suppose. If PHP allows it, either would work. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #81252 — Yasuo Ohgaki — 2015-01-27T18:21:36+00:00 URL: https://externals.io/message/81193#81252 · In reply to: #81250 Hi all, > > > > […] > > > It requires to catch all errors, I suppose. If PHP allows it, either would > work. I considered a little more. With error handler that catches `E_ERROR` cannot be complex handler as complex handler may use various resources that may cause all kinds of errors. Therefore, complex custom error handler logger may miss to log important errors. Best practice would be separating app log and system(PHP) error log. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #81255 — Stanislav Malyshev — 2015-01-27T19:43:00+00:00 URL: https://externals.io/message/81193#81255 · In reply to: #81235 Hi! > I feel it's kindof wrong direction. > > if you want custom logs, maybe you should use custom logger. Same here, API basing on modifying something that is supposed to be internal engine things feels wrong, even if it does work. Changing error message is borderline, changing things like lines/filenames just feels wrong. Maybe better access to logging system for custom logging would be more efficient instead? -- Stas Malyshev smalyshev@gmail.com --- ## #81282 — Thomas Bley — 2015-01-28T05:42:53+00:00 URL: https://externals.io/message/81193#81282 · In reply to: #81255 Hi, the idea behind the rfc is not to change error messages, it's only to append useful information to them. Changing lines/filenames is surely something controversial and might be only useful when compiling configuration files or templates to php code. Therefore we have 3 options in the voting section. > Maybe better access to logging system for custom logging would be more > efficient instead? The logging system can be accessed with 1 line of code (trigger_error, throw new exception, etc), so from userland perspective I don't see any deficits here. In case someone needs multiple error handlers, it might be necessary to implement sth. like spl_set_error_handler (similar to spl_autoload_register), but that's not in the scope of this rfc. Regards Thomas Stanislav Malyshev wrote on 27.01.2015 20:43: > Hi! > > > […] > > Same here, API basing on modifying something that is supposed to be > internal engine things feels wrong, even if it does work. Changing error > message is borderline, changing things like lines/filenames just feels > wrong. > > Maybe better access to logging system for custom logging would be more > efficient instead? > > -- > Stas Malyshev > smalyshev@gmail.com --- ## #81283 — Thomas Bley — 2015-01-28T06:11:37+00:00 URL: https://externals.io/message/81193#81283 · In reply to: #81235 There are some workarounds with register_shutdown_function to extend `E_ERROR` messages, but I think that's quite dirty in a system with many parallel requests. Here is an example: 'edit'); function check_for_fatal() { $error = `error_get_last()`; if ($error['type'] == E_ERROR) { $errstr = ''; if (!empty($_SERVER['REQUEST_URI'])) { $errstr .= ' Request-Uri: '.$_SERVER['REQUEST_URI'].PHP_EOL; } $errstr .= ' Request-Params: '.json_encode($_REQUEST); file_put_contents('php_error.log', $errstr. PHP_EOL, FILE_APPEND); } } register_shutdown_function('check_for_fatal'); $str = str_repeat('##########', 2*1024*1024); gives: [28-Jan-2015 07:00:53 Europe/Berlin] PHP Fatal error: Allowed memory size of 16777216 bytes exhausted (tried to allocate 20971521 bytes) in test.php on line 27 Request-Uri: /custom/foobar Request-Params: {"action":"edit"} Regards Thomas Xinchen Hui wrote on 27.01.2015 13:26: > Hey: > > > > […] > I feel it's kindof wrong direction. > > if you want custom logs, maybe you should use custom logger. > > thanks > > […] > > > > -- > Xinchen Hui > @Laruence > http://www.laruence.com/ --- ## #81284 — Xinchen Hui — 2015-01-28T06:15:06+00:00 URL: https://externals.io/message/81193#81284 · In reply to: #81283 > There are some workarounds with register_shutdown_function to extend `E_ERROR` messages, but I think that's quite dirty in a system with many parallel requests. > Here is an example: > > ini_set('error_reporting', E_ALL); > ini_set('display_errors', 0); > ini_set('log_errors', 1); > ini_set('error_log', 'php_error.log'); > ini_set('date.timezone', 'Europe/Berlin'); > ini_set('memory_limit', '16M'); > > $_SERVER['HTTP_HOST'] = 'mywebsite.com'; > $_SERVER['REQUEST_URI'] = '/custom/foobar'; > $_REQUEST = array('action' => 'edit'); > > function check_for_fatal() { > $error = `error_get_last()`; > if ($error['type'] == E_ERROR) { > $errstr = ''; > if (!empty($_SERVER['REQUEST_URI'])) { > $errstr .= ' Request-Uri: '.$_SERVER['REQUEST_URI'].PHP_EOL; > } > $errstr .= ' Request-Params: '.json_encode($_REQUEST); > file_put_contents('php_error.log', $errstr. PHP_EOL, FILE_APPEND); > } > } > > register_shutdown_function('check_for_fatal'); > > $str = str_repeat('##########', 2*1024*1024); > > gives: > > [28-Jan-2015 07:00:53 Europe/Berlin] PHP Fatal error: Allowed memory size of 16777216 bytes exhausted (tried to allocate 20971521 bytes) in test.php on line 27 > Request-Uri: /custom/foobar > Request-Params: {"action":"edit"} actually, this should already be in access.log(the last 500 error one)...... I don't see why you need it in error log.... thanks > > > Regards > Thomas > > > > Xinchen Hui wrote on 27.01.2015 13:26: > > > […] > -- Xinchen Hui @Laruence http://www.laruence.com/ --- ## #81286 — Yasuo Ohgaki — 2015-01-28T06:36:49+00:00 URL: https://externals.io/message/81193#81286 · In reply to: #81284 Hi Xinchen, > actually, this should already be in access.log(the last 500 error > one)...... I don't see why you need it in error log.... > PHP log would be more useful with user ID/name to identify who was trying to tamper with system, for example. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #81311 — Andrea Faulds — 2015-01-28T16:53:42+00:00 URL: https://externals.io/message/81193#81311 · In reply to: #81193 Hi Thomas, > > > Here is the rfc: > https://wiki.php.net/rfc/error_handler_callback_parameters_passed_by_reference > > Thanks to reeze for coding and putting it on the wiki. > > Regards > Thomas This feels rather weird and “hacky” to me. Error handlers really shouldn’t be able to modify messages, it’s bad enough that they can already ignore errors or do other weird things with them. So, I’m -1 on this. Thanks. -- Andrea Faulds http://ajf.me/ --- ## #81313 — Thomas Bley — 2015-01-28T17:36:09+00:00 URL: https://externals.io/message/81193#81313 · In reply to: #81311 Hi, the idea behind the rfc is not to change error messages, it's only to append useful information to them. Regards Thomas Andrea Faulds wrote on 28.01.2015 17:53: > Hi Thomas, > > > […] > > This feels rather weird and “hacky” to me. Error handlers really > shouldn’t be able to modify messages, it’s bad enough that they can already > ignore errors or do other weird things with them. > > So, I’m -1 on this. > > Thanks. > -- > Andrea Faulds > http://ajf.me/ --- ## #81314 — Andrea Faulds — 2015-01-28T17:38:29+00:00 URL: https://externals.io/message/81193#81314 · In reply to: #81313 Hi Thomas, > > > the idea behind the rfc is not to change error messages, it's only to append useful information to them. That’s still changing the message. Error handlers are global anyway, so this is useless (or dangerous) for libraries. I don’t get it. -- Andrea Faulds http://ajf.me/ --- ## #81315 — Thomas Bley — 2015-01-28T17:50:23+00:00 URL: https://externals.io/message/81193#81315 · In reply to: #81314 > Error handlers are global anyway, so this is useless (or dangerous) for > libraries. Yes that's true, maybe set_error_handler can be changed so it behaves like register_shutdown_function or spl_autoload_register when multiple callbacks are registered? Regards Thomas Andrea Faulds wrote on 28.01.2015 18:38: > Hi Thomas, > > > […] > > That’s still changing the message. > > Error handlers are global anyway, so this is useless (or dangerous) for > libraries. > > I don’t get it. > > -- > Andrea Faulds > http://ajf.me/ --- ## #81319 — Thomas Bley — 2015-01-28T17:59:06+00:00 URL: https://externals.io/message/81193#81319 · In reply to: #81315 > > […] I'm not sure if there is a library that parses php errors and is not able to handle prefixes or suffixes. I'm using a similar workaround for many years now in code bases with 500k+ lines of code and it was never useless or dangerous. Regards Thomas Thomas Bley wrote on 28.01.2015 18:50: > > […] > > Yes that's true, maybe set_error_handler can be changed so it behaves like > register_shutdown_function or spl_autoload_register when multiple callbacks are > registered? > > Regards > Thomas > > > Andrea Faulds wrote on 28.01.2015 18:38: > > > […] --- ## #81351 — Yasuo Ohgaki — 2015-01-29T02:51:39+00:00 URL: https://externals.io/message/81193#81351 · In reply to: #81314 Hi Andrea, > Error handlers are global anyway, so this is useless (or dangerous) for > libraries. I suggest library/framework developers *not* to use user defined error handler. It's better to leave error handler for application developers. I strongly suggest application developers to use user defined error handler to catch unexpected problems. If app codes are written not to raise errors under normal operations (which I strongly recommend to do so), errors are sign of system problems. e.g. Subsystem is down, misconfiguration, crackers are attacking system, etc. In case of attack, admins would like to know which account is causing the problem. Error message like "Warning: failed to write /etc/passwd" is less useful than "Warning: failed to write /etc/passwd. User: yohgaki". User defined error handler is for error handling customization, letting user customize message makes sense. Thomas, If there are many people concerns to change error message, how about append user error message to original error message when it's changed? It's a hacky behavior to me, though. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #81352 — Yasuo Ohgaki — 2015-01-29T02:55:54+00:00 URL: https://externals.io/message/81193#81352 · In reply to: #81351 Hi Thomas, > If there are many people concerns to change error message, how about > append user error message to original error message when it's changed? > How about this signature. function myErrorHandler($errno, $errstr, $errfile, $errline, &$extra_errstr) Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #81355 — Thomas Bley — 2015-01-29T07:48:02+00:00 URL: https://externals.io/message/81193#81355 · In reply to: #81351 > User defined error handler is for error handling customization, letting > user > customize message makes sense. This is exactly our case. > Thomas, > If there are many people concerns to change error message, how about > append user error message to original error message when it's changed? > It's a hacky behavior to me, though. I see no difference if the userland developer can extend (or even change) error messages if he can completely disable (using @, return true in the callback, etc.). Of course it is our task to inform about best practices in the php manual and warn about bad things. Regards Thomas Yasuo Ohgaki wrote on 29.01.2015 03:51: > Hi Andrea, > > > > > […] > > > I suggest library/framework developers *not* to use user defined error > handler. > It's better to leave error handler for application developers. > > I strongly suggest application developers to use user defined error handler > to catch > unexpected problems. > > If app codes are written not to raise errors under normal operations (which > I strongly > recommend to do so), errors are sign of system problems. e.g. Subsystem is > down, > misconfiguration, crackers are attacking system, etc. > > In case of attack, admins would like to know which account is causing the > problem. Error message like "Warning: failed to write /etc/passwd" is less > useful than "Warning: failed to write /etc/passwd. User: yohgaki". > > User defined error handler is for error handling customization, letting > user > customize message makes sense. > > Thomas, > If there are many people concerns to change error message, how about > append user error message to original error message when it's changed? > It's a hacky behavior to me, though. > > Regards, > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net --- ## #81357 — Thomas Bley — 2015-01-29T07:55:03+00:00 URL: https://externals.io/message/81193#81357 · In reply to: #81352 I think you mean: function myErrorHandler( int $errno , string $errstr [, string $errfile [, int $errline [, array $errcontext [, string &$extra_errstr ]]]] ) We have these empty-call-filled-on-return parameters in preg_match(...$match) but I am not sure if it makes things more complicated than they should be. For me, changing $errstr to &$errstr is the simplest thing to do. Regards Thomas Yasuo Ohgaki wrote on 29.01.2015 03:55: > > Hi Thomas, > > > > > […] > > > How about this signature. > > > function myErrorHandler($errno, $errstr, $errfile, $errline, &$extra_errstr) > > > Regards, > > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net I think you mean: > function myErrorHandler( int $errno , string $errstr [, string $errfile [, > int $errline [, array $errcontext [, string &$extra_errstr ]]]] ) > Yes. > > We have these empty-call-filled-on-return parameters in > preg_match(...$match) but I am not sure if it makes things more complicated > than they should be. > For me, changing $errstr to &$errstr is the simplest thing to do. > I agree. I would like to have this feature. Better log is better management. I don't care much about complexity, but simpler is better. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net