Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:117090
Content-Type: multipart/alternative;
	boundary="Apple-Mail=_B46AFEDD-994A-4F22-8ECC-C4FAE8B4AD99"
Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.120.0.1.13\))
Date: Mon, 21 Feb 2022 11:29:06 +0100
References: <A4A387E9-1D77-466D-83AA-89B78BA40814@gestixi.com>
 <67207814-3052-b116-e856-33e8440049d3@gmx.net>
 <153fc3b5-5409-44ad-b618-84297aa49a3d@www.fastmail.com>
To: internals@lists.php.net
In-Reply-To: <153fc3b5-5409-44ad-b618-84297aa49a3d@www.fastmail.com>
Message-ID: <DE3621F9-1B45-4762-ACC9-27628F53C897@gestixi.com>
Subject: Re: [PHP-DEV] Setting to disable the "Undefined array index" warning
From: nicolas@gestixi.com (Nicolas BADIA)

--Apple-Mail=_B46AFEDD-994A-4F22-8ECC-C4FAE8B4AD99
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8

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=E2=80=A6 I don=E2=80=99t want that.=20
After 2 hours, no bug found (but I generated one by translating the =
code) and I don=E2=80=99t find the code more readable. Here are some =
examples of changes:

if ($options['isCacheable'] =3D=3D=3D false) =20
if (($options['isCacheable'] ?? true) =3D=3D=3D false)=20

$twigVar =3D $pageClass->twigVars->$property;=20
$twigVar =3D $pageClass->twigVars->$property ?? null;

$data =3D $domains[$domain];
$data =3D $domains[$domain] ?? null;

$group =3D $data['group'];
$data =3D $data['group'] ?? null;

($options['masterAccount'] =3D=3D=3D 'only=E2=80=99)=20
(($options['masterAccount'] ?? null) =3D=3D=3D 'only')

if ($data['mysqlCache'] =3D=3D=3D false)=20
if (($data['mysqlCache'] ?? true) =3D=3D=3D false)

if ($data['allowCache'] !=3D=3D false)=20
if (($data['allowCache'] ?? null) !=3D=3D false)

if ($data['addRelations'] =3D=3D=3D true)=20
if (($data['addRelations'] ?? false) =3D=3D=3D true)

if ($options['ignoreMenu'] AND !$options['recursive']) {=20
if (!empty($options['ignoreMenu']) AND empty($options['recursive']))

if ($data['guid'] AND !$data['expandEvents']) {=20
if (isset($data['guid']) AND (!isset($data['expandEvents']) OR =
!$data['expandEvents=E2=80=99])) {

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=E2=80=99t know if there is less or more =
occurrences to change in my code base.=20
Anyway, I really don=E2=80=99t see the benefits to spend weeks doing =
this. It may highlight bugs, but it=E2=80=99s 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=E2=80=A6 I=E2=80=99m =
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=E2=80=A6

For me and for other PHP users with legacy code in the same distress, =
I=E2=80=99m ready to go into the process of an RFC to add a setting =
somewhere.=20

Let me know if this is something you would support. =46rom your last =
answer, I guess not :-(




> Le 15 f=C3=A9vr. 2022 =C3=A0 17:41, Larry Garfield =
<larry@garfieldtech.com <mailto:larry@garfieldtech.com>> a =C3=A9crit :
>=20
> 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=3D81417 =
<https://bugs.php.net/bug.php?id=3D81417> we use to check if a property =
exists by accessing it directly like we do in JavaScript.
>>>=20
>>> 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=E2=80=A6 We can=E2=80=99t even use the last version =
of PHP Debug for VSCode.
>>=20
>> 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.
>>=20
>> 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.
>>=20
>> 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.
>=20
> 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.
>=20
> 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.
>=20
> 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.
>=20
> 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.
>=20
> 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.
>=20
> --Larry Garfield
>=20
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php =
<https://www.php.net/unsub.php>

--Apple-Mail=_B46AFEDD-994A-4F22-8ECC-C4FAE8B4AD99--