Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:117835
MIME-Version: 1.0
References: <CAFv4g+Ej4JbM9SzQ0iU-d8C4T+2XNo=8jnJ1uwfQ=skZZXx9Tg@mail.gmail.com>
 <1755E8B5-229B-47B2-BBAF-B5E014F5473D@craigfrancis.co.uk> <1180af01-080f-ee0a-3159-74bf7e0a8aea@gmail.com>
In-Reply-To: <1180af01-080f-ee0a-3159-74bf7e0a8aea@gmail.com>
Date: Mon, 30 May 2022 16:04:18 +0200
Message-ID: <CAC5V6ghk2RcWU_ZgEAdO1CeDjdH_BULZ8Sdy9qY3pCfPaQNfPg@mail.gmail.com>
To: Rowan Tommins <rowan.collins@gmail.com>
Cc: internals@lists.php.net
Content-Type: text/plain; charset="UTF-8"
Subject: Re: [PHP-DEV] Re: NULL Coercion Consistency
From: guilliam.xavier@gmail.com (Guilliam Xavier)

On Tue, May 24, 2022 at 10:47 AM Rowan Tommins <rowan.collins@gmail.com> 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