Make exception messages and error output binary safe #413
Conversation
For whatever reason, prepend and append strings don't get prepended and appended when the error output is to stderr. This retains that behaviour in the new code.
|
Maybe a better solution would be to add a new printf modifier for printing a string with length? I mean, in PHP that's really common, but right now we always have to use PHPWRITE for it (for output) or manually copy (for string). Seems rather suboptimal. |
| str[len] = '\0'; | ||
|
|
||
| efree(prefix); | ||
| efree(suffix); |
There was a problem hiding this comment.
Couldn't you use %Z here too?
len = zend_spprintf(&str, 0, "exception '%s' with message '%Z' in %s:%ld\nStack trace:\n%s%s%s",
Z_OBJCE_P(exception)->name, &message, Z_STRVAL(file), Z_LVAL(line),
(trace && Z_STRLEN_P(trace)) ? Z_STRVAL_P(trace) : "#0 {main}\n",
len ? "\n\nNext " : "", prev_str);
|
Of course |
|
Adding it to php sprintf won't help the case with fprintf though. |
|
Comment on behalf of krakjoe at php.net: Since this PR has merge conflicts, seems to have been abandoned by author, and since it would seem at least partially unnecessary, I'm closing this PR. Please take this action as encouragement to work on this feature, if you think it valuable. Please ensure any new PR has satisfactory test coverage, and ensure those tests pass. |
This will probably need an RFC, but until I have time to write one, this PR will have to do to track this.
Previously, exception messages were documented as not being binary safe. The reality was actually more complicated than this:
Exception::$messageitself was binary safe if accessed throughException::getMessage(), but wasn't binary safe when the full exception string was constructed byException::__toString().I dislike arbitrary restrictions and inconsistent behaviour, hence this PR.
9b1069f removes the exception restriction. By itself, that still isn't all that useful, since error output also isn't binary safe. The follow up commits remove that restriction (of course, if your actual stdout/stderr aren't binary safe, you have lost regardless).
Things that would still have to be dealt with before this became mergeable:
Exception::__toString(). Is that really a problem in practice? Error and exception code aren't speed demons at the best of times.%Zprintf modifier. I presume there is a reason why this has been avoided until now, although I have no idea what that reason might be. This could be worked around if needed.