Function (usually taking filenames) inconsistently display important arguments in errors

[NattyNarwhal]

Description

If I get the same error with unlink, chown, and chmod, I only get the error filename in one:

calvin@anika src % /opt/calvin/php/bin/php 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
calvin@anika src % cat chmod.php 
<?php

unlink("/tmp");
chown("/", "calvin");
chmod("/", 0777);

As a real-world example of how annoying this can be, I was assisting someone debug an issue in WordPress. Because these are traditional PHP errors and not exceptions, and because WordPress abstracts direct filesystem calls in places like class-wp-filesystem-direct.php, you're left with a lot of messages with no context and no clue as to what it's trying to manipulate.

This is caused by the fact that unlink goes to a function that calls php_error_docref1 with the passed filename, and other functions just call php_error_docref.

Ideally, these should all be made consistent with each other and show important function arguments as needed.

[iluuu1994]

@NattyNarwhal Thanks for your analysis! Mind sending a PR for these cases? Otherwise I can create one. Searching for if (ret == -1) { might be useful.

[NattyNarwhal]

There are a lot of cases, and far beyond the file related things. It looks like one of those "look at every possible docref call" things unfortunately. That said, if you want to have a crack at it, feel free. The changes look simple to make, though you're probably have to touch a ton of phpt files (and add a ton of EXPECTF).

[iluuu1994]

@NattyNarwhal I don't think we need to "fix" all cases. Many of the docref calls include the params somewhere in the message. The cases where -1 is returned usually come from the underlying call, which normally don't contain the filename. Fixing those would be a good start.

[NattyNarwhal]

Makes sense. It might make sense to convert some of those to docref1 or whatever, but that's a separate concern.

[Girgias]

I'll just mention #4254 and #4394 as those are somewhat related.

Personally, I think either all arguments should be displayed or none at all. And because 99% of cases don't display anything, I'd be more inclined to remove the remaining docref1 and docref2 variants than anything else.

But if a generic way is found to display all argument error for all functions, than I am also OK with that.

[iluuu1994]

@Girgias Worsening error message for the sake of consistency doesn't seem like the right move. Displaying parameters for everything is fine, but as existing messages already often include them they would appear twice. Furthermore, for some errors we likely don't want to display them, at least not in full (e.g. file path is too long).

[Girgias]

Thus, the question becomes, when should the format func(param1, param2) be used.

Considering that those variants are only used for streams, I wonder if it doesn't make more sense to improve the description of the warnings and use the same standard format than using the format of docref1/2.

[NattyNarwhal]

Couldn't php_verror print the argument list for the function presumably on the top of the call stack, like how the stack trace code in debug_backtrace_get_args does? I don't know on the top of my head if it works for native code functions in same way, and it'd probably need to be refactored a bit (i.e. don't need to descend the call stack), but it'd print out the argument list, including handling for i.e. sensitive parameters.