More standard conform display of errors#308
Conversation
| foreach ($ini as $option => $value) { | ||
| $result = ini_set($option, $value); | ||
| if ($result === false) { | ||
| $errors[] = sprintf('ini setting "%s"', $option); |
There was a problem hiding this comment.
Please throw directly. It's over engineered this way.
There was a problem hiding this comment.
Of course, we could do it that way, but in my well-founded opinion, it would make a big difference whether one patch or several become required on the fail-path situatio. But that might not be obvious at first glance either.
To illustrate, here’s an example from my notes:
$ php8.3 doc-base/configure.php -V
PHP Fatal error: Uncaught Error: php.ini errors (3)
ini setting “display_errorsx”
ini setting “display_startup_errorsx”
ini setting “error_reportingx” in /home/builder/local/phpdoc/doc-base/configure.php:154
Stack trace:
#0 /home/builder/local/phpdoc/doc-base/configure.php(136): errorsWrap()
#1 /home/builder/local/phpdoc/doc-base/configure.php(22): errorsAll()
#2 {main}
triggered in /home/builder/local/phpdoc/doc-base/configure.php on line 154
# terminated with exit code 255A typical worst-case example for demonstration purposes: The implementation of the fail-fast test has been intentionally engineered over its total scope.
| } | ||
|
|
||
| function errorsAll(): int { | ||
| static $ini = [ |
There was a problem hiding this comment.
No need to use static here. Also it's better to set error_reporting here instead or returning E_ALL.
There was a problem hiding this comment.
well, static reflective, just saying, I kept the variable though; and if the error reporting call is mandated to move away from it's original call-point, I'd go with it — please find it updated and let me know if this was on your mind.
|
Are you using an LLM to write these PRs? It's a lot of information for such a small change, and the change feels over engineered. What you are trying to do is fine though. |
|
I vote against this change. Running Also, most recent changes in subprograms is to remove printing on STDERR, in favor of STDOUT, so the errors and warnings show very near each normal output, ans so errors/warnings can be correlated with their surrounding code. |
Well I currently have one PR open, and I've not used an LLM for my commit message. I use tooling though that radically promotes such usage to it's users. I have the AI Assistant disabled, including any so called local inline completion. I know some developers swear on the later one, but I hate it too often, for the little cases where it once was of benefit. BTW. while my IDE nowadays some subpar interface for git, you find me often typing
Yes, I've read you voiced that, but no sweat, there is no need to rush things through, take the time, one of the benefits of written communication, another is the reflection it allows me, LLMs do not offer any of these and that's why I prefer the human interaction in the communication.
I hope so, because I wanted to share it. For my own fork, it would not require any of these, it's just me trying to help based on yesterdays chatter. @alfsb reaction of a small panic mode my notion of the red build painting, it somewhat strived my thoughts today. All fine. |
Where does this change promote that? The rationale educates exactly to do it otherwise, and informs about the CLI SAPI in case it's not yet known and how it works with passing diagnostic information along on the command line. And for the subprograms please take a look if it is not preferable to integrate further, I remember to have seen an existing configure flag that is exactly for this - if even necessary as the programming environment on the command line normally offers enough control, often there is no need to even touch code then. Happy if you show me the cases where you prefer better reporting, and again, please do make use of your CLI interface, I can highly recommend it. |
Rationale
Users may execute the configure.php script within any
SAPI their PHP program of choice allows them, and the
configure PHP script makes very little judgment about
that.
In PHP 4.2 the experiment with a dedicated PHP CLI
SAPI was begun, with general availability in the next
minor version. It underwent more and more improvements,
and since PHP 5.1 the CLI SAPI offers an interactive
shell.
This PHP CLI SAPI is specifically intended to execute
PHP scripts *not* within a webserver environment, but
on the command line; for example when the user is
PHP-scripting in their shell.
The 'PHP CLI SAPI' defines the 'STDERR' constant as a
convenience feature (next to the 'STDIN' and 'STDOUT'
constants), unless some environmental situation
prevents that for the one or other standard stream.
This feature is available only in the PHP CLI SAPI,
therefore:
Given the configure.php script is evaluated by PHP,
when the 'STDERR' constant is defined,
then upgrade the display of errors [1].
This change is lifting the diagnostic output to the
standard error channel, 'standard error' (aka 'stderr',
file-descriptor number two, value 2), and by that
shifting all PHP diagnostic messages (aka 'errors').
These errors are now on their standard channel, that
streams important diagnostics, aka errors, to the user
of the script. The place where they belong on the
command-line, not any longer interfering on the output
channel.
This is in the tradition to not print the error message
on paper between some text on page x of n, but on the
display where they are useful, e.g. the diagnostic
channel where the printing has been invoked, or the
display of the printer itself, if it has one.
The traditional use of a (configure.)php script in a PHP
SAPI that outputs on the users request via an HTTP
client is reasonably unaffected.
Implementation
Subsumizes the change under the speaking function name
'errorsAll'. It returns the 'E_ALL' constant value for
the standard error-reporting under the configure.php
script invocation and improves setting the ini options
by collecting errors, and given there are some, throws
them under the PHP 7/8 Error protocol en-groupe.
NOTE: Under an error during all ini_set() operations,
configure.php now finally throws and exits in
FAILURE (not as previously ignoring all those
errors) - this is to *fail-fast* [2].
To give the full error picture, a small errorsWrap()
helper function has been added that allows to collect
errors over multiple operations, and 'wraps' one (or
more) nicely into one Error message, as a Stringable
Throwable, e.g. it can be shown, logged, or thrown.
The errorsAll() function serves as a usage example.
The implementation is maintaining in top-down direction,
based on the first-time-bottom-up implementation (1st
'errors_are_bad', then 'errorbox') in the error
diagnostics handling scope of the configuration.
[1] For PHP startup-errors it might be too late, but we
do it intentionally nevertheless to make the option
values available via a 'php.ini' file or ini_get(), e.g.
when configure.php delegates to other PHP scripts under
the same configuration. It should go without writing
that an improvement there certainly is possible as well,
but unrelated and independent of this change.
[2] 'fail-fast': If early configuration conditions are
already failing, the configure script can do a better
job by directly failing, as it has already seen that
it is not able to configure itself for any kind of
diagnostics that wouldn't even remotely match their
names; see "Fail-Fast System" in the English
Wikipedia to explore in a more technical context
<https://en.wikipedia.org/wiki/Fail-fast_system>.
Coming back to this question, what I actually do not know is if the owner of the Microsoft Github PHP org/namespace did opt-out of LLMs and applies that continuously and across their org? I'm using it for my PRs. @jordikroon |
Here: "The traditional use of a (configure.)php script in a PHP SAPI that outputs on the users request via an HTTP client is reasonably unaffected." HTTP clients should not be unaffected. If anything, HTTP clients should be blocked.
Considered, and rejected. |
|
There is no point in making error handling this complicated. Even less so in command line scripts that use |
3 participants
Rationale
Users may execute the configure.php script within any SAPI their PHP program of choice allows them, and the configure PHP script makes very little judgment about that.
In PHP 4.2 the experiment with a dedicated PHP CLI SAPI was begun, with general availability in the next minor version. It underwent more and more improvements, and since PHP 5.1 the CLI SAPI offers an interactive shell.
This PHP CLI SAPI is specifically intended to execute PHP scripts not within a webserver environment, but on the command line; for example when the user is PHP-scripting in their shell.
The 'PHP CLI SAPI' defines the 'STDERR' constant as a convenience feature (next to the 'STDIN' and 'STDOUT' constants), unless some environmental situation prevents that for the one or other standard stream.
This feature is available only in the PHP CLI SAPI, therefore:
This change is lifting the diagnostic output to the standard error channel, 'standard error' (aka 'stderr', file-descriptor number two, value 2), and by that shifting all PHP diagnostic messages (aka 'errors').
These errors are now on their standard channel, that streams important diagnostics, aka errors, to the user of the script); The right place where they belong on the command-line. not any longer interfeering with the output channel.
This is in the tradition to not print the error message on paper betweem some text on page x of n, but on the display where they are useful, e.g. the diagnostic channel where the printing has been invoked, or the display of the printer itself, if it has one.
The traditional use of a (configure.)php script in a PHP SAPI that outputs on the users request via an HTTP client is reasonably unaffected.
Implementation
Subsumizes the change under the speaking function name 'errorsAll'. It returns the 'E_ALL' constant value for the standard error-reporting under the configure.php script invocation and improves setting the ini options by collecting errors, and given there are some, throws them under the PHP 7/8 Error protocol en-groupe.
Note
Under an error during all ini_set() operations, configure.php now finally throws and exits in FAILURE (not as previously ignoring all those errors) - this is to fail-fast [2].
To give the full error picture, a small errorsWrap() helper function has been added that allows to collect errors over multiple operations, and 'wraps' one (or more) nicely into one Error message, as a Stringable Throwable, e.g. it can be shown, logged, or thrown.
The errorsAll() function serves as a usage example.
The implementation is maintaining in top-down direction, based on the first-time-bottom-up implementation (1st 'errors_are_bad', then 'errorbox') in the error diagnostics handling scope of the configuration.
[1] For PHP startup-errors it might be too late, but we do it intentionally nevertheless to make the option values available via a 'php.ini' file or ini_get(), e.g. when configure.php delegates to other PHP scripts under the same configuration. It should go without writing that an improvement there certainly is possible as well, but unrelated and independent of this change.
[2] 'fail-fast': If early configuration conditions are already failing, the configure script can do a better job by directly failing, as it has already seen that it is not able to configure itself for any kind of diagnostics that wouldn't even remotely match their names; see "Fail-Fast System" in the English Wikipedia to explore in a more technical context https://en.wikipedia.org/wiki/Fail-fast_system.