Proposal for RFC to remove undefined array index warning when used in a ternary View as Markdown

Hello internals!

There is a lot of code in existence that does something like this:

$item['id'] ? [$item['id']] : $something_else

which to dissolve the current warning must be changed to something like
this:

!empty($item['id']) ? [$item['id']] : $something_else

However, if performance is an issue, we can do this in one less opcode (in
fact, the same number of opcodes as the original):

empty($item['id']) ? $something_ else : [$item['id']]

As far as I can tell, when single array access is the only test in a
ternary operation, it is exactly the same semantics as wrapping the array
access in a "!empty()". Fixing this warning seems like a lot of "busy work"
with little to no tangible benefit.

I'd like to propose and implement an RFC that would consider dropping the
warning if and only if array access is the only test in ternary and short
ternary (?:) operations but first I'd like to start a discussion to see if
it is worth pursuing. I'd like to target PHP 8.2 or 8.3.

It may also be worth discussing extending it to multiple array accesses
because to invert the not empties is counter-intuitive and error-prone:

!empty($a['test']) && !empty($a['ok']) ? 'a' : 'b';

becomes:

empty($a['test']) || empty($a['ok']) ? 'b' : 'a';

which was before (and IMHO, more readable):

$a['test'] && $a['ok'] ? 'a' : 'b';

This is very tricky to get right and to be fair, there will probably be
automated tools (if you trust such things on large codebases). There are
also performance issues to consider, as some basic benchmarks show
ISSET_ISEMPTY_DIM_OBJ is 6x slower than FETCH_DIM_R.

Robert Landers
Software Engineer
Utrecht NL

I'd like to propose and implement an RFC that would consider dropping the
warning if and only if array access is the only test in ternary and short
ternary (?:) operations but first I'd like to start a discussion to see
if
it is worth pursuing. I'd like to target PHP 8.2 or 8.3.

The warning comes from the array key being missing, not from being
empty. There are many values which are empty which do not emit the warning.

If you want to provide a default should the array key be missing you
want null coalesce.

$x = $array['foo'] ?? 'somethingelse';

This is not semantically the same though. A $_POST of a form, for example,
will usually contain an empty value if the form input was empty, but
missing if the form control wasn't in the form (maybe the form control is
injected via Javascript, or conditionally output by the script).

Here is how the code might look currently to handle a default value for an
empty form post that may or may not be sent in the request (ignoring how
insecure this looks):

$name = $_POST['name'] ?: 'Default Name';

Semantically the same to get rid of the warning:

$name = empty($_POST['name']) ? 'Default Name' : $_POST['name'];

I'm not sure your suggestion to set a default applies since we have to do
the empty check anyway, which swallows the warning?

$name = empty($_POST['name'] ?? 'Default Name') ?: 'Default Name';

--

To unsubscribe, visit: https://www.php.net/unsub.php

Robert Landers
Software Engineer
Utrecht NL

This is not semantically the same though. A $_POST of a form, for
example,
will usually contain an empty value if the form input was empty, but
missing if the form control wasn't in the form (maybe the form control is
injected via Javascript, or conditionally output by the script).

PHP is a general purpose language, the behaviour you're asking for is
specific to one use case and may be of detriment to others, without
offering significant benefit in exchange.

I suggest you write yourself a function that performs the operations for
you. This may also be a good opportunity to move away from directly
accessing the superglobals in userland code.

$form->get('foo', 'defaultgoeshere');

I gave that as an example, not a literal line from real code, just to show
how common the situation is. Handling data structures like YAML, JSON, or
parsing virtually any file format will run into this issue where you want
to set a default if the value is falsy or non-existent. I've written a lot
of this parsing-type of code over the years and run into this case at least
once each time.

PHP is a general purpose language, the behaviour you're asking for is
specific to one use case and may be of detriment to others, without
offering significant benefit in exchange.

I'd also venture that this warning has caused more harm than good, in that
writing "$var['something'] ?? null" is second nature when writing new
code, even if there is practically no chance for a non-existent key. This
requires reviewers and authors to go digging into the source of the array
and trace it to its usage, just to figure out whether or not a key can be
non-existent under certain conditions (or discover a warning in
production). In my experience, arrays are usually derived from user/machine
input, which is impossible to predict the exact shape (outdated clients,
malicious users, etc.) and the warning becomes noise with enough traffic.

Perhaps a non-existent key should result in null by default (without a
warning, but maybe a notice) unless you want a different value (using "??")?

Performance concerns would be so so small they would be practically
undetectable in the context of a real request.

Also, this:

$name = empty($_POST['name'] ?? 'Default Name') ?: 'Default Name';

Should be:

$name = ($_POST['name'] ?? null) ?: 'Default Name')

--
Mark Randall

--

To unsubscribe, visit: https://www.php.net/unsub.php

I'd also venture that this warning has caused more harm than good, in
that
writing "$var['something'] ?? null" is second nature when writing new
code, even if there is practically no chance for a non-existent key.

Using null coalesce should only be used when you know you have the
possibility of a missing key.

That can't always be known, or requires tracing the stack to see where
it comes from and what modifies the array before it is passed to the
function you are modifying, it is an array after all, not a formalized
object with well-defined properties (which I'd prefer, but we can't always
change what someone did 15 years ago just for the sake of changing it).

For everything else you've probably entered an unexpected state and
should display a warning (that should ideally be picked up by an
exception handler and convert it to an exception).

Not always, and not in cases where people are just coalescing null into
null to avoid a warning.

When handling external data it is usually a good idea to use a
validation step before using it, such as JSON schema.

I don't usually write code dealing with handling external API requests in
my day-to-day work, so I can't really speak to JSON handling, specifically.
I have had to unserialize objects from JSON files from other internal
systems but that was back when there was only the Elvis operator (PHP
5.6ish), so it was used quite liberally.

An even better idea is to parse it into a specific type, rather than
just validating it, at which point you're going to be using a lot of
isset, null coalesces, array_key_exists, property_exists etc anyway and
sucking in some default other than null is almost always the wrong thing
to do.

I'm only suggesting this apply to ternary tests, not in general, so that
code can be easier to read and grok. I believe null is a sensible default
in cases when you are only establishing the truthiness of a value, and
whether or not the key exists is irrelevant (otherwise there's the null
coalesce operator and isset()). There may be times when you need more
validation, but those aren't usually handled in ternaries. In other words,
I believe this should still cause a warning if it is unset: $arr['unset']
?: $arr['unset']; because it is likely a bug and used outside of the
ternary test (the part before the "?" -- I'm not sure what the
technical name is for it). This should not, however: $arr['unset'] ?: 'some
default'; because it is only used in the test.

I also like the suggestion of creating some null-safe operator for arrays,
which might be a more general solution.

As to your final point: PHP internals voted by a supermajority to raise
this from a notice to a warning in PHP 8, it seems unlikely that 33% of
people are now going to change their mind and vote for the opposite.

We've had to live with it for a little while now and we have a pretty good
idea of what it is doing to codebases and the time it takes to "fix"
codebases by null coalescing null to null. I tried searching GitHub for
this, but it looks like the search functionality removes the question marks
in my query.