Thanks Allen,
Before I start, the RFC is still a Draft, and I'm trying to find a
solution... I'll also use a bit of sarcasm, just to be entertaining/funny,
so it's not a page of boring technical stuff, I'm not arguing or saying
you're wrong, and I genuinely appreciate your views on this.
On Sun, 2 Jan 2022 at 11:27, AllenJB <php.li...@allenjb.me.uk> wrote:
> I think this RFC is trying to solve the wrong problem / only fixing one
> small portion of the "immediate" problem without paying attention to the
> bigger picture.
>
Dynamic properties do cause problems, and I'm convinced they should
deprecated, whereas NULL is a useful value.
e.g. when a value has not been set; and then providing NULL to a function
like htmlspecialchars(), I don't think should be a problem.
I feel that effort would be better spent on improving and encouraging
> use of tooling such as the PHPCompatibility CodeSniffer ruleset.
This one requires variable tracing from source to sink, not an easy task
(it's why I noted "Psalm at level 3, with no baseline").
Neither PHPCompatibility or Rector have rules for this at the moment, and
I'm pretty sure they never will; other than the really obvious ones, e.g.
`str_replace('a', NULL, $name)`, which is clearly a terrible thing to do :-)
As an aside, PHPCompatibility did the bad thing of using trim() on
getConfigData(), which can return NULL:
https://github.com/PHPCompatibility/PHPCompatibility/commit/ab7b0b82d95c82e62f9cffb0f1960f3ccbf0805e
https://github.com/squizlabs/PHP_CodeSniffer/blob/d6a58bdd8707bab1032344f33d01c0627e0f3f97/src/Config.php#L1479
As noted in the RFC, while these don't take long to fix, finding them is
tricky, it nearly always results in more code (yay?), and it is being seen
as wasting time on a fairly questionable improvement.
The migration guides could include documentation on updating existing
> code for each change
While migration guides are useful (I appreciate them), I'm focusing on this
change to avoid unnecessary work. I don't think nullable variables should
need to be explicitly cast to a string before calling these functions -
doing so doesen't seem to make the code better (if anything, it's making it
more complex).
... this RFC creates work for developers both in debugging code, and for
> developers with
> defensive programming styles to (re-)add checks to detect the exact
> class of problem these deprecations / warnings highlight.
>
For example?
e.g. `$name`, if it's set to NULL, why should `ucwords()` have a problem
with that? Won't it cause extra work having to explicitly convert it to a
string?
Consider those who aren't using Static Analysis (or they are using the
entry levels, with a baseline), it requires that everyone remembers that
while their HTML form would normally provide this value, the user can edit
the <form> in their browser DOM, or a browser extension could do something
weird (e.g. a password manager), or a network issue results in a partial
page load... and kaboom, future Fatal TypeError in the middle of
processing? yay? party time?
...all the frameworks used in the example have an additional
> default value parameter. This makes all of these (and the null
> coalesce alternative) relatively trivial to update using a regular
> expression find and replace, that most text editors can do.
They use a default value of NULL so you can determine when the value has
not been provided... should we tell them to stop doing this? default to
returning an empty string instead? I'm not sure they will accept that one.
Some of the suggested functions (parameters) to change are strange
> selections in my opinion
It's a draft, I just started with as many candidates as I could find, on
the basis that it's easier to cut back if NULL can cause a problem.
But as to your examples (gz compressing, bcmath, password_hash, and mail);
they can all be provided with user input, and other sources of NULL.
e.g. `password_hash()`, while I wouldn't advise anyone to use a blank
password, or anything less than 8 characters, this function does accept it.
And because passwords often come from a POST request, if the value is not
present (framework function returns NULL)... well, assuming this
deprecation indicates a future where this is not acceptable, what happens?
future Fatal TypeError?
This is why I have the "Future Scope" section - noting that some parameters
could be updated (in a different RFC) to complain when an Empty String or
NULL is provided. e.g. `setcookie()` already complains when `$name` is
empty, and `strpos()` probably should complain when `$needle` is empty.
> If this change were to be made, I think it would be better to severely
> restrict the list to only the most commonly used functions.
I'm fine with that, but what should be removed? Feel free to do this on or
off-list, pull request, etc. If you can, I'd really appreciate examples
where the NULL would cause a problem but an empty string is fine - i.e.
assume developers will be lazy, and just use strval() to fix the hideous
crime they have been committing.
How would code that declares strict_types be affected by this RFC?
Good question... personally I think a trim() on NULL should return an empty
string, not a fatal error, so maybe we are looking at this problem in a
slightly different way?
Craig
