Everything

php

Setting to disable the "Undefined array index" warning

php.internals

Nicolas BADIA

4 years ago

Hi,

As it is explained in this ticket https://bugs.php.net/bug.php?id=81417 we use to check if a property exists by accessing it directly like we do in JavaScript.

Personally, I subscribe to this coding style and we use it all over our codebase (more than 130 000 lines of PHP code). When it became a notice, we disabled them, but now that it is a warning, it is a lot more problematic for us… We can’t even use the last version of PHP Debug for VSCode.

As I’m not ready to spend weeks upgrading our codebase because our coding style is not supported by the community, I’m looking at alternatives.

The more obvious and simple for me would be to add a setting in php.ini to allow this. So I’d like to know if it is something you would support.

If not, I guess I will have to fork PHP, but this will complicate a lot our deployment process and make our code not portable. As I believe we are not the same in this case, I hope you will support the addition of a setting…

Thanks in advance for considering this.

Best regards,

Nicolas

Thomas Nunninger

4 years ago

Hi Nicolas,

as far as I understand, adding new ini settings is not the way to go as
this would mean that your code behaves differently on different
environments.

Two suggestions:

* Write your own error handler that ignores those errors.

* Try some tool like Rector to rewrite your code.

Best regards
Thomas

Am 15.02.22 um 13:31 schrieb Nicolas BADIA:

Pierre

4 years ago

Le 15/02/2022 à 13:46, Thomas Nunninger a écrit :

> Hi Nicolas,
>
> as far as I understand, adding new ini settings is not the way to go
> as this would mean that your code behaves differently on different
> environments.
>
> Two suggestions:
>
> * Write your own error handler that ignores those errors.
>
> * Try some tool like Rector to rewrite your code.
>
> Best regards
> Thomas

Hello,

I do agree with Thomas here, changing PHP internal behavior by
configuration is a dangerous thing to do.

If I understand correctly, there is "more than 130 000 lines of PHP
code" to fix, believe me I migrated a few projects along the way from
PHP 5.6 to 8.0 over time (one was about this size, but it stopped at PHP
7.0 a few years back). They probably contain less "legacy code" since we
have strict conventions from the beginning, but we did migrate some
projects still.

Fact is, a legacy project cannot run without evolving if you want to
support a recent PHP version. The language itself evolves, and that's
for the best. Now for really legacy unmaintained projects, there's still
the solution to keep them running on deprecated PHP version using some
container technology. It's not ideal, but it can help until you finally
find the necessary resources to port.

Using a good static analysis tool and possibly some other tools such as
Rector, 130k lines is not an impossible goal.

I don't think there's an easy solution here, it's either don't upgrade,
or either refactor, but modern tooling should help you a lot with that.

If it can help you, can can still replace your array access using `$a =
$foo['bar'] ?? null` instead of `$a = \array_key_exists('bar', $foo) ?
$foo['bar'] : null`, it's both easier to read and write (you just store
` ?? null` in your clip board and hit paste on every use case you find).

Hope it helps,

Regards,

--

Pierre

Andreas Leathley

4 years ago

On 15.02.22 13:31, Nicolas BADIA wrote:

> As it is explained in this ticket https://bugs.php.net/bug.php?id=81417 we use to check if a property exists by accessing it directly like we do in JavaScript.
>
> Personally, I subscribe to this coding style and we use it all over our codebase (more than 130 000 lines of PHP code). When it became a notice, we disabled them, but now that it is a warning, it is a lot more problematic for us… We can’t even use the last version of PHP Debug for VSCode.

The problem with your way of writing code is that it is ambiguous in
meaning, which is why this is a warning. You are not checking if a key
in an array exists, you are checking if an array value is "false-y". If
the value is an empty string, it is also interpreted as false, if it is
null, it is also interpreted as false, if it is an integer of value zero
it is also interpreted as false, if it is the string "0" it is also
interpreted as false.

When you write code as you do, it is easy to introduce subtle errors
because of these implicit casts. If you want to check if an array key is
defined, you should do it explicitly - this is not about a "coding
style", this is about writing the code in a way that actually does what
you intend it to do without it meaning multiple things of which some are
probably not expected. If you just want a quick fix without solving the
underlying problem, you can just add "?? null" to each array key access.

If you want to upgrade/improve your code base, which is a worthwhile
task, I would suggest the use of a static analyzer (like Psalm or
PHPStan) to find out where your code is ambigious. I have found many
undetected errors that way, where implicit casts have lead to unexpected
outcomes, which is why I am very much in favor of these warnings for
undefined array keys.

Rowan Collins

4 years ago

On 15/02/2022 12:54, Andreas Leathley wrote:

> The problem with your way of writing code is that it is ambiguous in
> meaning, which is why this is a warning.

I think that's a good way of looking at it. There's actually quite a lot
of code hiding a check like "if ($array['key'])"; roughly speaking, it's
short-hand for:

if ( array_key_exists('key', $array) && $array['key'] !== null &&
$array['key'] !== false && $array['key'] !== 0 && $array['key'] !== 0.0
&& $array['key'] !== '' && $array['key'] !== [] )

Now, if that's what you intended, there's a syntax that's slightly more
explicit while still being reasonably short: the empty() pseudo-function:

if ( ! empty($array['key']) )

On the other hand, if what you actually meant was this:

if ( array_key_exists('key', $array) && $array['key'] !== null )

Then you might have some bugs lurking, and actually want the isset()
pseudo-function instead:

if ( isset($array['key']) )

For other cases, you do have to spend a few more characters to be
explicit, often using the "??" operator; for instance, if you expect
$array['key'] to be either unset or a boolean, and want this:

if ( array_key_exists('key', $array) && $array['key'] === true )

Then the shortest is probably something like this:

if ( $array['key'] ?? false === true )


Regards,

--
Rowan Tommins
[IMSoP]

Guilliam Xavier

4 years ago

On Tue, Feb 15, 2022 at 3:07 PM Rowan Tommins <rowan.collins@gmail.com>
wrote:

> On 15/02/2022 12:54, Andreas Leathley wrote:
> > The problem with your way of writing code is that it is ambiguous in
> > meaning, which is why this is a warning.
>
>
> I think that's a good way of looking at it. There's actually quite a lot
> of code hiding a check like "if ($array['key'])"; roughly speaking, it's
> short-hand for:
>
> if ( array_key_exists('key', $array) && $array['key'] !== null &&
> $array['key'] !== false && $array['key'] !== 0 && $array['key'] !== 0.0
> && $array['key'] !== '' && $array['key'] !== [] )
>

Agreed, but you forgot the most "annoying":

&& $array['key'] !== '0'

> Now, if that's what you intended, there's a syntax that's slightly more
> explicit while still being reasonably short: the empty() pseudo-function:
>
> if ( ! empty($array['key']) )
>
> On the other hand, if what you actually meant was this:
>
> if ( array_key_exists('key', $array) && $array['key'] !== null )
>
> Then you might have some bugs lurking, and actually want the isset()
> pseudo-function instead:
>
> if ( isset($array['key']) )
>
> For other cases, you do have to spend a few more characters to be
> explicit, often using the "??" operator; for instance, if you expect
> $array['key'] to be either unset or a boolean, and want this:
>
> if ( array_key_exists('key', $array) && $array['key'] === true )
>
> Then the shortest is probably something like this:
>
> if ( $array['key'] ?? false === true )
>

Mind operator precedence! This is interpreted as:

if ( $array['key'] ?? (false === true) )

(obviously not intended), so you need explicit parentheses:

if ( ($array['key'] ?? false) === true )

I will also note that the "shorthands" (!empty(), isset(), ?? null) will
also "hide" another warning when the $array variable itself is undefined

Rowan Collins

4 years ago

On 15/02/2022 15:12, Guilliam Xavier wrote:

> On Tue, Feb 15, 2022 at 3:07 PM Rowan Tommins <rowan.collins@gmail.com>
> wrote:
>
>> if ( array_key_exists('key', $array) && $array['key'] !== null &&
>> $array['key'] !== false && $array['key'] !== 0 && $array['key'] !== 0.0
>> && $array['key'] !== '' && $array['key'] !== [] )
>>
> Agreed, but you forgot the most "annoying":
>
> && $array['key'] !== '0'

Ah yes, that one really demonstrates the dangers of type juggling in
general. If it _wasn't_ considered empty, that would be inconsistent
with how integer juggling works, but it definitely catches people out!

> Mind operator precedence! This is interpreted as:
>
> if ( $array['key'] ?? (false === true) )
>
> (obviously not intended), so you need explicit parentheses:
>
> if ( ($array['key'] ?? false) === true )

Oops, I admit I didn't test that. That does make the operator somewhat
less attractive, and maybe it would be more straight-forward to just be
explicit:

if ( isset($array['key']) && $array['key'] === true )

> I will also note that the "shorthands" (!empty(), isset(), ?? null) will
> also "hide" another warning when the $array variable itself is undefined

Ah, yes, so they do; as does the ?? operator, for that matter.

I seem to remember someone proposing an explicit operator for "this
array dimension might not exist", something like this:

if ( $array[?'key'] === true )

Which would translate to:

if ( (array_key_exists('key', $array) ? $array['key'] : null) === true )

Regards,

--
Rowan Tommins
[IMSoP]

Larry Garfield

4 years ago

On Tue, Feb 15, 2022, at 6:54 AM, Andreas Leathley wrote:

> On 15.02.22 13:31, Nicolas BADIA wrote:
>> As it is explained in this ticket https://bugs.php.net/bug.php?id=81417 we use to check if a property exists by accessing it directly like we do in JavaScript.
>>
>> Personally, I subscribe to this coding style and we use it all over our codebase (more than 130 000 lines of PHP code). When it became a notice, we disabled them, but now that it is a warning, it is a lot more problematic for us… We can’t even use the last version of PHP Debug for VSCode.
>
> The problem with your way of writing code is that it is ambiguous in
> meaning, which is why this is a warning. You are not checking if a key
> in an array exists, you are checking if an array value is "false-y". If
> the value is an empty string, it is also interpreted as false, if it is
> null, it is also interpreted as false, if it is an integer of value zero
> it is also interpreted as false, if it is the string "0" it is also
> interpreted as false.
>
> When you write code as you do, it is easy to introduce subtle errors
> because of these implicit casts. If you want to check if an array key is
> defined, you should do it explicitly - this is not about a "coding
> style", this is about writing the code in a way that actually does what
> you intend it to do without it meaning multiple things of which some are
> probably not expected. If you just want a quick fix without solving the
> underlying problem, you can just add "?? null" to each array key access.
>
> If you want to upgrade/improve your code base, which is a worthwhile
> task, I would suggest the use of a static analyzer (like Psalm or
> PHPStan) to find out where your code is ambigious. I have found many
> undetected errors that way, where implicit casts have lead to unexpected
> outcomes, which is why I am very much in favor of these warnings for
> undefined array keys.

As a data point, TYPO3 is a roughly 500,000 line code base.  (Non-comment lines of code as reported by phploc.)  It also relied very heavily on the old "silently ignore missing values and pretend it's a null" behavior, which broke badly in PHP 8.

It took one person (me) about 3 weeks I think of running unit tests, adding ?? in various places (or array_key_exists, or whatever made sense in context), running tests again, etc. to fix nearly all of them.  There's still a few that pop up now and again that didn't have test coverage, but they're rare.  And that's me doing it all 100% manually, no Rector or similar automation tools.

Yes, there is work required for this change.  However, with a code base 1/4 the size, and using better automation tools than I did you should be able to address all the upgrade issues in less than one person-week.

And that's without even getting into the question of array-centric code with properties being maybe-undefined is already a code smell, and has been since PHP 4.  (There's been a lot of very smelly code from the PHP 4 era, but it was smelly even then.)  Even without any (probably good) changes to the architecture or business logic of the application, this would improve the quality of the application.

I'd wager it's less work in terms of raw time to just fix up the code base than it is to write, implement, debate, pass, and merge an RFC to make you not have to do so.

--Larry Garfield

Nicolas BADIA

4 years ago

So I took some time to think more about it and spend some time starting upgrading the code to see the kinds of changes which would be needed.
Now, I think it is not possible to automate this. There are too many different cases and an automated script will add some ?? null everywhere even where it is not needed… I don’t want that.
After 2 hours, no bug found (but I generated one by translating the code) and I don’t find the code more readable. Here are some examples of changes:

if ($options['isCacheable'] === false)
if (($options['isCacheable'] ?? true) === false)

$twigVar = $pageClass->twigVars->$property;
$twigVar = $pageClass->twigVars->$property ?? null;

$data = $domains[$domain];
$data = $domains[$domain] ?? null;

$group = $data['group'];
$data = $data['group'] ?? null;

($options['masterAccount'] === 'only’)
(($options['masterAccount'] ?? null) === 'only')

if ($data['mysqlCache'] === false)
if (($data['mysqlCache'] ?? true) === false)

if ($data['allowCache'] !== false)
if (($data['allowCache'] ?? null) !== false)

if ($data['addRelations'] === true)
if (($data['addRelations'] ?? false) === true)

if ($options['ignoreMenu'] AND !$options['recursive']) {
if (!empty($options['ignoreMenu']) AND empty($options['recursive']))

if ($data['guid'] AND !$data['expandEvents']) {
if (isset($data['guid']) AND (!isset($data['expandEvents']) OR !$data['expandEvents’])) {

Larry Garfield said it took him 3 weeks to make the changes. It had more lines of code, but it does not mean it will be faster for me as I used this everywhere and we can’t know if there is less or more occurrences to change in my code base.
Anyway, I really don’t see the benefits to spend weeks doing this. It may highlight bugs, but it’s not sure and for me it does not worth it at all...

If I started a new project, I agree it is a good practice, but for a legacy project, this warning is just a lot of pain… I’m sure there are other projects which uses this coding style and did not wake up yet. This behavior might be the thing which will stop them from moving to PHP 8. At least for us, it is.

I took a look at the PHP code source and it seems there are just two lines of code which causes this behavior…

For me and for other PHP users with legacy code in the same distress, I’m ready to go into the process of an RFC to add a setting somewhere.

Let me know if this is something you would support. From your last answer, I guess not :-(

Mark Randall

4 years ago

On 21/02/2022 10:29, Nicolas BADIA wrote:

> and I don’t find the code more readable. Here are some examples of changes:

The example changes you provide are what many people here would
recommend as good practice to improve the context and safety your code
provides.

Anyone looking at that code can immediately infer that there is an
expectation that those keys may be missing, with a suitable default
provided for if that is the case.

The engine can see this too, which avoids it emitting warnings.

It takes a little more code, yes. But that is because you're being more
clear in your intent.

This is a good thing. You should keep at it.

You could probably have the majority of your codebase converted before
you could even hold a vote. Even if the vote was passed, which seems
most unlikely, you would still need to wait 10 months for 8.2 to come
out to use it.

Mark Randall

4 years ago

On 15/02/2022 12:31, Nicolas BADIA wrote:

> As I’m not ready to spend weeks upgrading our codebase because our coding style is not supported by the community, I’m looking at alternatives.

I don't think you'll be in luck, and should plan to get ready.

As to the specific behaviour:

If anything this behaviour will get tighter over time, and it's entirely
possible that in a later major release it will throw an error (although
that's not being proposed right now).

From my conversations, there is little apetite to add additional INI
based controls for engine behaviour, as they are believed to lead to
inconsistencies that harm the ecosystem, rather than enhance it.

Forking PHP to avoid resolving ambiguities in your code seems like it
would be a false economy.

Ideally you should bring your code up-to-date with what is expected, but
if you are unwilling or unable to do that, you may instead wish to stay
on an older version that hax more lax behaviour, and purchase LTS from
the likes of Zend.

Mark Randall