-
Notifications
You must be signed in to change notification settings - Fork 8k
Update request startup error messages #4865
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
Update request startup error messages #4865
Conversation
82b04f7
to
c6b5352
Compare
While we're doing something about this, could we maybe drop the "line 0" ?
I agree with @krakjoe, think it is time for that redundant part of the error message to go for good
Sure! I'm happy to make the additional change. If I understand correctly that is coming from:
Line 1270 in c6b5352
PG(last_error_lineno) = error_lineno;Would it be worth it to handle the "in Unkown" part as well? Which comes from here:
Line 1265 in c6b5352
error_filename = "Unknown";
If you can do it I suppose it would be reasonable to bundle it in one PR.
I've added a commit to remove another source of Warning: Unknown:
in error messages. Previously errors generated during PHP_RSHUTDOWN
would have an Unknown
function name, the commit checks for EG_FLAGS_IN_SHUTDOWN
to detect this.
I also looked over the past weekend and came up with two options for removing in Unknown on line 0
:
-
Add a bunch of conditionals to
php_error_cb
in order to check thaterror_filename
anderror_lineno
are known, e.g. around:Line 1351 in 8c5ca24
fprintf(stderr, "%s: %s in %s on line %" PRIu32 "\n", error_type_str, buffer, error_filename, error_lineno);
This would probably end up making the function a lot more complicated, however. -
Replace
php_error_docref
(here) and its variants with a macro that passes in__FILE__
and__LINE__
, and then setting somePG
global to track the PHP source code that triggered the error. Instead of aPG
global I could also pass them as parameters tophp_verror
(here), but I found a couple callsites that callphp_verror
directly, and I'm not sure if non-bundled extensions call the function directly as well. Setting a global inphp_error_docref
feels less likely to break things, but is kind of weird.
Let me know if I should pursue either of these, or if a better idea comes to mind.
b9ba8c9
to
a143a49
Compare
What's the status of this PR? Can we get it ready to merge to master for 8.2?
Hey @ramsey! No one seemed to be interested in following up on my comment, so I'd be happy to just fix the current conflicts in order to at least get an incremental improvement out for PHP 8.2.
@ramsey I believe the PR is mergeable now, so let me know if you need anything on my end.
First part (not sending headers) merged in 922371f
The rest merged via 09237f6 . Thanks!
I also did some checking around removing in Unknown on line 0
I think it would be worth to check if we could just not show it in php_error_cb
when the line is 0. Which is basically what you suggested in number 1. I think that's probably better option. Not sure however if there is any valid case when printing 0 would make sense so it might require some checking.
About the option 2, it could be done by adding extra parameter (flag) to php_error_cb
but that might not be appreciated by external extensions like Xdebug that overloads it so maybe that global flag (PG) would be better. Although I agree that it's kind of messy so that's why I think the option 1 is better.
I think this doesn't need to be done here as this is already improving things. Because it could actually still make the feature freeze for PHP 8.2, I decided to merge it as it is (split in to 2 parts as those were unrelated things). Of course if you want to work on the above, which would go most likely to PHP 8.3, that would be great! In any case thanks for you contribution so far!
This pull request adds a
PG(during_request_startup)
check in order to display a more accurate error message earlier on in the PHP request lifecycle.Additionally, I noticed a secondary warning was being generated by the
PG(expose_php)
check if an earlier warning caused the headers to already be sent. This PR adds a check forSG(headers_sent)
to prevent this spurious message.I've included some of the output of
make test
on my changes below:Something is causing the PHP version of my local build (on OS X) to be wonky and leads to the above test failure, but that is not related to my changes.