Re: Setting to disable the "Undefined array index" warning

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 :-( > Le 15 févr. 2022 à 17:41, Larry Garfield <larry@garfieldtech.com > <mailto:larry@garfieldtech.com>> a écrit : > > 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 >>> <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 > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: https://www.php.net/unsub.php > <https://www.php.net/unsub.php>