Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:117044
User-Agent: Cyrus-JMAP/3.5.0-alpha0-4773-gd41406ac52-fm-20220208.001-gd41406ac
Mime-Version: 1.0
Message-ID: <153fc3b5-5409-44ad-b618-84297aa49a3d@www.fastmail.com>
In-Reply-To: <67207814-3052-b116-e856-33e8440049d3@gmx.net>
References: <A4A387E9-1D77-466D-83AA-89B78BA40814@gestixi.com>
 <67207814-3052-b116-e856-33e8440049d3@gmx.net>
Date: Tue, 15 Feb 2022 10:41:44 -0600
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] Setting to disable the "Undefined array index" warning
From: larry@garfieldtech.com ("Larry Garfield")

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=3D8=
1417 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 o=
ur codebase (more than 130 000 lines of PHP code). When it became a noti=
ce, we disabled them, but now that it is a warning, it is a lot more pro=
blematic for us=E2=80=A6 We can=E2=80=99t even use the last version of P=
HP 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 ze=
ro
> 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 a=
re
> probably not expected. If you just want a quick fix without solving the
> underlying problem, you can just add "?? null" to each array key acces=
s.
>
> 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 unexpect=
ed
> 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-commen=
t 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" behavio=
r, which broke badly in PHP 8.

It took one person (me) about 3 weeks I think of running unit tests, add=
ing ?? 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, b=
ut 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 b=
een 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) ch=
anges to the architecture or business logic of the application, this wou=
ld improve the quality of the application.

I'd wager it's less work in terms of raw time to just fix up the code ba=
se than it is to write, implement, debate, pass, and merge an RFC to mak=
e you not have to do so.

--Larry Garfield