[PHP-DEV] Re: NULL Coercion Consistency

[Craig Francis]

On 28 May 2022, at 03:36, Craig Francis  wrote:
> On 8 Apr 2022, at 18:34, Craig Francis  wrote:
>> I've written a new draft RFC to address the NULL coercion problems:
>> https://wiki.php.net/rfc/null_coercion_consistency
> 
> 
> I give up.



For everyone affected by this... Rector has a "solution" via 
NullToStrictStringFuncCallArgRector.

It has a list of 275 functions (442 parameters), and every time these functions 
are used, and Rector isn't sure a variable cannot be NULL, it will add a string 
type cast, e.g.

- 
+ 

So yeah, it's very messy, and while I think it makes the code worse, at least 
it's one way to avoid the fatal errors (coming in 9.0).

Oh, and developers can easily re-introduce more "problems" (because they might 
not be aware which variables can be NULL), so Rector will need to re-run.

I'm also tempted to create a composer library to provide a second solution, by 
using a namespace to re-define these functions, e.g.



Have fun,
Craig



https://github.com/rectorphp/rector-src/blob/main/rules/Php81/Rector/FuncCall/NullToStrictStringFuncCallArgRector.php#L37

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] Re: NULL Coercion Consistency

[Guilliam Xavier]

On Mon, May 30, 2022 at 4:59 PM Rowan Tommins  wrote:
>
> The actual code in this case ended up in a generic routine that used
> isset() to choose which SQL to generate. An empty string would generate
> a WHERE clause that matched zero rows, but a null would omit the WHERE
> clause entirely, and match *all* rows. So an extra pre-validation on the
> string format might be useful for debugging, but wouldn't result in
> materially different results.

Maybe the routine could use e.g. array_key_exists() rather than
isset()? (anyway, sometimes you *actually* want isset() null behavior,
or use a null default for a parameter as a "not passed" argument...)

> That's actually an interesting observation. It's probably quite common
> to treat empty strings as null when going from input to storage; and to
> treat null as empty string when retrieving again. Importantly, databases
> generally *don't* treat them as equivalent,

Yeah, I only know Oracle to do something as... "clever" as storing an
empty VARCHAR '' as NULL (for "optimization" IIRC) -_-

> so forgetting that
> translation can be a real cause of bugs. I often advocate for string
> columns in databases to allow either null or empty string, but not both
> (by adding a check constraint), so that such bugs are caught earlier.

Same (sometimes you have no choice but allow NULL, e.g. an optional
foreign key, but the referenced primary key is not nullable and should
generally also reject '')

-- 
Guilliam Xavier

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] Re: NULL Coercion Consistency

[Rowan Tommins]

On 30/05/2022 15:04, Guilliam Xavier wrote:

For this specific example, shouldn't it rather [already] be like this anyway?

function getByIdentifier(string $identifier) {
  if ( $identifier === '' ) {
   throw new InvalidArgumentException("Empty identifier");
  }
  // ...
}

(but I guess you could find actual examples where an empty string is
"valid" and null is not, indeed)



The actual code in this case ended up in a generic routine that used 
isset() to choose which SQL to generate. An empty string would generate 
a WHERE clause that matched zero rows, but a null would omit the WHERE 
clause entirely, and match *all* rows. So an extra pre-validation on the 
string format might be useful for debugging, but wouldn't result in 
materially different results.




I'm just worried to see people rather disabling deprecation notices
(via error_reporting, or a custom error handler, or even patching
PHP's source and recompiling their own) than "fixing" the code
(granted, that's not specific to*this*  RFC, but somewhat "highlighted" here)



Indeed, I think we have a general problem with how deprecations are 
communicated and acted on in general. I have been thinking about how to 
improve that, other than "never change anything" or "never warn people 
we're going to change anything", and will try to write up my ideas soon.




function null_to_empty_string(?string $string_or_null): string
{ return $string_or_null === null ? '' : $string_or_null; }

(but also its "opposite" empty_string_to_null(string $string): ?string)


That's actually an interesting observation. It's probably quite common 
to treat empty strings as null when going from input to storage; and to 
treat null as empty string when retrieving again. Importantly, databases 
generally *don't* treat them as equivalent, so forgetting that 
translation can be a real cause of bugs. I often advocate for string 
columns in databases to allow either null or empty string, but not both 
(by adding a check constraint), so that such bugs are caught earlier.


To go back to Craig's favourite example, that could be a genuine problem 
caused by passing null to htmlspecialchars() - if we intended that null 
to be stored as such in the database, we've silently converted it into a 
non-equivalent empty string. (Yes, escaping should be done on output not 
input, but it's not completely infeasible that that combination might 
happen.)


Regards,

--
Rowan Tommins
[IMSoP]

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] Re: NULL Coercion Consistency

[Guilliam Xavier]

On Tue, May 24, 2022 at 10:47 AM Rowan Tommins  wrote:
>
> 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");
>  }
>  // ...
> }

For this specific example, shouldn't it rather [already] be like this anyway?

function getByIdentifier(string $identifier) {
 if ( $identifier === '' ) {
  throw new InvalidArgumentException("Empty identifier");
 }
 // ...
}

(but I guess you could find actual examples where an empty string is
"valid" and null is not, indeed)

> 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.

I agree the "issue" looks like different PoV of benefit VS cost, plus
the reticence about going back on the general trend of "more
strictness" (even with strict_types=0).
I'm just worried to see people rather disabling deprecation notices
(via error_reporting, or a custom error handler, or even patching
PHP's source and recompiling their own) than "fixing" the code
(granted, that's not specific to *this* RFC, but somewhat "highlighted" here)


FWIW, to avoid the "risks" of `expr ?? ''` (possibly hiding undefined)
and `(string)expr` / `strval(expr)` (potentially casting "too much"
without errors), I've already seen custom functions like

function null_to_empty_string(?string $string_or_null): string
{ return $string_or_null === null ? '' : $string_or_null; }

(but also its "opposite" empty_string_to_null(string $string): ?string)

Regards,

--
Guilliam Xavier

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

[PHP-DEV] Re: NULL Coercion Consistency

[Mark Randall]

On 28/05/2022 03:36, Craig Francis wrote:
 In this case, two of my clients are considering the cost of modifying their code (by adding a load of `?? ''` everywhere), and they would rather avoid that (time consuming, and makes their code more complicated). 



Alternatively, they may wish to define their own functions that do take 
null and then run a find / replace or rector rule to convert them over.


I would still recommend ?? because it's the most future proof.

We have passed two RFCs in recent months which will eliminate 
default-nulls in 2 of the 3 major variable sources in the engine (the 
remaining one being arrays), and that too may eventually find itself 
promoted.


If it's a framework, I would suggest adding methods for explicitly 
getting a string that defaults to '' instead of null.


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] Re: NULL Coercion Consistency

[Aleksander Machniak]

On 28.05.2022 04:36, Craig Francis wrote:

On 8 Apr 2022, at 18:34, Craig Francis  wrote:

I've written a new draft RFC to address the NULL coercion problems:
https://wiki.php.net/rfc/null_coercion_consistency


I give up.


Don't give up. You have my Yes vote.

Imo, the RFC:
- fixes real upgrade problem (very important),
- improves consistency in a better way than the solution introduced in 8.1.
- does not change strict_types behavior

Some people don't care with what arguments their functions are called 
with. As long as the value can be coerced to the expected type the 
function will do what it is supposed to do. Other people have 
strict_types. All people don't want to be forced to modify a working code.


So, this is another "battle" between strict and non-strict camps. I'd 
like to see which is the majority these days. I hope that even some 
strict-code proponents can see this makes sense.


--
Aleksander Machniak
Kolab Groupware Developer[https://kolab.org]
Roundcube Webmail Developer  [https://roundcube.net]

PGP: 19359DC1 # Blog: https://kolabian.wordpress.com

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

[PHP-DEV] Re: NULL Coercion Consistency

[Craig Francis]

On 8 Apr 2022, at 18:34, Craig Francis  wrote:
> I've written a new draft RFC to address the NULL coercion problems:
> https://wiki.php.net/rfc/null_coercion_consistency



I give up.



I'm clearly not clever enough to understand what the benefits are for breaking 
NULL coercion... considering NULL has been coerced for internal functions since 
forever, and continues to work in other contexts.

If anyone wants to work on a solution to the problem, feel free to either edit 
my RFC, or create your own RFC.

I believe my RFC documents every position put forward, and includes all of 
details I think are relevant.

I fear a vote now will only result in rejection; and once people put their 
names down for a certain position, they will never change their mind.

That said, if someone did continue, and got their RFC accepted, I should be 
able to organise some funding for the implementation.

For some background, I paid for the `is_literal()` implementation before that 
RFC vote, as that patch is useful irrespective of the result (it's being used 
in a few projects now, and works incredibly well; thanks again to Joe Watkins 
for writing, and Máté Kocsis for testing). In this case, two of my clients are 
considering the cost of modifying their code (by adding a load of `?? ''` 
everywhere), and they would rather avoid that (time consuming, and makes their 
code more complicated). I suggested putting aside a budget to either do that 
modification, or fund the implementation if my RFC was to pass. I'm not sure 
how big or complicated the task would be, but I noted the R11 suggested day 
rate of $500 (~£390), and the implementation could take a few weeks.

If anyone does want to email me about this, I won't respond until at least June 
6th.

Craig

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] Re: NULL Coercion Consistency

[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
 


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.


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

[PHP-DEV] Re: NULL Coercion Consistency

[Craig Francis]

On 8 Apr 2022, at 18:34, Craig Francis  wrote:
> I've written a new draft RFC to address the NULL coercion problems:
> https://wiki.php.net/rfc/null_coercion_consistency 
> 



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?

Craig

[PHP-DEV] Re: NULL Coercion Consistency

[Craig Francis]

On 6 May 2022, at 16:26, Björn Larsson  wrote:
> Den 2022-04-08 kl. 19:34, skrev Craig Francis:
>> Hi,
>> I've written a new draft RFC to address the NULL coercion problems:
>> https://wiki.php.net/rfc/null_coercion_consistency
>> ...
> 
> One code pattern to upgrade your code to PHP 8.1 and 9.0 is to use the
> null coalescing operator like rtrim($string ?? '',...).
> 
> If one then has a legacy codebase that works flawlessly I would say that
> this path is the preferred one since it requires a minimum of work. To
> dig into your code base and address why the parameter ends up like null
> is probably more cumbersome. OTOH, one doesn't reap the benefits of the
> "Deprecate passing null to non-nullable arguments of internal functions"
> RFC since it requires to much work.
> 
> One could argue that the "Deprecating passing null" RFC is nice, but has
> a flaw when applied for existing code. Applying the null coalescing code
> pattern is an easy way to make your code 8.1 compatible, but not sure if
> it adds value to the application itself.
> 
> Now if this RFC is the best way to improve things if one think this is
> something of an issue with the original RFC worth addressing I don't
> know. But at least worth having a discussion about! The purist within
> me sighs a little when applying the code pattern above...


Thanks Björn,

My main concern is about backwards compatibility.

If this RFC fails, I think the only way for existing code to avoid the issue 
will be via an automated tool, but for it to do this safely, I agree with your 
suggestion, I think it will need to use null coalescing (or casting via 
`(string) $var` or `strval($var)`) at the variable sinks... but the idea of 
doing that to the ~335 parameters I've listed, is, well, ugly to say the 
least... but I'd much rather do that, instead of having projects stuck on 8.x 
(where they will inevitably stop receiving security patches).

I would like to know about the future benefits, and this is where my discussion 
with Rowan has been interesting (trying to work out the benefits vs costs)... 
but, tbh, I'm not really seeing a future benefit for breaking NULL coercion 
with internal functions (sorry). I do see benefits for consistency, and 
rejecting weird forms of type coercion (e.g. `htmlspecialchars(array())`), and 
I've tried to factor these into my RFC, but considering how NULL and an Empty 
String can often be seen as interchangeable (many developers giving no thought 
to the difference), I'm not sure how these fatal errors will help.

Craig

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

[PHP-DEV] Re: NULL Coercion Consistency

[Björn Larsson via internals]

Den 2022-04-08 kl. 19:34, skrev Craig Francis:

Hi,

I've written a new draft RFC to address the NULL coercion problems:

https://wiki.php.net/rfc/null_coercion_consistency

This is due to the result of the Allow NULL quiz:

https://quiz.craigfrancis.co.uk/

14 votes for Fatal Type Errors irrespective of `strict_types=1`;
13 votes for NULL coercion when not using `strict_types=1`;
8 votes to update some parameters to allow NULL;

I appreciate some want to force strict type checking on everyone, but I
want to make sure we have this properly documented, with names, and
explanations.

Breaking changes should be justified - if they aren't, they only
make upgrading difficult and frustrating (bad for security).

Craig


Hi,

Have been busy for a while and haven't followed all the details around
this RFC, but now back on track.

One code pattern to upgrade your code to PHP 8.1 and 9.0 is to use the
null coalescing operator like rtrim($string ?? '',...).

If one then has a legacy codebase that works flawlessly I would say that
this path is the preferred one since it requires a minimum of work. To
dig into your code base and address why the parameter ends up like null
is probably more cumbersome. OTOH, one doesn't reap the benefits of the
"Deprecate passing null to non-nullable arguments of internal functions"
RFC since it requires to much work.

One could argue that the "Deprecating passing null" RFC is nice, but has
a flaw when applied for existing code. Applying the null coalescing code
pattern is an easy way to make your code 8.1 compatible, but not sure if
it adds value to the application itself.

Now if this RFC is the best way to improve things if one think this is
something of an issue with the original RFC worth addressing I don't
know. But at least worth having a discussion about! The purist within
me sighs a little when applying the code pattern above...

r//Björn L

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php