Aggressively use actual function parameters in php_verror

[NattyNarwhal]

PHP errors used to not show parameter info consistently. Make it so that it uses a backtrace to get function info, similar to how exceptions work.

This makes the docref error functions' parameter argument mostly vestigal, being used only if allocation fails basically. The parameter argument may be useful in the case it is more verbose than the actual function args (is there a case?).

Several tests will fail from the fact we include function params. One annoyance is that _build_trace_args truncates strings according to exception_string_param_max_len, but at least this is user controllable.

See GH-12048. Test output will need a ton of rewriting, which I have not got to. CI will have a field day there...

[NattyNarwhal]

The new hotness:

calvin@anika php-src % sapi/cli/php ~/src/chmod.php

Warning: unlink('/tmp'): Operation not permitted in /Users/calvin/src/chmod.php on line 3

Warning: chown('/', 'calvin'): Operation not permitted in /Users/calvin/src/chmod.php on line 4

Warning: chmod('/', 511): Operation not permitted in /Users/calvin/src/chmod.php on line 5

Versus the old busted stuff:

calvin@anika php-src % /opt/calvin/php/bin//php ~/src/chmod.php

Warning: unlink(/tmp): Operation not permitted in /Users/calvin/src/chmod.php on line 3

Warning: chown(): Operation not permitted in /Users/calvin/src/chmod.php on line 4

Warning: chmod(): Operation not permitted in /Users/calvin/src/chmod.php on line 5

[Girgias]

Thanks for looking into this!

[NattyNarwhal]

The big awful annoyance is going to be rewriting pretty much every test file. I know there's been other wide sweeping commits that changed a lot of stuff before though. Were they doing anything automated to clean those up?

[iluuu1994]

@NattyNarwhal run-tests.php --bless

[NattyNarwhal]

That works, although paths are kinda annoying - all the absolute paths are there, and truncated, i.e. Warning: symlink('/Users/calvin/s...', '../bad/./symlin...'):. At least it fixed the resource IDs. I'm considering changing bless to make this easier to deal with.

[Girgias]

That works, although paths are kinda annoying - all the absolute paths are there, and truncated, i.e. Warning: symlink('/Users/calvin/s...', '../bad/./symlin...'):. At least it fixed the resource IDs. I'm considering changing bless to make this easier to deal with.

Changing bless probably makes sense as part of this PR yes, ideally it should replace those with %s

[NattyNarwhal]

So my change to bless as below:

diff --git a/scripts/dev/bless_tests.php b/scripts/dev/bless_tests.php
index fa49647fcf..2422512880 100755
--- a/scripts/dev/bless_tests.php
+++ b/scripts/dev/bless_tests.php
@@ -72,6 +72,8 @@ function normalizeOutput(string $out): string {
         'Resource ID#%d used as offset, casting to integer (%d)',
         $out);
     $out = preg_replace('/string\(\d+\) "([^"]*%d)/', 'string(%d) "$1', $out);
+    // Replace absolute paths, particularly those truncated; they're likely to have your homedir in it
+    $out = preg_replace("/'\\/.*\.\\.\\.'/", "'%s'", $out);
     $out = str_replace("\0", '%0', $out);
     return $out;
 }

...does work, like so:

-Warning: fileperms(): stat failed for /no/such/file/dir in %s on line %d
+Warning: fileperms('%s'): stat failed for /no/such/file/dir in %s on line %d

Of course, bless is also a little insensitive, so you do have to manually postprocess these (unless there's a better way to do it?)

-Warning: chmod(): %s in %s on line %d
+Warning: chmod('/etc/passwd', 511): Operation not permitted in %s on line %d

To speak nothing of the tests I can't run because i.e. Windows.

[Girgias]

One thing that I just thought about is that if the parameter is going to be displayed it should be suppressed if the SensitiveParam attribute is used.

[NattyNarwhal]

One thing that I just thought about is that if the parameter is going to be displayed it should be suppressed if the SensitiveParam attribute is used.

The code that prints the arguments in errors is shared with code that prints backtraces i.e. on exceptions. Both cases are handled, so you'll get Warning: odbc_connect('bogusdsn', 'user', Object(SensitiveParameterValue)): SQL error: [unixODBC][Driver Manager]Data source name not found and no default driver specified, SQL state IM002 in SQLConnect and Stack trace: #0 /Users/calvin/src/chmod.php(10): sensitive(Object(SensitiveParameterValue)).

[NattyNarwhal]

One thing that came to mind was turning this into an INI option, and it could be off for just the test suite or by default in general if the new output is intrusive. That said, I don't think configurability is a good idea (in terms of making the option used, and the PHP stance on introducing new options in general), but bringing it up anyways.

[Girgias]

Yeah not a fan about the INI setting :D

[NattyNarwhal]

I've pushed changes to ext/standard/tests/file, mostly because it would likely have a ton of absolute paths in the new parameters, and a small enough set to check bless output without getting overwhelmed. Unfortunately, I still had to change a lot of the test outputs back (or further manually) in cases where bless was overzealous, ignorant (still a lot of absolute paths in error messages), or a bit naive (i.e. fscanf test putting format strings like %d in paramters, which confuses EXPECTF). The question is if there's a better way to scale this up to all the other tests.

[NattyNarwhal]

I've rebased this onto master and added an INI option to gate this behind. I don't like adding INI options, but it might be a lesser evil than touching every PHPT file.

[bwoebi]

I don't think we should do this. Like, adding configs, just because it avoids updating some tests. What we were talking about over in the other PR is displaying the stacktrace, which anyway already has an ini.

[bukka]

I thought about this and the INI actually makes sense here. The reason is that this is more an operational thing. It's really just not about avoiding the tests updates. The thing is that for some legacy projects where warnings happening quite often (I saw quite a few such projects in past), this can lead not only to a significant increase of the log space but to the potential compliance issues. An example of that might be a function receiving email addresses and compliance with GDPR or similar. I realise that there is such potential with stack traces already but those are usually less common than warnings in the logs. So for some users it might be convenient to disable logging of parameters so having such INI seems reasonable to me.

[NattyNarwhal]

I'm not sure if you're talking about GH-17056, this PR, or both. Sorry for any confusion by mentioning that PR on a commit here; the discussion in that PR was motivating me to revive this PR I had.

[bukka]

I was talking about this PR but it could apply to both. But this one can especially increase the log size as fatal errors are not that common. However if you have lots regular warning in logs and they suddenly get all params logged, then it can increase the size and cost. So having an option to disable is a good think IMHO.