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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
