Fix GH-18823: setlocale's 2nd and 3rd argument ignores strict_types

[ndossche]

No description provided.

[divinity76]

Nitpick, don't use u32, use size_t.
Using u32 leads to bugs like https://bugs.php.net/bug.php?id=80523 (where 64bit php would choke on source code above 4GB)

[ndossche]

The argument count is 32-bits.

[divinity76]

Reminds me of a famous quote attributed to Bill Gates,

4.2 billion arguments ought to be enough for anybody!

Ok nevermind.

Code LGTM!

[ndossche]

I believe promotion rules turn it into an unsigned number, but indeed should be unsigned. I'll change it

[DanielEScherzer]

My reading of https://www.php.net/manual/en/function.setlocale.php is that you can either use

• setlocale(int $category, string $locales, string ...$rest) where argument 2, 3, etc. are strings
• setlocale(int $category, array $locale_array) where argument 2 is an array and there is no argument 3

and so I would expect error for the third argument to just say string, not array|string.

But, looking at the code it appears that any of the subsequent parameters can be an array - is that correct, and the documentation is wrong? Or is that unintended?

Should the types of the values in the array also be checked?

[ndossche]

Any arg can be an array here so the docs are simply wrong. We should change the docs to match the C sources because we consider the C sources as the source of truth.
Coercion inside arrays is the de facto norm, but no strong feelings there.

[Girgias]

This doesn't cover all cases.

For some other references:

• Why we don't mess with values within arrays with strict_types: #5823 (comment)
• We could warn if the array doesn't strictly hold a string: #6921

[Girgias]

This doesn't cover the case where a non-stringable object is passed as argument, nor a resource, or null. (in weak mode)

[ndossche]

I first was confused by your comment, but you mean the coercions in weak mode are not done a priori and thus there is also no proper error handling of that.
I suppose we can call zend_parse_arg_str ourselves and do the coercions upfront instead of in try_setlocale_zval.

[Girgias]

That and also types that should be rejected by our normal semantics were not :)

[ndossche]

Test fails on arm, probably because of weak mode coercion changes, probably due to the deprecation now being emitted and the diff algorithm also seems confused as usual.
EDIT: that was indeed the reason

[remicollet]

NULL needs to be allowed for the second parameter, reported as #19070