Change E_WARNING to ValueError in sprintf functions

[moufmouf]

I'm considering writing an RFC to turn some warning into exceptions in a number of PHP functions.

This PR is just a support for the discussion I'm opening on internals. Do not merge.

TODO:

• Discuss whether it is appropriate to consider moving from E_WARNING to Exceptions in a number of PHP functions on the internals mailing list
• Write an RFC
• Add other functions (not only sprintf)
• Declare specific exceptions (rather than throwing Error)

[nikic]

We're generally okay with these warning -> exception conversions in library functions without an RFC as long as they meet certain requirements (which seems to be the case here from a quick look).

[Girgias]

Added some suggested changes to use the new ValueError exception introduced with PHP 8

[Girgias]

@moufmouf We've been already changing some of the warnings to exceptions for warnings which are triggered on programming errors.
To me those changes fall into that scope.

Edit: see for example the array functions (which I still need to update to use zend_error_value) or the GD library.

[moufmouf]

We're generally okay with these warning -> exception conversions in library functions without an RFC as long as they meet certain requirements (which seems to be the case here from a quick look).

Excellent! I was under the impression a RFC was needed.

as they meet certain requirements

Are those requirements documented somewhere?

[Girgias]

as they meet certain requirements

Are those requirements documented somewhere?

As it is a recent thing, not really:

• type errors (use zend_type_error() function)
• invalid mode/flag values
• non-negative/non-empty errors

is the general guideline

[moufmouf]

Hey @Girgias ,

Thanks a lot for the detailed comments and for the explanation.
I pushed a fix that should resolve all your comments. We are now throwing the new "ValueError" exception.

[Girgias]

I apologise I forgot we had zend_argument_count_error() which probably makes more sense in this case.

[moufmouf]

I started working on replacing the error message with something like:

zend_argument_count_error("%d parameters are required, %d given", argnum + 2, argc + 1);

It makes a lot of sense most of the time, but it turns out I'm running in an issue with "vprintf".

vprintf takes an array of placeholders as the second argument:

vprintf ( string $format , array $args ) : int

The message generated would be something like:

5 parameters are required, 3 given

which is blatantly misleading.

I was thinking about replacing the message with something like:

4 placeholders are required, 2 given

My question: does it still qualifies as a ArgumentCountError ? Or should I stick to the ValueError ?

[Girgias]

I'm not familiar with this piece of code, but if the v*print* family of functions use the same handler it's probably best to keep a value error, (splitting the function in two seems a bit pointless here), because when using an array you are passing the correct number of arguments but the array has the wrong "shape", thus I'd use a ValueError in that case, if this means using it for all other cases, I would suppose this to be alright.

[moufmouf]

I added a new arguments to php_formatted_print to specify the kind of error message to be thrown. That way, I can throw a ValueError for vprint functions and a ArgumentCountError otherwise, with appropriate error messages.

I'm not completely sure if this make sense, and the error message ("The arguments array must contain %d items, %d given") might need to be worked upon.

Let me know!

[Girgias]

I'm not sure either, tbh, @nikic any opinion? I don't feel like this complication is worth it, but the error messages being more descriptive is quite nice ...

[Girgias]

32 bits test failure is legit on Azure Pipelines.

[drealecs]

I like the promotions to Exceptions for most of the PHP internal functions.
But sprintf's $format parameter could maybe come from a database of local formatting units. Or sent using a badly designed API: search for "sprintf($_GET["
Basically, for a lot of functions we can find that parameters are sometimes not safe to trust so there will always be some BC break and most probably we will want to do it i a major version, 8.0.
I just want to point this out so you can think again if we need a RFC for it, maybe...

[nikic]

@drealecs This change is for PHP 8. Of course, we cannot make such a change in 7.4.

[villfa]

Just in case it got unnoticed, ext/standard/tests/strings/printf.phpt didn't pass on Azure for this PR, and now that it's merged this test also fails with master.