Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:112088
MIME-Version: 1.0
References: <DM6PR07MB6618FA11638F81F3AC01EEC4F91F0@DM6PR07MB6618.namprd07.prod.outlook.com>
In-Reply-To: <DM6PR07MB6618FA11638F81F3AC01EEC4F91F0@DM6PR07MB6618.namprd07.prod.outlook.com>
Date: Wed, 21 Oct 2020 12:15:17 +0200
Message-ID: <CAF+90c994V_NcUb3+zNBR0WRY0u6RMuMnogULEp1NFe_o5cvyQ@mail.gmail.com>
To: tyson andre <tysonandre775@hotmail.com>
Cc: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000c1167e05b22ba0ae"
Subject: Re: [PHP-DEV] Idea: Warning about named parameters with a missing constructor?
From: nikita.ppv@gmail.com (Nikita Popov)

--000000000000c1167e05b22ba0ae
Content-Type: text/plain; charset="UTF-8"

On Wed, Oct 21, 2020 at 1:01 AM tyson andre <tysonandre775@hotmail.com>
wrote:

> Hi internals,
>
> Currently, if there is no constructor, php handles it just like
> `__construct(...$args) {}`,
> both for positional and named parameters. Is there any interest in
> changing that to be a deprecation warning if one or more parameters are
> passed?
> (or if 1 or more named parameters are passed)
>
> The default behavior of missing constructors would result in somewhat
> surprising behavior
> for `new $className(paramName: 123)` if this was used unexpectedly
> (silently ignoring it)
> (Desired behavior would be to deprecate/warn/throw)
>
> ```
> php > class A {}
> php > var_export(new A(paramName: 123));
> A::__set_state(array(
> ))
> php > var_export(new stdClass(paramName: 123));
> (object) array(
> )
> php > var_export(new stdClass(123));
> (object) array(
> )
> ```
>
> (I don't think https://wiki.php.net/rfc/named_params mentioned this
> (the current behavior follows from handling of extra positional
> parameters),
> and this mistake is probably unlikely in practice,
> but if it were to occur it would very likely be a bug)
>
> PHP already throws ArgumentCountError for passing too many parameters for
> internal methods
> so the default method not throwing is a surprise. I haven't yet looked
> into how much code would break if this were changed.
>
> Aside: If a static analyzer such as Phan, Psalm, or PHPStan are set up,
> those bugs could be detected easily,
> so the impact seems low, but I wonder if this type of bug could cause
> confusion (or feel unexpected)
> for those who are more familiar with other languages than they are with
> php.
>
> Thanks,
> - Tyson
>

Thanks for bringing this up Tyson!

There's two ways to interpret how this works:
1. We're calling an implicit constructor with signature __construct() {},
in which case unknown named args should not be accepted.
2. We're calling an implicit constructor with signature
__construct(...$args) {}, in which case unknown named args should be
accepted.

I believe we should be following 1. here and have put up
https://github.com/php/php-src/pull/6364 to fix this issue. (This actually
happens to match what we were doing prior to PHP 8, I changed the signature
of the pass function for unrelated reasons without considering this
interaction.)

As to the case of normal positional arguments, I agree that we should move
towards making that an error as well, but this will need a deprecation
phase, as it is long standing behavior. There's also the more general
question of passing additional arguments to userland functions, but that
one is a lot more controversial.

Regards,
Nikita

--000000000000c1167e05b22ba0ae--