Repository navigation
Function (usually taking filenames) inconsistently display important arguments in errors #12048
Description
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.
Activity
iluuu1994 commentedon Aug 28, 2023
@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 commentedon Aug 28, 2023
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
phptfiles (and add a ton ofEXPECTF).iluuu1994 commentedon Aug 28, 2023
@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
-1is returned usually come from the underlying call, which normally don't contain the filename. Fixing those would be a good start.NattyNarwhal commentedon Aug 28, 2023
Makes sense. It might make sense to convert some of those to
docref1or whatever, but that's a separate concern.Girgias commentedon Aug 30, 2023
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 commentedon Aug 30, 2023
@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 commentedon Aug 30, 2023
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 commentedon Aug 30, 2023
Couldn't
php_verrorprint the argument list for the function presumably on the top of the call stack, like how the stack trace code indebug_backtrace_get_argsdoes? 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.Aggressively use actual func params in verror
Girgias commentedon Jun 20, 2026
Going to close this as #12276 is merged.