Re: [PHP-DEV] NULL Coercion Consistency
On Thu, May 26, 2022 at 5:21 AM Craig Francis wrote: > > 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). > > Nearly *all* code has a problem with null. It very much feels like the original effort to deprecate null calls decided to resolve this by saying "let's have the language help developers improve their code so it doesn't have these problems in the first place", and this effort is trying to resolve this by saying "let's have the language support the buggy code in ways that makes it work". At my job, my task for the last three weeks has literally been upgrading our internal codebase for 8.1, and the biggest set of logs I'm dealing with is exactly what you're talking about here: null's passed to internal functions. Every single case I've looked at so far has been traced to code that was written incorrectly, where some code somewhere was not properly guarding its values, and error cases were slipping through. Jordan
Re: [PHP-DEV] NULL Coercion Consistency
On Thursday, May 26, 2022 at 11:41 AM, Craig Francis mailto:cr...@craigfrancis.co.uk)> wrote: > On 26 May 2022, at 15:01, Michael Babker wrote: > > On Thu, May 26, 2022 at 7:21 AM Craig Francis > > wrote: > > > 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? > > > > No, they should not. Laravel made an explicit choice to handle null values > > in its escaping function for its templating framework. That is their choice > > to make. They should not be forced to implicitly handle null values because > > the language decides to add implicit type coercion to user defined > > functions (because consistency seems to be so important to some), and they > > should not be forced to reject null values because underlying language > > functions reject them as well. > > > Erm, I'm simply asking - If there is a good reason for throwing an exception > when NULL is passed to `htmlspecialchars()`, then that reason would also > apply to Laravel Blade for it's HTML encoding. Laravel’s `e()` helper function is more than a wrapper around `htmlspecialchars()` which supports values beyond a string, and has elected to support a null value being passed into that function with explicit handling for it. I’ve seen similar patches land in other packages as well. Whether it is to prevent that package from triggering the deprecation regarding null values itself or the package owners choosing to explicitly handle the null case so users don’t need to deal with it doesn’t really matter, the point is folks have made that decision and it is not problematic in any way IMO. You’re basically saying that a framework choosing to apply the same `htmlspecialchars($string ?? ‘’)` patch that applications are suggested to use (for those who don’t care to differentiate between null and empty string before something reaches an escaping function) is in the wrong by arguing for Laravel to revert that patch. On Thursday, May 26, 2022 at 11:41 AM, Craig Francis mailto:cr...@craigfrancis.co.uk)> wrote: > > If a function, by explicit design, can safely work with null values then > > that parameter should be properly declared nullable. > > > `htmlspecialchars()` can and always has safely worked with NULL, the same > with the other 335 parameters I've listed. But when I suggested changing > these parameters to be nullable, it was not well received. The arginfo for the `htmlspecialchars()` function, and if I’m reading correctly (I’m no C developer so I could very well be misinterpreting things) the internal `php_html_entities()` function which `htmlspecialchars()` calls, does not declare null as a supported value for the `$string` parameter. Meaning that depending on whether your code uses strict_types or not, either calling the function with a null value triggers a type error or requires implicit type coercision to convert the value to a type that the function does support. This means to me that the `htmlspecialchars()` function does NOT support null values and that passing null through only works because of the reliance on implicit type coercision. That, to me, is a valid reason for a type error to be raised; the code very explicitly requires a string and is not written to process a null value. A blanket suggestion to make every string parameter which implicitly supports a null value nullable because they previously supported null through type coercion IMO should be shot down. Instead, parameters which are emitting a deprecation notice because they are receiving an unsupported null value should be reviewed, and if that function can by design support null values, those parameters should be updated to reflect nullability. IMO, `htmlspecialchars()` is not a function which should expliciitly support a null value and the present deprecation is correct. On Thursday, May 26, 2022 at 11:41 AM, Craig Francis mailto:cr...@craigfrancis.co.uk)> wrote: > On 26 May 2022, at 15:01, Michael Babker wrote: > > Yes, that means that there is a lot of work involved for folks between now > > and the release of PHP 9.0 to either refactor code to avoid type errors or > > to get the language to expand the supported types in appropriate functions, > > but IMO that effort from all parties is worth it in the long run to ensure > > language consistency (I really don’t think userland PHP and > > internal/extension PHP should have vastly different behaviors, and type > > coercion support based on where a function is designed is one of those > > holes that needs filled) and to ensure all APIs explicitly declare what > > types of data they support. > > > > I don't think userland and internal functions should have different > behaviours either (my RFC does say "keep user-defined and internal functions > consistent"). > > To a
Re: [PHP-DEV] Re: ***SPAM*** [PHP-DEV] [Discussion] Expand deprecation notice scope for partially supported callables
Hey Julie,
On Thu, 26 May 2022 at 12:45, Juliette Reinders Folmer
wrote:
>
> I propose to open the vote on this RFC tomorrow.
Before you open the vote, please could you split the voting into two,
one for the is_callable, and one for the callable type check?
Rowan wrote in https://news-web.php.net/php.internals/117670:
> is that passing "99 red balloons" to an "int"
> parameter raised an E_NOTICE in PHP 7.x, so a "callable" parameter
> raising an E_DEPRECATED should be fine.
There's one issue.
When "99 red balloons" is coerced to an int, that is done once, and
then any further int type check will pass. For callables, it's pretty
common to pass them down a stack of code, e.g. similar to:
function foo(callable $fn, $data)
{
$fn($data);
}
function bar(callable $fn, $data)
{
return foo($fn);
}
function quux(callable $fn, $data)
{
return bar($fn, $data);
}
function main(array $data)
{
$fn = get_callable_from_input();
if (is_callable($fn) === false) {
// give some error.
return;
}
quux($data);
}
As far as I understand it, this code would give a deprecation notice
for each call level, which seems quite undesirable. Particularly if
the callable is being used in a loop.
Also, without a patch it's hard to guess what performance impact this
would have. I doubt it would be huge, but it probably wouldn't be zero
either. Performance wouldn't matter for is_callable, as that is
typically only done once per callable, but callable type checks are
done continuously.
cheers
Dan
Ack
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] NULL Coercion Consistency
On 26 May 2022, at 15:01, Michael Babker wrote:
> On Thu, May 26, 2022 at 7:21 AM Craig Francis
> wrote:
>> 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?
>
> No, they should not. Laravel made an explicit choice to handle null values
> in its escaping function for its templating framework. That is their choice
> to make. They should not be forced to implicitly handle null values because
> the language decides to add implicit type coercion to user defined functions
> (because consistency seems to be so important to some), and they should not
> be forced to reject null values because underlying language functions reject
> them as well.
Erm, I'm simply asking - If there is a good reason for throwing an exception
when NULL is passed to `htmlspecialchars()`, then that reason would also apply
to Laravel Blade for it's HTML encoding.
If you know what that reasons is, you should be able to make that case to them,
and get that patch reverted.
I believe the only time it's helpful is when working in a strict type
environment, which is best checked with static analysis, as those extra type
checks can highlight unexpected values. But most (I'm assuming ~85%) developers
simply do not use strict type checking, and don't seem to consider the sources
of NULL (see the "Common sources of NULL" I've listed in the RFC), and for
those developers, spending days editing their code to manually convert those
NULL values, that does not seem justified.
To respond to your comment, Laravel doesn't provide a native type declaration
(it's in a DocBlock), so they don't have any type coercion for their `e()`
function:
https://github.com/laravel/framework/blob/c113768dbd47de7466d703108eaf8155916d5666/src/Illuminate/Support/helpers.php#L109
> Every implicit type coercion is a bug in hiding IMO.
That's fine, but strict static analysis is so much better at finding these
issues (i.e. can this variable be NULL, vs a runtime check to see if the value
is NULL this time)... and if you want runtime checks as well, that's where
`strict_types=1` comes in.
> If a function, by explicit design, can safely work with null values then that
> parameter should be properly declared nullable.
`htmlspecialchars()` can and always has safely worked with NULL, the same with
the other 335 parameters I've listed. But when I suggested changing these
parameters to be nullable, it was not well received.
> Yes, that means that there is a lot of work involved for folks between now
> and the release of PHP 9.0 to either refactor code to avoid type errors or to
> get the language to expand the supported types in appropriate functions, but
> IMO that effort from all parties is worth it in the long run to ensure
> language consistency (I really don’t think userland PHP and
> internal/extension PHP should have vastly different behaviors, and type
> coercion support based on where a function is designed is one of those holes
> that needs filled) and to ensure all APIs explicitly declare what types of
> data they support.
I don't think userland and internal functions should have different behaviours
either (my RFC does say "keep user-defined and internal functions consistent").
To achieve that, for those not using `strict_types=1`, I'm simply suggesting
that NULL coercion continues to work for internal functions, and we update
user-defined functions to allow NULL coercion. This would also help developers
add native types to their functions. Then we have a setup which is basically
the same as how the string "6" can be coerced to the integer 6, and how string
concatenation with NULL works, how `'' == $nullable` works`, how `sprintf('Hi
%s', $nullable)` works, how `echo $nullable` works, etc.
Craig
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
[PHP-DEV] PHP 8.0.20RC1 Ready for testing
PHP 8.0.20RC1 has just been released and can be downloaded from: https://downloads.php.net/~pollita/ Or use the git tag: php-8.0.20RC1 Windows binaries are available at https://windows.php.net/qa#php-8.0 Please test it carefully, and report any bugs in the bug system: https://bugs.php.net. Hash values and PGP signatures can be found below or at: https://gist.github.com/sgolemon/efa581558c226f7de4f45d85352e7588 8.0.20 should be expected in 2 weeks, i.e. on 9th June 2022. Thank you, and happy testing! Regards, Gabriel Caruso & Sara Golemon php-8.0.20RC1.tar.gz SHA256 hash: c2e1dc4f0162700c2dc75616c47df28087bfca1d10d66ef97a7b75b15330383e PGP signature: -BEGIN PGP SIGNATURE- iQJEBAABCgAuFiEEFyn4OTjaROJ7oPTT29s5dHDRIXIFAmKNKF4QHHBvbGxpdGFA cGhwLm5ldAAKCRDb2zl0cNEhcpyHD/4vaMLLMH3ab1aJ+5RLsNtDpgA0IjmKLIAB zlpEuteqypqLKq68cQGO6mQZoOYelEIDx80GHCFT7mSBtSCU30ZuViatWZRkTc6i vt80pyHILy694BepRn77AQOu/BpcN+8PuPnrxLxhmTWdI9qMgyA5OWjoaEL5cS9W cTHddGrxukuPaKqrgV0+o3XtIMGJw5joTBW8mnq75ncQDSsk17JnlBdPtfyxloFG /RCbDlmuUzvzDuL65adpWInWWwN/jbuddrcBnvhg1za7+cR5AvsRPjYE5bCQMgQV AZ5J3e7S1Z2KDW4Yy9sTazOuYKC0PJDRQP4Q2DeS4Pcxk6bHpFY8nFvQd/fGlAh4 OKBR1sunjX5tTBAP1gaMg7ES24HO9VgnuDILB6lrPCCO5LVIEDgErwdQswPV+sqM 016AXyq6pFu5LXEMxt6nmIsveP0j3zXWPY2OvAEK2bUtHN194WyRqUCMzwWp9hSZ 4AbCale/DGO4jO9oCUVVVXBVIcj9/V03ajRTGv4FSP12Ia2dOgtIE+Rjl5U9iBti c+DA0UA5MdCv8kPlfJUIiAp1Oe2u9G1ILhelFA3KVZh97FYxigmZXozQnAZWI8Pr Igp6SX0CleCSu3GCRxOjgIdPTYIdQQNvNUjQzVJFT9edhA1km69NI2NBsKcHPaZ8 qypgnODfIg== =zglc -END PGP SIGNATURE- php-8.0.20RC1.tar.bz2 SHA256 hash: 4380b18ae67e064cd015cf7e876cf3cb844eb0e9ae68eb715ddc178b54283c82 PGP signature: -BEGIN PGP SIGNATURE- iQJEBAABCgAuFiEEFyn4OTjaROJ7oPTT29s5dHDRIXIFAmKNKGEQHHBvbGxpdGFA cGhwLm5ldAAKCRDb2zl0cNEhckn3EACtYxMT2y3A/5jXfmjQg3gsBbjWy4nsMY1R vR3fb4ZmDyROMpcdJdlU5TLqV1n1YJzcNapoSA9UxHGXrp2eofVHFm4lkzh3mLuP UlvH0itIjEJE7EzEa5qrgqhhqv2nt9Py2LSL47gEwpvX7sxW92DWEpaH7kqpH25U Wpii2CcWm9BZmNdMhCrnsu+53mwzQfow6eomzJNkeNtdA3B6yOy5ZQE1UNW/1acf 7FNOygmrgWIxu+P/F4UdwDtMvqkoJTD4bbQUc7+9nsare/Vm/oohWo0DMYNpZnyq ghDRSbAhlvNqiZu9fVK93EoKZAEAPVYoCY//oRceoXNh9vm7Crq7Gywc9ZtUxUzU Pm4HHKJbAmv5aI6aPwXvqJgpz6WqOWYDg4mo1nHjs0mfE0qtXWJ/g+Xu0Q5hsvZY LnntqBTZ2V0V8Ons2aAHiqbDKY+MzyVVdIj0lYVdB9a3e1id45lSXkcPku3H3XzC TXcfHQSAU5Yliuy9FC89FrGEgNn1k54aIstHdcQIpL9z8l5GSu8dCIXYTTjijx5l fMyiVNF0+ETU3vjnoITcP/C2q3aKlk85Dx68EOBUkmsyR/Bfjsk/5YwQw/xRQVZN YgekTLMJaHhX/Oux4Uj773B8k6oHx07NGC1aZ9BrXiQhJ1R1qrpsVREDXpZ5glav A2aJK5/uhg== =aOV1 -END PGP SIGNATURE- php-8.0.20RC1.tar.xz SHA256 hash: f6cdb3f90838d0a0215f92e3ce41c00934f36e9c6a3ab05491168ba5c5b6c8b4 PGP signature: -BEGIN PGP SIGNATURE- iQJEBAABCgAuFiEEFyn4OTjaROJ7oPTT29s5dHDRIXIFAmKNKGIQHHBvbGxpdGFA cGhwLm5ldAAKCRDb2zl0cNEhcvH3D/wLdAuYArF+rTxHkQ43Ts1EU1TFJz1x9Lus d2s5+E0ZSFNZiwlltd2KqI8tYx2f4qrxdesbmF9V4AVL4nM2yDgi6psNfhmnMjac JLjACsHqOmXgbaW4a4SuoXo1oE8VkWFrI7sFdbI3ZqyduwrOzukJIzAKQP4RW5s1 o0WrIGLkPBN4faJ+ATVfmL/r8DBDT8Y0Fv9/keKdSqE009IBucFBIvIv13wY4RoL EBjUdj8O2XIT7dgW7XIcLY4Om9f6YqK9Qa40LKk9YLJXu/qm9zPQZ6fxR51m9ouE r9cwPL8s/QAU58Csfc5wlR6Rps1mGOyMMfyAncNjN6FoG2ZXXj5tM88B56YUHX0t rrmI9Q0jSkv+dK77ZJNQaD96p8fqevE4sWiTdJCIO3NzQGquklDmaY2NvkXeqYBH Eu4YT1nPdfTIz5FTuPM7CHQFtid2v8lTFExFCtRwd4adph4AyvQQjjDbVIWjS+9f HsXFMVGIQrCbZKs64mDyaQcKQOwoN1apyhGVcmkhZWNEG9Zj6s3cRPATFojKh/Uv r1gkD38qxBs1puhtQbKUSauF8S3GaU7APnlrFJXUF6HntW5zOA2XZUz6jRmlOiMh FeAYx/LoSTQx7Bg4qahsBykTwQs0F41Ny3l5uEK4CctCPzvIN3Np3VDLvylQYgsY QhSisPloXQ== =77/O -END PGP SIGNATURE-
Re: [PHP-DEV] NULL Coercion Consistency
On Thu, May 26, 2022 at 7:21 AM Craig Francis wrote: > 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? > No, they should not. Laravel made an explicit choice to handle null values in its escaping function for its templating framework. That is their choice to make. They should not be forced to implicitly handle null values because the language decides to add implicit type coercion to user defined functions (because consistency seems to be so important to some), and they should not be forced to reject null values because underlying language functions reject them as well. Every implicit type coercion is a bug in hiding IMO. If something doesn’t explicitly support null, you shouldn’t have been passing null to it in the first place. Relying on magic language behaviors to prevent type errors is not a long term sustainable code pattern. If a function, by explicit design, can safely work with null values then that parameter should be properly declared nullable. If a function, by explicit design, does not support null values then a type error is a hugely acceptable response. Yes, that means that there is a lot of work involved for folks between now and the release of PHP 9.0 to either refactor code to avoid type errors or to get the language to expand the supported types in appropriate functions, but IMO that effort from all parties is worth it in the long run to ensure language consistency (I really don’t think userland PHP and internal/extension PHP should have vastly different behaviors, and type coercion support based on where a function is designed is one of those holes that needs filled) and to ensure all APIs explicitly declare what types of data they support. -- - Michael Please pardon any errors, this message was sent from my iPhone.
Re: [PHP-DEV] NULL Coercion Consistency
On 24 May 2022, at 09:47, Rowan Tommins 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 `Hi {{ $name }}`, 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
[PHP-DEV] Re: ***SPAM*** [PHP-DEV] [Discussion] Expand deprecation notice scope for partially supported callables
On 12-5-2022 18:54, Juliette Reinders Folmer wrote: After the prior discussion about the same topic: https://externals.io/message/117342, I have created an RFC to expand the scope of the deprecation notices being thrown for the deprecated partially supported callables to include is_callable() and the callable type in PHP 8.2. With this email I'm opening the two week discussion period for this RFC. All points raised in the prior discussion are already included in the RFC. https://wiki.php.net/rfc/partially-supported-callables-expand-deprecation-notices I look forward to your feedback. Seeing how there isn't any new discussions on this RFC and presuming that means that the previous discussion touched all concerns, I propose to open the vote on this RFC tomorrow. RFC: https://wiki.php.net/rfc/partially-supported-callables-expand-deprecation-notices If anyone still wants to know a little more about the background, you may want to have a listen to Derick's podcast about this RFC: https://phpinternals.news/101 Smile, Juliette
