Implemented FR: Allow error_handler callback parameters to be passed by reference

[reeze]

Dicussion: http://marc.info/?t=142181539200002&r=1&w=2

Implemented FR: Enable error_handler callback parameters to be passed by reference.

for example, we can now have the ability to modify the errmsg.

<?php
set_error_handler(function(&$code, &$errmsg, &$file, &$lineno) {
    $code   = E_WARNING;
    $errmsg = "[Prefix] - {$errmsg} - [Suffix]" . $_SERVER['URI'];
    $file   = "a-new-file.ext.php";
    $lineno = 999;

    return false;
});

[yohgaki]

Nice work!

[laruence]

I don't think this patch is acceptable, the patch only cover the $errmsg in a specific situation.

we need a common / better patch for all cases like:

function error_callback(&$lineno, &$errmsg, &$line, &file)

thanks

[reeze]

I am not sure we should overwrite $lineno and $file etc parameters. If we think that is needed it will be easy to improve it.

[laruence]

why only one parameter can be reference, but others not?
doesn't it sound ugly to you?

[reeze]

I know your concern, I am afraid that modify the lineno and file seems a not good practice, since PHP don't have the ability to use macro, (such as the bison generator did), so I don't know is there any case require it to be a reference parameter.

[yohgaki]

Allowing modified errno, line, file is interesting idea.
We don't have macro, but we certainly change line/file for appropriate one. e.g. Actual cause may be in other file/line. Setting proper file/line may not be trivial, but it would be useful. However, one may get proper line/file by debug_backtrace() in most cases.

+1 for Laruence idea.

Since one can change message, proper file/line may be included in the message. So it's not strictly required.

[reeze]

Sounds reasonable, I will update it to allow all of the first four (but not context parameter) parameters to support pass by reference.

[reeze]

I just implemented the ability to pass the first four parameters to be reference.

Do I need a RFC for this?

[staabm]

@reeze could you describe a use-case when someone needs this by-ref here?

[reeze]

@staabm see the discussion here: http://marc.info/?t=142181539200002&r=1&w=2

One case I want it is I may need to add request uri and logid to errmsg, then our log aggregation system could merge the error message to help monitoring.

[smalyshev]

I personally think passing a bunch of params by-ref to change engine internal things like line number is not a good idea. With message, it could work as some kind of a stretch since message is not really an engine thing but with other params I think it's just a bad API. I think if you need custom error reporting just report custom errors. But feel free to submit an RFC and see if there are people that like it.

[yohgaki]

@reeze Yes. RFC is needed for this patch.

I suggest to have options in the RFC.

1 Make parameter reference

For people voted for YES, choose

1. Make message a reference
2. Make all parameters references

It would be easier to pass the vote. IMO.
BTW, I'm definitely willing to have by ref parameter and will vote for "make message a reference"..

[reeze]

Thank you guys for your responses!

Thomas says he will draft a RFC for it, I will collaborate with him.

FYI CC Thomas

On 24 January 2015 at 07:46, Yasuo Ohgaki notifications@github.com wrote:

@reeze https://github.com/reeze Yes. RFC is needed for this patch.

I suggest to have options in the RFC.

1 Make parameter reference

For people voted for YES choose

1. Make message a reference
2. Make all parameters references

It would be easier to pass the vote. IMO.
BTW, I'm definitely willing to have by ref parameter and will vote for
"make message a reference"..

Reply to this email directly or view it on GitHub
#1018 (comment).

[reeze]

Close as RFC been declined.