Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:117779
Message-ID: <1180af01-080f-ee0a-3159-74bf7e0a8aea@gmail.com>
Date: Tue, 24 May 2022 09:47:30 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Content-Language: en-GB
To: internals@lists.php.net
References: <CAFv4g+Ej4JbM9SzQ0iU-d8C4T+2XNo=8jnJ1uwfQ=skZZXx9Tg@mail.gmail.com>
 <1755E8B5-229B-47B2-BBAF-B5E014F5473D@craigfrancis.co.uk>
In-Reply-To: <1755E8B5-229B-47B2-BBAF-B5E014F5473D@craigfrancis.co.uk>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] Re: NULL Coercion Consistency
From: rowan.collins@gmail.com (Rowan Tommins)

On 23/05/2022 19:45, Craig Francis wrote:

> For those against my RFC, can you take a quick look at this patch for Laravel:
>
> https://github.com/laravel/framework/pull/36262/files#diff-15b0a3e2eb2d683222d19dfacc04c616a3db4e3d3b3517e96e196ccbf838f59eR118 <https://github.com/laravel/framework/pull/36262/files#diff-15b0a3e2eb2d683222d19dfacc04c616a3db4e3d3b3517e96e196ccbf838f59eR118>
>
> If passing NULL to `htmlspecialchars()` represents a problem, as used in templates like `<p>Hi {{ $name }}</p>`, presumably this patch should be reverted?


I notice that the docblock didn't previously list null as valid input, 
so this was only working by mistake, as the commit message admits. If 
you copied the documented union to the native type declaration on the 
parameter, it would immediately reject nulls, because that has always 
been the behaviour of user-defined functions. Static analysis tools will 
also have complained that null was not a valid input according to the 
documentation.

I also note that the commit message says "On PHP >= 8.1, an error is 
thrown if `null` is passed to `htmlspecialchars`." which is of course 
not true for native PHP, only if you make the highly dubious decision to 
promote deprecations to errors.


As an anecdote, I was recently working on a bug involving nulls causing 
unintended behaviour in an API. As part of the clean-up, I changed a 
function signature from getByIdentifer($identifier) to 
getByIdentifer(string $identifier), and during testing, got an error 
because I'd missed a scenario where null was passed. This was a good 
result - it prevented the broken behaviour and alerted me to the case 
that needed fixing. If the parameter had instead been silently coerced 
to an empty string, I would have got neither an error nor the original 
null behaviour, causing a new bug that might have taken much longer to 
track down.

If your RFC as drafted was implemented, I could only have achieved the 
desired check by turning on strict_types=1 for whole sections of the 
application, which would probably require dozens of other fixes, or 
writing an ugly manual check:

function getByIdentifier(?string $identifier) {
     if ( $identifier === null ) {
          throw new TypeError("Function doesn't actually accept nulls");
     }
     // ...
}

As I have said previously, I share your concern about the impact of the 
currently planned change, but I don't think loosening the existing type 
rules on user-defined functions is a good solution.


Regards,

-- 
Rowan Tommins
[IMSoP]