Warn when declaring required parameter after optional one

5 years ago by Nikita Popov — view source — reply

unread

Hi internals,

I've created https://github.com/php/php-src/pull/5067 to make code like

function test($foo = null, $bar) {}

throw a warning:

Warning: Required parameter $bar follows optional parameter

Historically, having an "optional" parameter before a required one was
useful for poor man's nullable types. That is, one could write

function test(FooBar $param = null, $param2)

to get an effective

function test(?FooBar $param, $param2)

signature on old PHP versions that did not have native support for nullable
types.

Since nullable types have been available since PHP 7.1, having a required
parameter after an optional one is increasingly likely a bug rather than an
intentional workaround, so I think it would be good to throw a warning for
this case.

What do you think?

Nikita

5 years ago by Sebastian Bergmann — view source — reply

unread

Am 09.01.2020 um 13:26 schrieb Nikita Popov:

What do you think?

I would prefer erroring out over just emitting a warning.

5 years ago by Mark Randall — view source — reply

unread

I would prefer erroring out over just emitting a warning.

I'm in agreement with Sebastian, a compile time error is the way to go.

I could understand a warning or deprecation in a minor release, but as
there's already so many engine level exceptions landing in 8.0, I think
it makes sense to throw this on the pile as well to be swept up in the
same pass of migrations that people do in preperation for 8.0.

Better to get the pain over and done with, rather than drag it out over
multiple versions, as I'd guess that even if it was a warning now, it
would end up a compile error a few years from now.

--
Mark Randall

5 years ago by Sebastian Bergmann — view source — reply

unread

Am 09.01.2020 um 13:31 schrieb Sebastian Bergmann:

I would prefer erroring out over just emitting a warning.

What I meant, of course, was deprecation (or warning) first before
erroring out. Sorry.

5 years ago by Bob Weinand — view source — reply

unread

Hey,

While I generally agree that it likely is a bug in new code, I would rather deprecate it than warn or even error: the change would make it impossible to retain a type without warning while preserving compatibility with an old PHP version and making incremental migrations harder (would then not be possible to write warning-free code running on 7.0 and 8.0 at the same time while retaining type information).

I would like to see it deprecated and then removed in 4+ years.

Bob

Am 09.01.2020 um 13:27 schrieb Nikita Popov nikita.ppv@gmail.com:

Hi internals,

I've created https://github.com/php/php-src/pull/5067 to make code like

function test($foo = null, $bar) {}

throw a warning:

Warning: Required parameter $bar follows optional parameter

Historically, having an "optional" parameter before a required one was
useful for poor man's nullable types. That is, one could write

function test(FooBar $param = null, $param2)

to get an effective

function test(?FooBar $param, $param2)

signature on old PHP versions that did not have native support for nullable
types.

Since nullable types have been available since PHP 7.1, having a required
parameter after an optional one is increasingly likely a bug rather than an
intentional workaround, so I think it would be good to throw a warning for
this case.

What do you think?

Nikita

5 years ago by Nikita Popov — view source — reply

unread

Hey,

While I generally agree that it likely is a bug in new code, I would
rather deprecate it than warn or even error: the change would make it
impossible to retain a type without warning while preserving compatibility
with an old PHP version and making incremental migrations harder (would
then not be possible to write warning-free code running on 7.0 and 8.0 at
the same time while retaining type information).

I would like to see it deprecated and then removed in 4+ years.

Bob

As deprecation and later removal seem to be preferred by multiple people,
I've changed the PR to implementation that instead.

Nikita

Am 09.01.2020 um 13:27 schrieb Nikita Popov nikita.ppv@gmail.com:

Hi internals,

I've created https://github.com/php/php-src/pull/5067 to make code like

function test($foo = null, $bar) {}

throw a warning:

Warning: Required parameter $bar follows optional parameter

Historically, having an "optional" parameter before a required one was
useful for poor man's nullable types. That is, one could write

function test(FooBar $param = null, $param2)

to get an effective

function test(?FooBar $param, $param2)

signature on old PHP versions that did not have native support for
nullable
types.

Since nullable types have been available since PHP 7.1, having a required
parameter after an optional one is increasingly likely a bug rather than
an
intentional workaround, so I think it would be good to throw a warning
for
this case.

What do you think?

Nikita

5 years ago by Niklas Keller — view source — reply

unread

Hi Nikita,

while this is a rather small change, it has quite some BC impact, as
not all old code has been adjusted to run on PHP 7.1+ only using
nullable types.

I'd like to see an RFC with a vote for this.

Regards,
Niklas

5 years ago by Olumide Samson — view source — reply

unread

Should it be a warning or an error?

Sent from Mail for Windows 10

From: Niklas Keller
Sent: Saturday, January 11, 2020 2:35 PM
To: Nikita Popov
Cc: Bob Weinand; PHP internals
Subject: Re: [PHP-DEV] Warn when declaring required parameter after optionalone

Hi Nikita,

while this is a rather small change, it has quite some BC impact, as
not all old code has been adjusted to run on PHP 7.1+ only using
nullable types.

I'd like to see an RFC with a vote for this.

Regards,
Niklas

5 years ago by Olumide Samson — view source — reply

unread

Error(in PHP 8) after warning or deprecation in 7.xnything more preferred…

Sent from Mail for Windows 10

From: Olumide Samson
Sent: Saturday, January 11, 2020 2:42 PM
To: Niklas Keller; Nikita Popov
Cc: Bob Weinand; PHP internals
Subject: RE: [PHP-DEV] Warn when declaring required parameter afteroptionalone

Should it be a warning or an error?

Sent from Mail for Windows 10

From: Niklas Keller
Sent: Saturday, January 11, 2020 2:35 PM
To: Nikita Popov
Cc: Bob Weinand; PHP internals
Subject: Re: [PHP-DEV] Warn when declaring required parameter after optionalone

Hi Nikita,

while this is a rather small change, it has quite some BC impact, as
not all old code has been adjusted to run on PHP 7.1+ only using
nullable types.

I'd like to see an RFC with a vote for this.

Regards,
Niklas

5 years ago by Nikita Popov — view source — reply

unread

Hi Nikita,

while this is a rather small change, it has quite some BC impact, as
not all old code has been adjusted to run on PHP 7.1+ only using
nullable types.

I'd like to see an RFC with a vote for this.

Regards,
Niklas

I was interested in seeing how prevalent this pattern, is, so I ran some
analysis on the top 2k composer packages. I found 527 signatures that would
throw a deprecation warning with this change. Of these 187 are potentially
used as "poor man's nullable types" (the optional argument has both a type
and a null default), while the other 340 are definite bugs.

Regards,
Nikita

5 years ago by Aran Reeks — view source — reply

unread

Hi all,

Whilst I totally see the benefits of this, I'm not sure if this is a job
for PHP itself, rather a coding standard enforced optionally by something
like PHPCS. It's certainly a bad practice, but I'm not sure the benefit
will be worth the refactors required.

I feel we might be an interesting example, however, it'd cause us a lot of
pain for the following reason:

We have a portion of our codebase (shared over hundreds of websites) which
is symlinked into each of the websites so the core functionality can be
easily updated when required on all sites. These sites are staggered at
present across multiple PHP versions whilst being migrated (and due to the
number of sites to migrate, they will be for some time).

If this change were to come into effect, the same code we've safely had
symlinked running across multiple PHP versions would suddenly become
incompatible on the latest version, with no easy way to be suppressed which
would mean we have to fork things and maintain two codebases.

I appreciate this isn't likely to be a normal workflow for people, but it
is an example of how it could cause unintended pain.

Just my thoughts anyway.

Cheers,
Aran

Hi Nikita,

while this is a rather small change, it has quite some BC impact, as
not all old code has been adjusted to run on PHP 7.1+ only using
nullable types.

I'd like to see an RFC with a vote for this.

Regards,
Niklas

I was interested in seeing how prevalent this pattern, is, so I ran some
analysis on the top 2k composer packages. I found 527 signatures that would
throw a deprecation warning with this change. Of these 187 are potentially
used as "poor man's nullable types" (the optional argument has both a type
and a null default), while the other 340 are definite bugs.

Regards,
Nikita

5 years ago by Theodore Brown — view source — reply

unread

I've created https://github.com/php/php-src/pull/5067 to make code like
function test($foo = null, $bar) {}
throw a warning

I was interested in seeing how prevalent this pattern, is, so I ran
some analysis on the top 2k composer packages. I found 527 signatures
that would throw a deprecation warning with this change. Of these 187
are potentially used as "poor man's nullable types" (the optional
argument has both a type and a null default), while the other 340 are
definite bugs.

Given that most of these usages are definite bugs, I'm in favor of
deprecating this in PHP 8 and making it a compile error in PHP 9. This
should provide plenty of time for codebases to migrate to the simpler
nullable types syntax for the minority of usages that aren't bugs.

Theodore

5 years ago by Andrea Faulds — view source — reply

unread

Hi Nikita,

Nikita Popov wrote:

Since nullable types have been available since PHP 7.1, having a required
parameter after an optional one is increasingly likely a bug rather than an
intentional workaround, so I think it would be good to throw a warning for
this case.

Wouldn't it be trivial to special-case = NULL here to not cause a
warning, or to cause an E_NOTICE instead of an E_WARNING? That would
make this less annoying for old code. I don't know whether I think this
worth doing or not, I just want to point out the possibility.

Thanks,
Andrea

5 years ago by Gert — view source — reply

unread

I could understand a warning or deprecation in a minor release, but as
there's already so many engine level exceptions landing in 8.0, I think
it makes sense to throw this on the pile as well to be swept up in the
same pass of migrations that people do in preperation for 8.0.

If this would've generated a warning in 7.4 then making this a compile
time error is acceptable.
But going from nothing to an error without any kind of warning makes
the upgrade harder than it needs to be.
I think this should be a warning, and then an exception in 9.0. Unless
there is gonna be a PHP 7.5
where only deprecations get added, in that case an error in 8.0 would be fine.

--
Gert

I would prefer erroring out over just emitting a warning.

I'm in agreement with Sebastian, a compile time error is the way to go.

I could understand a warning or deprecation in a minor release, but as
there's already so many engine level exceptions landing in 8.0, I think
it makes sense to throw this on the pile as well to be swept up in the
same pass of migrations that people do in preperation for 8.0.

Better to get the pain over and done with, rather than drag it out over
multiple versions, as I'd guess that even if it was a warning now, it
would end up a compile error a few years from now.

--
Mark Randall

5 years ago by Nikita Popov — view source — reply

unread

Hi Nikita,

Nikita Popov wrote:

Since nullable types have been available since PHP 7.1, having a required
parameter after an optional one is increasingly likely a bug rather than
an
intentional workaround, so I think it would be good to throw a warning
for
this case.

Wouldn't it be trivial to special-case = NULL here to not cause a
warning, or to cause an E_NOTICE instead of an E_WARNING? That would
make this less annoying for old code. I don't know whether I think this
worth doing or not, I just want to point out the possibility.

I like this idea and have updated the pull request to ignore the "Type
$param = null" case, so the deprecation should now just catch the
"definitely" incorrect signatures.

So to summarize the current state:

function test($foo = "bar", $baz) {}
// Deprecated: Required parameter $baz follows optional parameter $foo

function test(Abc $foo = null, $baz) {}
// No warnings, works fine!

With that adjustment made, are there any further concerns about this change?

Regards,
Nikita

5 years ago by Guilliam Xavier — view source — reply

unread

Hi,

I like this idea and have updated the pull request to ignore the "Type
$param = null" case, so the deprecation should now just catch the
"definitely" incorrect signatures.

So to summarize the current state:

function test($foo = "bar", $baz) {}
// Deprecated: Required parameter $baz follows optional parameter $foo

function test(Abc $foo = null, $baz) {}
// No warnings, works fine!

With that adjustment made, are there any further concerns about this
change?

Can you please just clarify which of the following will emit a deprecation?

function f1($a = null, $b) {}
function f2(A $a = null, $b) {}
function f3(?A $a = null, $b) {}

(I think f1 will, f2 won't, but f3?)

Thanks

Regards,
Nikita

--
Guilliam Xavier

5 years ago by Nikita Popov — view source — reply

unread

On Wed, Feb 5, 2020 at 1:15 PM Guilliam Xavier guilliam.xavier@gmail.com
wrote:

Hi,

I like this idea and have updated the pull request to ignore the "Type
$param = null" case, so the deprecation should now just catch the
"definitely" incorrect signatures.

So to summarize the current state:

function test($foo = "bar", $baz) {}
// Deprecated: Required parameter $baz follows optional parameter $foo

function test(Abc $foo = null, $baz) {}
// No warnings, works fine!

With that adjustment made, are there any further concerns about this
change?

Can you please just clarify which of the following will emit a deprecation?

function f1($a = null, $b) {}
function f2(A $a = null, $b) {}
function f3(?A $a = null, $b) {}

(I think f1 will, f2 won't, but f3?

f1 will, f2 won't, f3 also won't. f3 could technically emit one, but I'm
not sure it's worth having a special case of a special case for this.

Nikita