Update request startup error messages

[ericnorris]

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 for SG(headers_sent) to prevent this spurious message.

I've included some of the output of make test on my changes below:

=====================================================================
TEST RESULT SUMMARY
---------------------------------------------------------------------
Exts skipped    :   47
Exts tested     :   26
---------------------------------------------------------------------

Number of tests : 15262             10259
Tests skipped   : 5003 ( 32.8%) --------
Tests warned    :    0 (  0.0%) (  0.0%)
Tests failed    :    1 (  0.0%) (  0.0%)
Expected fail   :   36 (  0.2%) (  0.4%)
Tests passed    : 10222 ( 67.0%) ( 99.6%)
---------------------------------------------------------------------
Time taken      :  298 seconds
=====================================================================

=====================================================================
FAILED TEST SUMMARY
---------------------------------------------------------------------
Test ReflectionZendExtension class [ext/reflection/tests/ReflectionZendExtension.phpt]
=====================================================================

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.

[krakjoe]

While we're doing something about this, could we maybe drop the "line 0" ?

[KalleZ]

I agree with @krakjoe, think it is time for that redundant part of the error message to go for good

[ericnorris]

Sure! I'm happy to make the additional change. If I understand correctly that is coming from:

php-src/main/main.c

Line 1270 in c6b5352

PG(last_error_lineno) = error_lineno;

Would it be worth it to handle the "in Unknown" part as well? Which comes from here:

php-src/main/main.c

Line 1265 in c6b5352

error_filename = "Unknown";

[Girgias]

Sure! I'm happy to make the additional change. If I understand correctly that is coming from:

php-src/main/main.c

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:

php-src/main/main.c

Line 1265 in c6b5352

error_filename = "Unknown";

If you can do it I suppose it would be reasonable to bundle it in one PR.

[ericnorris]

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:

1. Add a bunch of conditionals to php_error_cb in order to check that error_filename and error_lineno are known, e.g. around:

   php-src/main/main.c

   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.

2. Replace php_error_docref (here) and its variants with a macro that passes in __FILE__ and __LINE__, and then setting some PG global to track the PHP source code that triggered the error. Instead of a PG global I could also pass them as parameters to php_verror (here), but I found a couple callsites that call php_verror directly, and I'm not sure if non-bundled extensions call the function directly as well. Setting a global in php_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.

[ramsey]

What's the status of this PR? Can we get it ready to merge to master for 8.2?

[ericnorris]

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.

[ericnorris]

@ramsey I believe the PR is mergeable now, so let me know if you need anything on my end.

[bukka]

First part (not sending headers) merged in 922371f

[bukka]

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!