Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:117551
MIME-Version: 1.0
References: <CAFv4g+Ej4JbM9SzQ0iU-d8C4T+2XNo=8jnJ1uwfQ=skZZXx9Tg@mail.gmail.com>
 <42D0A480-F262-4F72-9C4D-887762A8D800@gmail.com> <CAFv4g+ELZ5ZA6zyDFHCJCM6vSy3MFTvEGS9=zo5ZM1izHQWuFw@mail.gmail.com>
 <0b061f28-a087-efd3-8602-424ee03458e0@gmail.com>
In-Reply-To: <0b061f28-a087-efd3-8602-424ee03458e0@gmail.com>
Date: Wed, 20 Apr 2022 18:02:09 +0100
Message-ID: <CAFv4g+HROPPx4hT226ypbKZwM_7nq31jJ6+X+RNq_THZDVrJPA@mail.gmail.com>
To: Rowan Tommins <rowan.collins@gmail.com>
Cc: internals@lists.php.net
Content-Type: multipart/alternative; boundary="000000000000e4e64705dd18f4db"
Subject: Re: [PHP-DEV] NULL Coercion Consistency
From: craig@craigfrancis.co.uk (Craig Francis)

--000000000000e4e64705dd18f4db
Content-Type: text/plain; charset="UTF-8"

On Tue, 19 Apr 2022 at 14:17, Rowan Tommins <rowan.collins@gmail.com> wrote:

> On 19/04/2022 12:34, Craig Francis wrote:
> > The developers I work with would assume the last definition
>
>
> I think you've somewhat missed my point. I wasn't talking about people's
> habits or preferences, I was talking about different *scenarios* where
> null is used to mean different things.
>


Yep, and I'm thankful you listed them... I'm just trying to focus on how
PHP has worked (implicitly based on requirements), and how changing that
causes problems (and will be an even bigger problem when it becomes a Fatal
Type Error).



Yes, some people prefer languages that "fails early" and some are more
> interested in "do what I mean", but not everything is about that.
>


Agreed, the only thing I'd add... failing early with NULL might help debug
some problems (assuming there is a problem), but I believe static analysis
and unit tests are much better at doing this (e.g. static analysis is able
to see if a variable can be NULL, and apply those stricter checks, as noted
by George with Girgias RFC).

In contrast, failing early at runtime, on something that is not actually a
problem, like the examples in my RFC, creates 2 problems (primarily
upgrading; but also adding unnecessary code to explicitly cast those
possible NULL values to the correct type, which isn't needed for the other
scalar types).




> > I also cannot explain why NULL should be rejected ... does it avoid
> > any bugs?
>
>
> Yes, sometimes. Imagine an array of values provided by the user; during
> validation, those which were optional and not provided get set to null.
> You then loop through and display those which were provided:
>
> foreach ( $fields as $name => $value ) {
>      if ( $value !== null ) {
>           echo "<strong>$name:</strong> $value <br>\n";
>      }
> }
>
> Then, you realise you forgot about escaping, and decide to run
> everything through htmlspecialchars():
>
> $htmlFields = array_map('htmlspecialchars', $fields);
> foreach ( $htmlFields as $name => $value ) {
>      if ( $value !== null ) {
>           echo "<strong>$name:</strong> $value <br>\n";
>      }
> }
>
> Spot the bug? $value will now never be null, because htmlspecialchars()
> will silently turn the nulls into empty strings.
>


Thank you... but I will add, while `htmlspecialchars()` rejecting NULL
would get you to look at the code again, I wouldn't say it's directly
picking up the problem, and it relies on there being a NULL value
in $fields for this to be noticed (if that doesen't happen, you're now in a
position where random errors will start happening in production).

This is where I'd note the value of unit tests, as they are much better
placed to check this feature.

Bit of a tangent, I'm uncomfortable that `$name` is not being HTML encoded,
which takes us to context aware templating engines, and how you can
identify these mistakes via the `is_literal` RFC or the `literal-string`
type.



I've also seen the opposite problem: a string function was removed
> because it was no longer needed, and the code broke because values,
> including nulls, were no longer being cast to string.
>
>
> Is protecting against this worth the backwards compatibility cost of
> changing the behaviour, and requiring extra code in other scenarios?
> Possibly not. But that's different from not having any benefit.



That's fair, adding in an extra check might give some benefit in some
cases, but I don't think it's reliable, or worth the cost in updating
existing code (with no tooling to help) and the additional code that will
be needed to cast these possible NULL values to the relevant type (which
isn't needed for the other scalar types).

That said, if you (or anyone) have any better ideas on how to address this
problem, please let me know.

Craig

--000000000000e4e64705dd18f4db--