On Sun, Feb 27, 2022 at 12:35 PM Mark Randall <marand...@php.net> wrote:
> On 27/02/2022 09:12, Robert Landers wrote: > > I'd also venture that this warning has caused more harm than good, in > that > > writing "$var['something'] ?? null" is second nature when writing new > > code, even if there is practically no chance for a non-existent key. > > > > Using null coalesce should only be used when you know you have the > possibility of a missing key. > That can't always be known, or requires tracing the stack to see where it comes from and what modifies the array before it is passed to the function you are modifying, it is an array after all, not a formalized object with well-defined properties (which I'd prefer, but we can't always change what someone did 15 years ago just for the sake of changing it). > > For everything else you've probably entered an unexpected state and > should display a warning (that should ideally be picked up by an > exception handler and convert it to an exception). > Not always, and not in cases where people are just coalescing null into null to avoid a warning. > > When handling external data it is usually a good idea to use a > validation step before using it, such as JSON schema. > I don't usually write code dealing with handling external API requests in my day-to-day work, so I can't really speak to JSON handling, specifically. I have had to unserialize objects from JSON files from other internal systems but that was back when there was only the Elvis operator (PHP 5.6ish), so it was used quite liberally. > > An even better idea is to parse it into a specific type, rather than > just validating it, at which point you're going to be using a lot of > isset, null coalesces, array_key_exists, property_exists etc anyway and > sucking in some default other than null is almost always the wrong thing > to do. > I'm only suggesting this apply to ternary tests, not in general, so that code can be easier to read and grok. I believe null is a sensible default in cases when you are only establishing the truthiness of a value, and whether or not the key exists is irrelevant (otherwise there's the null coalesce operator and isset()). There may be times when you need more validation, but those aren't usually handled in ternaries. In other words, I believe this should still cause a warning if it is unset: $arr['unset'] ?: $arr['unset']; because it is likely a bug and used outside of the ternary test (the part before the "?" -- I'm not sure what the technical name is for it). This should not, however: $arr['unset'] ?: 'some default'; because it is only used in the test. I also like the suggestion of creating some null-safe operator for arrays, which might be a more general solution. > > As to your final point: PHP internals voted by a supermajority to raise > this from a notice to a warning in PHP 8, it seems unlikely that 33% of > people are now going to change their mind and vote for the opposite. > > We've had to live with it for a little while now and we have a pretty good idea of what it is doing to codebases and the time it takes to "fix" codebases by null coalescing null to null. I tried searching GitHub for this, but it looks like the search functionality removes the question marks in my query.
