On 24 May 2022, at 09:47, Rowan Tommins <rowan.coll...@gmail.com> wrote:
> 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
>>
>> 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.
To confirm the order of events:
First, the Docblock originally said this function did not accept NULL, but at
runtime it accepted/coerced NULL to an empty string. This is exactly how
`htmlspecialchars()` worked pre 8.1. Where developers using static analysis
tools can choose to treat NULL as an invalid value, and those tools could
report nullable variables as an error (via strict type checking).
Second, the Docblock and implementation was updated to allow NULL, because NULL
is common value (a backwards compatibility issue), so this HTML encoding
function, used by Blade templates by default, now accepts NULL.
This seems to undermine the argument that NULL should not be passed to
`htmlspecialchars()`.
I did suggest updating the ~335 function parameters to be nullable; but that
was rejected because some developers prefer to treat NULL as an invalid value.
To keep those developers happy, and avoid the backwards compatibility issue, it
seems easier to allow NULL to be coerced (like other contexts, e.g. string
concat).
> 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.
While I'd put the word "error" down as a typo, the intention is for 9.0 to
throw a type error.
And while user-defined functions are part of the conversation (for consistency
reasons), I'm trying to find the benefits of breaking NULL coercion for
internal functions (because, if there is an overall benefit, that Laravel Blade
patch should be reverted).
> 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");
> }
> // ...
> }
It sounds like you got lucky - you have a function that has a problem with NULL
(but I assume it's fine with an empty string?), and during your testing you
happened to pass NULL to this function. As noted before, static analysis is
*considerably* better at these types of checks, because it's able check if
variables *can* contain NULL. They can also perform other checks as well
(important when your code seems to care about NULL vs an empty string).
> 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.
If you have a better solution, please let me know.
I don't think breaking NULL coercion for internal functions helps (the change
seems to involve loads of tiny updates, probably by littering projects with
`$val ?? ''`, `strval($val)`, or `(string) $val`, with hard to define
benefits); in contrast, allowing NULL coercion for user-defined functions would
at least keep user-defined and internal functions consistent, with NULL
coercion continuing to work in the same way as other contexts (e.g. string
concat, == comparisons, sprintf, etc); or how coercion works between
string/int/float/bool.
That said, I would still like to know about the benefits of rejecting NULL for
`htmlspecialchars()`, in the context of that Laravel Blade patch; because, if
it is beneficial (vs the BC break), they should revert that patch... shouldn't
they?
Craig
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
