Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:117790
Content-Type: text/plain;
	charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.80.82.1.1\))
In-Reply-To: <1180af01-080f-ee0a-3159-74bf7e0a8aea@gmail.com>
Date: Thu, 26 May 2022 13:20:53 +0100
Cc: internals@lists.php.net
Content-Transfer-Encoding: quoted-printable
Message-ID: <73F563E2-7C31-4B5E-A6B9-AE1BD05ADD1C@craigfrancis.co.uk>
References: <CAFv4g+Ej4JbM9SzQ0iU-d8C4T+2XNo=8jnJ1uwfQ=skZZXx9Tg@mail.gmail.com>
 <1755E8B5-229B-47B2-BBAF-B5E014F5473D@craigfrancis.co.uk>
 <1180af01-080f-ee0a-3159-74bf7e0a8aea@gmail.com>
To: Rowan Tommins <rowan.collins@gmail.com>
Subject: Re: [PHP-DEV] NULL Coercion Consistency
From: craig@craigfrancis.co.uk (Craig Francis)

On 24 May 2022, at 09:47, Rowan Tommins <rowan.collins@gmail.com> wrote:
> On 23/05/2022 19:45, Craig Francis wrote:
>=20
>> For those against my RFC, can you take a quick look at this patch for =
Laravel:
>>=20
>> =
https://github.com/laravel/framework/pull/36262/files#diff-15b0a3e2eb2d683=
222d19dfacc04c616a3db4e3d3b3517e96e196ccbf838f59eR118
>>=20
>> If passing NULL to `htmlspecialchars()` represents a problem, as used =
in templates like `<p>Hi {{ $name }}</p>`, presumably this patch should =
be reverted?
>=20
>=20
> I notice that the docblock didn't previously list null as valid input, =
so this was only working by mistake, as the commit message admits. If =
you copied the documented union to the native type declaration on the =
parameter, it would immediately reject nulls, because that has always =
been the behaviour of user-defined functions. Static analysis tools will =
also have complained that null was not a valid input according to the =
documentation.


To confirm the order of events:

First, the Docblock originally said this function did not accept NULL, =
but at runtime it accepted/coerced NULL to an empty string. This is =
exactly how `htmlspecialchars()` worked pre 8.1. Where developers using =
static analysis tools can choose to treat NULL as an invalid value, and =
those tools could report nullable variables as an error (via strict type =
checking).

Second, the Docblock and implementation was updated to allow NULL, =
because NULL is common value (a backwards compatibility issue), so this =
HTML encoding function, used by Blade templates by default, now accepts =
NULL.

This seems to undermine the argument that NULL should not be passed to =
`htmlspecialchars()`.

I did suggest updating the ~335 function parameters to be nullable; but =
that was rejected because some developers prefer to treat NULL as an =
invalid value. To keep those developers happy, and avoid the backwards =
compatibility issue, it seems easier to allow NULL to be coerced (like =
other contexts, e.g. string concat).



> I also note that the commit message says "On PHP >=3D 8.1, an error is =
thrown if `null` is passed to `htmlspecialchars`." which is of course =
not true for native PHP, only if you make the highly dubious decision to =
promote deprecations to errors.


While I'd put the word "error" down as a typo, the intention is for 9.0 =
to throw a type error.

And while user-defined functions are part of the conversation (for =
consistency reasons), I'm trying to find the benefits of breaking NULL =
coercion for internal functions (because, if there is an overall =
benefit, that Laravel Blade patch should be reverted).



> As an anecdote, I was recently working on a bug involving nulls =
causing unintended behaviour in an API. As part of the clean-up, I =
changed a function signature from getByIdentifer($identifier) to =
getByIdentifer(string $identifier), and during testing, got an error =
because I'd missed a scenario where null was passed. This was a good =
result - it prevented the broken behaviour and alerted me to the case =
that needed fixing. If the parameter had instead been silently coerced =
to an empty string, I would have got neither an error nor the original =
null behaviour, causing a new bug that might have taken much longer to =
track down.
>=20
> If your RFC as drafted was implemented, I could only have achieved the =
desired check by turning on strict_types=3D1 for whole sections of the =
application, which would probably require dozens of other fixes, or =
writing an ugly manual check:
>=20
> function getByIdentifier(?string $identifier) {
>     if ( $identifier =3D=3D=3D null ) {
>          throw new TypeError("Function doesn't actually accept =
nulls");
>     }
>     // ...
> }


It sounds like you got lucky - you have a function that has a problem =
with NULL (but I assume it's fine with an empty string?), and during =
your testing you happened to pass NULL to this function. As noted =
before, static analysis is *considerably* better at these types of =
checks, because it's able check if variables *can* contain NULL. They =
can also perform other checks as well (important when your code seems to =
care about NULL vs an empty string).



> As I have said previously, I share your concern about the impact of =
the currently planned change, but I don't think loosening the existing =
type rules on user-defined functions is a good solution.


If you have a better solution, please let me know.

I don't think breaking NULL coercion for internal functions helps (the =
change seems to involve loads of tiny updates, probably by littering =
projects with `$val ?? ''`, `strval($val)`, or `(string) $val`, with =
hard to define benefits); in contrast, allowing NULL coercion for =
user-defined functions would at least keep user-defined and internal =
functions consistent, with NULL coercion continuing to work in the same =
way as other contexts (e.g. string concat, =3D=3D comparisons, sprintf, =
etc); or how coercion works between string/int/float/bool.

That said, I would still like to know about the benefits of rejecting =
NULL for `htmlspecialchars()`, in the context of that Laravel Blade =
patch; because, if it is beneficial (vs the BC break), they should =
revert that patch... shouldn't they?

Craig