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