Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RFC: Error Backtraces v2 #17056
RFC: Error Backtraces v2 #17056
Changes from 7 commits
3b9bd2f
aa9c4f4
47148c1
a44ac31
e843e06
2cd6c3f
aaa2443
5fd14ee
b1db022
9f29e80
126a785
a6c9063
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% confident in this being the right fix, though it is similar to the
EG(exception) = NULL
on L906 above. Thoughts?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems correct, this will clear the message for the next error. Do you have a test for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a sense, yes, since the existing
ext/soap
tests triggered the need for this, and once it was fixed the tests passed. On the other hand, it was only theext/soap
code that needs this. For example, throwing an exception that doesn't get caught in normal PHP won't trigger a double stack trace.I'm concerned this (clearing the trace when throwing an exception) may not be enough and there may be other "weird" error handling things in extensions that may also end up having a double stack trace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard to understand the problem exactly, it would be best to replicate in a new test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not exactly sure how to replicate it in a test, since it has something to do with how
ext/soap
manipulateszend_error_cb
in C. I linked theext/soap
test that demonstrates this behavior in the commit message for this change: e843e06When I was debugging this, I noticed that
ext/soap
changes thezend_error_cb
here:php-src/ext/soap/soap.c
Line 548 in a091e52
It's then kind of hard to follow, but roughly speaking it ends up in here:
php-src/ext/soap/soap.c
Lines 1868 to 1871 in a091e52
If I remember correctly, the backtrace exists at this point, and then calling
zend_throw_exception_object(&fault);
andzend_bailout();
end up triggering the originalzend_error_cb
which prints out the exception and the backtrace, thus producing a double "Stack trace:" output.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having dealt a tiny bit with ext/soap my guess is the following:
SOAP Uses the bailout mechanisms (i.e. fatal errors) as its "exception" mechanism (as it predates exceptions existing in the language), and it now catches those to throw them as proper exceptions (although it sometimes also throws normal exceptions) which is probably the cause of this.
Refactoring ext/soap so it stops doing this is a long term goal of mine, but the extension is quite complex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that makes sense. Do you think the above is an appropriate fix that might apply to other extensions doing stuff like this? I'm repeating myself, but I'm concerned that there might be other complex flows involving
zend_error_cb
that might run into related issues.Would it make more sense to do backtrace fetching only in
php_error_cb
? Perhaps that would solve the issue since presumablyEG(exception)
would be set and could be checked before fetching? Edit: though that would mean other extensions couldn't rely on the backtrace existing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frankly, the only other extensions I see overloading
zend_error_cb
are debuggers and profilers. ext/soap is very special in this instance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is supposed to be readable in HTML, but I doubt the backtrace will be displayed in a readable manner. Perhaps, this should be wrapped in a
<pre>
tag or smth alike?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I'll look into that.
Semi-related tangent so feel free to disregard: Is it on anyone's radar to refactor this function at all? I saw "Deprecate the
error_prepend_string
anderror_append_string
INI directives" in https://wiki.php.net/rfc/deprecations_php_8_5, which would help make this a little more readable, but I wonder if there's more we could do simplify this logic. I acknowledge that is probably easier said than done, however.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt refactoring this function is on someone's TODO list. There are indeed some outdated pieces (e.g. the ones you mentioned but also xmlrpc_errors imo). However, the question with refactoring is always how much it's worth spending time on this. This function likely is not on any hot code path and refactoring also brings in a risk of breaking things 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be also nice to break up those huge long lines when you are in it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nielsdos apologies, but what's the best way to wrap a
zend_string
in<pre>
?strpprintf
? Or would it be the worst thing to just change this to%s%s%s
and then pass inZSTR_LEN(backtrace) ? "<pre>Stack trace:\n" : "", ZSTR_VAL(backtrace), ZSTR_LEN(backtrace) ? "</pre>" : ""
? We could also drop adding thepre
at all since we already have some fatal errors with stack traces that do not havepre
: https://github.com/search?q=repo%3Aphp%2Fphp-src+%22%3Cb%3EFatal+error%3C%2Fb%3E%22&type=code, e.g.@bukka I received feedback elsewhere to not break up long lines, though maybe here is a good counterpoint. I'd personally like to err on keeping it as-is since I'd like to be able to merge this relatively soon, but if people feel strongly I could break this up. This is kind of what I was getting at with my earlier comment - this function could probably be simplified to make it more readable, besides just the long line length.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we wrap it with pre it should be done consistently.
If I were to write the code, I would go for the ZSTR_LEN() ternary trick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow - what I was saying was that we currently do not wrap it with
<pre>
(for errors that already have backtraces before my change), so if we wrap it with<pre>
here we'd be inconsistent with existing errors like:php-src/sapi/cgi/tests/004.phpt
Lines 43 to 46 in 72708f2
Thanks, I was leaning that way but I wanted your opinion. I'm still not convinced we should add the
<pre>
based on the above, but if you think we should I'll go theZSTR_LEN
route.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case you are referring to my remark here: #16937 (comment) - this case of a printf-style function is quite different and splitting it across multiple lines would be reasonable, because it accurately represents the complexity of the statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok let's not do the
<pre>
conversion.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TimWolla I was! and when I said "maybe here is a good counterpoint", I meant that I could see that you might agree with splitting this up. My preference was to not do this myself though, since for consistency I should probably do that for all of them, and I was concerned with making it a topic for bikeshedding.