NULL Coercion Consistency

Hi,

https://wiki.php.net/rfc/null_coercion_consistency

Thanks for the draft.

On Sat, Apr 9, 2022 at 11:30 AM Craig Francis craig@craigfrancis.co.uk
wrote:

On Fri, 8 Apr 2022 at 19:07, Craig Francis craig@craigfrancis.co.uk
wrote:

In particular, does this propose changing user-defined functions under
strict_types=0 to accept null for scalar types?

With user defined functions, I think it's up for debate (still a draft),
but I think those NULL's should be coerced to the specified type (as
documented), where I don't think PHP should be doing type checking under
strict_types=0.

I've updated the RFC to note that user-defined functions don't cause a
backwards compatibility issue; but I have added an example to highlight
the coercion inconstancy:

You've updated the Documentation section (also: did you mean
"inconsistency" rather than "inconstancy"?) but still not the Proposal
(BTW all those sections between "Introduction" and "Proposal" would
probably better be sub-sections of a section named "Problem" or "Current
State" or something).

And for the question: (currently) the RFC is named "NULL Coercion
Consistency" and the Proposal says "Must keep the spirit of the original
RFC, and keep user-defined and internal functions consistent.", which (to
me) implies not only reverting the 8.1 deprecation on internal functions
but also "changing user-defined functions under strict_types=0 to
[coerce] null for scalar type[ declaration]s" indeed.
In any case, that should be written clear in the RFC (either in "Proposal"
or "Open Issues").

Regards,

--
Guilliam Xavier

This doesn't only apply to end user developers, but also to library and
toolchain developers who need to maintain code covering a range of PHP
versions

Yep, and thank you for commenting.

Library authors are the ones receiving pressure at the moment, and while
users of the library sometimes help (e.g. pull requests), it's a fairly
thankless and time consuming task, with little/no value for scripts not
using strict_types=1 (you're not the only one simply adding ?? ''
everywhere).

Just one thing to add, this is from people who are complaining about a
deprecation notice (which can and is being ignored), now think of the
typical non-library code when it becomes a Fatal Error.

Craig

You are taking parts of the documentation out of context, and omitting
the start of the whole "Converting to string" section:

"A value can be converted to a string using the (string) cast or the
strval() function. String conversion is automatically done in the scope
of an expression where a string is needed. This happens when using the
echo or print functions, or when a variable is compared to a string. The
sections on Types and Type Juggling will make the following clearer."

I'm sorry, I've read this several times now, and I'm not sure what this
adds. My RFC simply quotes the paragraphs that explain how null is coerced
(other than the small abbreviation for booleans, those paragraphs are
included in their entirety), I don't think it needs any more context than
that, it's not like I'm mis-representing how coercion works (and is used)
in PHP.

I could expand my first quote to include "... For example, a function that
is given an int for a parameter that expects a string will get a variable
of type string", but I don't think it's that useful.

In the same section it is also defined how resources and arrays get
converted to strings. Following your argument, you should be able to
pass arrays and resources to a string parameter because there is a clear
definition in the documentation of what then happens.

No, my RFC is about the problems this change has caused, how it is making
it difficult to upgrade. I'm only referencing the documentation to confirm
how NULL has always been coerced.

I unfortunately really don't like the direction of your RFC, because I

see null as a real type and because it increases the divide between
strict_types and not using strict_types. I have never used strict_types

• in modern PHP code I find it unnecessary, if you use parameter,
  property and return types. I am glad PHP has decreased the difference
  between using strict_types or not, so your direction of trying to
  increase the difference seems like the wrong way to go. Ideally I would
  rather want to see a future where strict_types can be removed/reconciled.

You can see NULL however you like, but most developers do not share that
view. NULL has been passed to these functions, since, well, forever; and
changing code to manually convert NULL to the relevant type is incredibly
time consuming, and of questionable value (e.g. when developers simply add
strval() everywhere).

Either way, I'm not trying to "increase the difference" (this is how it has
always worked). If you want type checking, good, it can be really useful,
and I recommend it with static analysis... but it's enabled with
strict_types=1.

Craig

https://wiki.php.net/rfc/null_coercion_consistency

I've spent a large amount of time making coercive typing mode more
sensible and aligning the behaviour as close to reasonably possible with
strict_types so that the possibility of dropping strict_types all together
wouldn't be outrageous.
I've written about this elsewhere and on this list already but main points
are located here: https://github.com/Girgias/unify-typing-modes-rfc

Thank you George, I do appreciate your work on this, I agree with pretty
much everything you have written, and I really do want PHP to move in that
direction.

The only thing I disagree with is NULL coercion, because it has worked
perfectly well for years, and is used so often.

Userland scalar types, however they would have been introduced, did not

include coercion from NULL for very good reasons.
Wanting to change this behaviour needs more justification than a paragraph
in docs, as the docs should reflect the reality of the implementation.

Some people see NULL as an invalid value, which can sometimes be useful in
a strict_types=1 environment, but that's the only "good reason" I can
find... it's why I changed my mind about making some parameters nullable,
and took a different approach with this RFC.

But most developers do not use NULL like that, and that's why it's passed
so often to the ~335 function parameters I've noted (most of them are on
the list because they realistically receive user values, which can be NULL
in most frameworks).

The second aspect is the general consensus that internals and userland

should behave identically.

Yep, they really should... it's why I said we "Must keep the spirit of the
original RFC, and keep user-defined and internal functions consistent".

Moreover, the fact that we are deprecating this and not just changing the

behaviour from one version to another is that we do recognize PHP
developers use NULL in this way, and we are giving them advance notice of
the change.
Also a deprecation notice should, IMHO, never be promoted to an exception.

Unfortunately deprecation notices are being ignored (as noted in my RFC,
with examples on how); and there is the intention to use Type Error
Exceptions in 9.0 (the thing I'm trying to avoid).

I'm approaching this from a security point of view - the last thing I want
to do in ~5 years is auditing code/systems where they have stuck with 8.x
because it's too hard to upgrade.

There are behaviours of PHP that will probably never be "fixable", but this

should not prevent us from improving the aspects we can.
And if we only focused on how developers use PHP we would still have
register globals, magic quotes, etc.

I want the language to be better as well, and I was very glad to see the
back of Register Globals and Magic Quotes (I know they were a pain to
remove, but it was well justified, as they caused so many problems).

Clearly it seems you won't listen to why these changes were made and want

to reverse course, therefore good luck convincing enough people to accept
such an RFC.

I have been listening/reading (it's why I've taken about 2 hours to read
what you have written, and reply). I have also been discussing this with
different people (on and off list)... unlike the original RFC discussion,
where it was only Craig Duncan that mentioned this issue:

https://externals.io/message/112327#112531

And it's because I've been listening to people that I changed my mind, and
created this new RFC (as noted above).

I think I've said all there is to say and won't interact further with any

such proposals.

That's a shame, as I'd like to discuss solutions to this problem you would
be happy with.

Craig

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

Hey Craig

Hi Marco,

Thanks for your thoughts.

Not what I'm going for... but anyway, to get an idea of your position, do you think the string '15' should be coerced to a number, or should it fatal error as well? e.g.

$my_number = round($request->get('my_number'));

Passing '15' for int should throw a type error: this is why people (slowly, steadily) opt into strict types.

I don't think everyone agrees :-)

I mostly refrained from commenting on this RFC thus far exactly because I'm already "out of the woods", and I already add strict types everywhere.

Still, when upgrading legacy PHP apps, I would have to deal with the problem, at which point I prefer looking for type errors in internal function calls, than looking for changes in null coercion (the one proposed here) everywhere.

Practically, my idea is:

• coercion rules are old and well known
• changing coercion rules around null drags in complexity
• focus should be in migrating to strict types instead

Regardless of the output of this RFC, people upgrading to 8.0 or 8.1 will already experience BC breaks in internal function input types, and adapt to them.

I will note that my RFC does not change NULL coercion (why I quoted the documentation), and I'm not against the other BC breaks where certain type coercions are clearly problematic.

It was 8.1 which introduced this specific BC problem (with ignorable deprecation notices)... and while that change was to begin alignment with user defined functions, it's the user defined functions that have been the oddity (i.e. do not use NULL coercion, unlike other contexts, such as concatenation, == comparisons, arithmetics, sprintf, print, echo, array keys). Personally I think it should have been user defined functions that worked with the "old and well known" coercion rules, and doing so would have reduced complexity (i.e. working in the same way).

I'm also not against increasing strictness, and some of the type errors (ref previous emails, and noted in my RFC), but I think it's more of a balance, where an overly strict environment creates different problems (e.g. making the language much more verbose, and can be unnecessarily hard to learn/use).

But my focus is on upgrading existing code, and this one is difficult to find and update (e.g. the lack of tooling to help).

Craig

On Sat, May 7, 2022 at 2:11 PM Jordan LeDoux jordan.ledoux@gmail.com
wrote:

I'm concerned from your replies in this thread and the content of the RFC
that you don't actually understand the extent of a BC-break you're
proposing.

Sorry, that was a tad more combative than I intended. What I'm trying to
say is that I'm concerned there may be a blindspot here about the impact
which could significantly impact developers in ways that aren't being
accounted for.

Jordan

What exactly would be the purpose of ?int if this RFC was passed? To pass
the value as null instead of 0? That's it?

Yes. No change here.

What about int|float? Which one would it be coerced to?

What happens if you pass FALSE to such an argument? int(0). The same
would happen with NULL.

This pretty radically changes how typing itself
would work in existing user programs. What I'm saying is that for such a
radical BC break you need to provide some compelling justification.

If coercion is possible it should be done as it is done for
bool/int/float/string. Inside the function you can still trust the
variable type is as expected. The only difference is that it will not
throw an error on NULL. Right now you can still use your function
"incorrectly" and not get an error, e.g. if you pass FALSE to it (I
understand NULL might be more common). And, if you use strict_types=1
nothing changes for you.

I don't see this as a radical change. Definitely not more radical than
making the code to throw errors where they were not thrown before (no
matter how long the deprecation period was). So, imo the BC-break
argument is not that strong in this case.

The consistency argument is also not that strong (but here probably more
in favor of the change) because as it was mentioned already current
behavior is consistent with some things and inconsistent with others.

The essential question is whether we want more of "weak-typing coercion"
or not. More would mean that there's no breakage for existing code in
PHP9 (essentially only 8.1 would be "broken" (because of the deprecation
notice) so people could just skip it). No need for ugly code like "$var
?? ''" everywhere is also a win.

Ok, this was my take, now I will shut up.

--
Aleksander Machniak
Kolab Groupware Developer [https://kolab.org]
Roundcube Webmail Developer [https://roundcube.net]

PGP: 19359DC1 # Blog: https://kolabian.wordpress.com

Not what I'm going for... but anyway, to get an idea of your position, do you think the string '15' should be coerced to a number, or should it fatal error as well? e.g.

$my_number = round($request->get('my_number'));

'15' is not an undefined value.

If a program uses the paradigm that null represents the equivalent of something like unset($var)

Hi Jordan,

Some programers can do that, but it's not how everyone sees it.

NULL is not the same as unset(), it is a value in itself, and many programmers simply give no thought to the distinction between an Empty String and NULL. e.g. when using the noted frameworks, NULL is provided for user values, and developers will often treat it the same as an Empty String... if $name == '', then show an error saying your name is required; if strlen($name) > 200 then your name is too long, etc.

What exactly would be the purpose of ?int if this RFC was passed? To pass the value as null instead of 0?

I'll talk about int below; but just so I can focus on the nullability part of this question, I'll use ?string...

Yes, that's all I'm proposing.

For example ?string would not change (it will continue to provide the NULL value to the function), whereas string would coerce NULL to an Empty String, e.g.

function accept_nullable_string(?string $my_string) {
  var_dump($my_string);
}

function accept_string(string $my_string) {
  var_dump($my_string);
}

accept_nullable_string(3);     // string(1) "3"
accept_nullable_string('2');   // string(1) "2"
accept_nullable_string(true);  // string(1) "1"
accept_nullable_string(false); // string(0) ""
accept_nullable_string(NULL);  // `NULL` 

accept_string(3);     // string(1) "3"
accept_string('2');   // string(1) "2"
accept_string(true);  // string(1) "1"
accept_string(false); // string(0) ""
accept_string(NULL);  // Currently TypeError, change to string(0) ""

That's it?

Yep, but the point is about avoiding the upgrade problem when internal functions will stop coercing NULL, and instead throw type errors (these are hard to find, and will break code in seemly random places).

What about int|float? Which one would it be coerced to?

Good question,

Short answer; today, if you pass the string '4' to a function that uses int|float, then it receives int(4):

function accept_number(int|float $my_int) {
  var_dump($my_int);
}

accept_number('4'); // int(4)

Longer answer...

First I'll note the documentation says (for string to integer conversion) "If the string is numeric or leading numeric then it will resolve to the corresponding integer value, otherwise it is converted to zero (0)."

https://www.php.net/manual/en/language.types.integer.php#language.types.integer.casting https://www.php.net/manual/en/language.types.integer.php#language.types.integer.casting

But an Empty String is not coerced to int(0) for user defined functions, or arithmetic (e.g. 5 + "" complains after PHP 7.1)... whereas, an empty string is converted to 0 when using intval($var), and (int) $var.

While I think this can be a bit harsh, I can see how it might avoid some ambiguity/issues, e.g. $offset = ''; $a = substr('abc', $offset), although I have seen substr('abc', NULL, $length) a few times.

If PHP was to throw an exception for NULL to integer coercion with internal functions, while I recognise this would still create a BC break (e.g. round($nullable)), I'd might be fine with it, because an Empty String to Integer is also rejected.

That said, and back to your question about int|float, because the string '4' is coerced to int(4), I'd prefer NULL to be coerced to int(0).

This pretty radically changes how typing itself would work in existing user programs.

I wouldn't say this is radical, considering it's just removing a single type error (I suspect it's rare for code to rely on NULL coercion triggering an exception)... in contrast, internal functions have always worked with null coercion, and works in other contexts like string concatenation ('A' . $nullable), == comparisons, the print()/echo() functions, etc.

What I'm saying is that for such a radical BC break you need to provide some compelling justification, and I'm concerned from your replies in this thread and the content of the RFC that you don't actually understand the extent of a BC-break you're proposing.

As noted previously, and the example I've added to the "Backward Incompatible Changes" section, I don't think this is a radical BC break, it's fairly small change designed to make NULL coercion work as documented, in a constant way (all contexts), and most importantly, avoid the upgrade problems for PHP 9.0 (please keep in mind the lack of tooling to help with this, where the only safe and easy way to handle this would be to add NULL coalescing throughout the code base, which is not a good idea).

Craig

On Thu, May 26, 2022 at 7:21 AM Craig Francis craig@craigfrancis.co.uk
wrote:

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?

No, they should not. Laravel made an explicit choice to handle null values
in its escaping function for its templating framework. That is their
choice to make. They should not be forced to implicitly handle null values
because the language decides to add implicit type coercion to user defined
functions (because consistency seems to be so important to some), and they
should not be forced to reject null values because underlying language
functions reject them as well.

Every implicit type coercion is a bug in hiding IMO. If something doesn’t
explicitly support null, you shouldn’t have been passing null to it in the
first place. Relying on magic language behaviors to prevent type errors is
not a long term sustainable code pattern. If a function, by explicit
design, can safely work with null values then that parameter should be
properly declared nullable. If a function, by explicit design, does not
support null values then a type error is a hugely acceptable response.
Yes, that means that there is a lot of work involved for folks between now
and the release of PHP 9.0 to either refactor code to avoid type errors or
to get the language to expand the supported types in appropriate functions,
but IMO that effort from all parties is worth it in the long run to ensure
language consistency (I really don’t think userland PHP and
internal/extension PHP should have vastly different behaviors, and type
coercion support based on where a function is designed is one of those
holes that needs filled) and to ensure all APIs explicitly declare what
types of data they support.

• Michael Please pardon any errors, this message was sent from my iPhone.

On Thu, May 26, 2022 at 5:21 AM Craig Francis craig@craigfrancis.co.uk
wrote:

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).

Nearly all code has a problem with null. It very much feels like the
original effort to deprecate null calls decided to resolve this by saying
"let's have the language help developers improve their code so it doesn't
have these problems in the first place", and this effort is trying to
resolve this by saying "let's have the language support the buggy code in
ways that makes it work".

At my job, my task for the last three weeks has literally been upgrading
our internal codebase for 8.1, and the biggest set of logs I'm dealing with
is exactly what you're talking about here: null's passed to internal
functions. Every single case I've looked at so far has been traced to code
that was written incorrectly, where some code somewhere was not properly
guarding its values, and error cases were slipping through.

Jordan

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).

The same events can be given a very different narrative:

Originally, this function was not intended to accept null. The
documentation clearly stated the accepted types, and any static analysis
tool would reject any value not in that list. The author of the function
chose to treat NULL as an invalid value.

Because the list of valid types wasn't enforced at run-time, it was
accidentally possible to pass null. Many developers who weren't using
additional tooling came to rely on this bug, so as a compromise, the
authors decided to change the documented behaviour of nulls.

At no point did anybody look at the function and say "I can safely pass
null to this, as long as I remember never to use static analysis tools".
They accidentally passed null, and by luck got a useful result.

I also note that the commit message says "On PHP >= 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.

My point is that there is no action required on PHP 8.1. The commit
message should have said, "On PHP >= 9.0, an error will be thrown if
null is passed to htmlspecialchars."

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).

This is a complete non sequitur. It's perfectly possible for different
scenarios to support different decisions, and what Laravel decides that
particular function should accept is based on their own assessment of
the trade-offs. PHP is free to make an entirely different decision based
on entirely different trade-offs.

Meanwhile, the benefits have been explained repeatedly in this thread.
You may not agree that they are worth the cost, and as I've repeatedly
said, I have some sympathy for that. But please stop trying to take the
conversation back to the very beginning by implying that you've asked a
question and not received an answer.

Regards,

--
Rowan Tommins
[IMSoP]

For this specific example, shouldn't it rather [already] be like this anyway?

function getByIdentifier(string $identifier) {
if ( $identifier === '' ) {
throw new InvalidArgumentException("Empty identifier");
}
// ...
}

(but I guess you could find actual examples where an empty string is
"valid" and null is not, indeed)

The actual code in this case ended up in a generic routine that used
isset() to choose which SQL to generate. An empty string would generate
a WHERE clause that matched zero rows, but a null would omit the WHERE
clause entirely, and match all rows. So an extra pre-validation on the
string format might be useful for debugging, but wouldn't result in
materially different results.

I'm just worried to see people rather disabling deprecation notices
(via error_reporting, or a custom error handler, or even patching
PHP's source and recompiling their own) than "fixing" the code
(granted, that's not specific tothis RFC, but somewhat "highlighted" here)

Indeed, I think we have a general problem with how deprecations are
communicated and acted on in general. I have been thinking about how to
improve that, other than "never change anything" or "never warn people
we're going to change anything", and will try to write up my ideas soon.

function null_to_empty_string(?string $string_or_null): string
{ return $string_or_null === null ? '' : $string_or_null; }

(but also its "opposite" empty_string_to_null(string $string): ?string)

That's actually an interesting observation. It's probably quite common
to treat empty strings as null when going from input to storage; and to
treat null as empty string when retrieving again. Importantly, databases
generally don't treat them as equivalent, so forgetting that
translation can be a real cause of bugs. I often advocate for string
columns in databases to allow either null or empty string, but not both
(by adding a check constraint), so that such bugs are caught earlier.

To go back to Craig's favourite example, that could be a genuine problem
caused by passing null to htmlspecialchars() - if we intended that null
to be stored as such in the database, we've silently converted it into a
non-equivalent empty string. (Yes, escaping should be done on output not
input, but it's not completely infeasible that that combination might
happen.)

Regards,

--
Rowan Tommins
[IMSoP]