Concerns about passing null to some polyfill arguments

[pilif]

For years our code-base (like presumably many others) had an existing mb_trim() polyfill of our own and as it pre-dates even php 7, it had a slight quirk in that it accepted a null as the first argument.

PHP 8.4 will provide its own mb_trim() implementation which has its first argument typed as string, but which also accepts a null argument, albeit with a deprecation warning.

The symfony polyfill on the other hand does not accept a null argument and thus, it being an userland function will throw a TypeError instead.

In the case of our code-base, this caused an issue where a completely non-related composer update now turned code which run perfectly well (and would have changed to raising a deprecation notice in 8.4) into a TypeError.

I know that this is probably a slight edge-case, but I'm still coming here to raise the issue because it might affect other users.

IMHO, to be a proper polyfill for PHP 8.4 mb_trim, your implementation should accept nulls too and raise an E_USER_DEPRECATED error if a null is passed.

And the same is probably true for many other functions as well.

[Ayesh]

I'd rather not deviate the polyfill from the actual function signature TBH.

Would it not be easier to add a (string) cast to all of the callsites in the application?

- mb_trim($value);
+ mb_trim((string) $value);

[nicolas-grekas]

Duplicate of #499, which already has #501, isn't it?

[pilif]

yes. if course. That's (more or less) what I have done.

But still. It feels incorrect for a polyfill to not actually polyfill the function it's supposed to polyfill but to be more opinionated about its arguments.

Think from a user's perspective. Even with all the semver rules in place, updating from 1.30 to 1.31 (or in my case updating a patch release of a dependency) caused a TypeError to be thrown where there previously wasn't.

[pilif]

I'm not sure this is a duplicate of #499.

This is about throwing a TypeError where the polyfilled original function is only throwing a deprecation notice.

[stof]

Accepting null in the polyfill should indeed be the right behavior (as done in other polyfills), to match the behavior of the native function. This is what we do for most polyfilled functions, but it looks like this has been missed for the more recent additions to the mbstring polyfill.