Deprecate required param after optional by nikic · Pull Request #5067 · php/php-src

nikic · 2020-01-09T11:24:11Z

This addresses https://bugs.php.net/bug.php?id=53399.

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 ?FooBar typehint on PHP versions that did not natively support it.

Since we've had nullable types since PHP 7.1 now, having a required parameter after an optional one is increasingly likely a bug rather than an intentional workaround, so we should throw a warning for this case.

carusogabriel

👍 For this one

BackEndTea · 2020-01-09T14:25:25Z

Zend/zend_compile.c

 			default_node.op_type = IS_UNUSED;
 			op_array->required_num_args = i + 1;
+			if (have_optional_param) {
+				zend_error(E_COMPILE_WARNING, "Required parameter $%s follows optional parameter",


Maybe the message should also say this will become an error in a later PHP version. (If that is the plan)

nikic · 2020-01-09T14:45:06Z

Per the ML discussion, I've changed this to throw a deprecation warning instead.

ostrolucky · 2020-01-09T19:45:00Z

This looks controversial to me, there should be an RFC. string foo = null is common in other languages.

Girgias · 2020-01-09T20:40:48Z

This looks controversial to me, there should be an RFC. string foo = null is common in other languages.

This is not to remove this altogether, it's to deprecate the fact that you have a default value on some parameters before some which don't, meaning that you have an optional param before mandatory ones.

ostrolucky · 2020-01-09T20:43:19Z

I stand corrected, that shouldn't be controversial 👍

vvval · 2020-01-13T15:21:52Z

And what if an optional param doesn't have a type?

/**
 * @param mixed|null $param
 * @param            $param2
 */
function test($param = null, $param2)

Girgias · 2020-01-13T20:50:24Z

And what if an optional param doesn't have a type?
/**
 * @param mixed|null $param
 * @param            $param2
 */
function test($param = null, $param2)

This case is also covered and would be deprecated.

ostrolucky · 2020-01-13T20:56:37Z

And what if an optional param doesn't have a type?
/**
 * @param mixed|null $param
 * @param            $param2
 */
function test($param = null, $param2)
This case is also covered and would be deprecated.

What's the alternative for that though? There is no mixed type so you can't replace it with function test(?mixed $param, $param)

edit: we can do function test($param, $param2) of course 🤦‍♂

Girgias · 2020-01-13T21:00:16Z

And what if an optional param doesn't have a type?
/**
 * @param mixed|null $param
 * @param            $param2
 */
function test($param = null, $param2)
This case is also covered and would be deprecated.
What's the alternative for that though? There is no mixed type so you can't replace it with function test(?mixed $param, $param)

The alternative is to have your optional parameter in second place instead of first.

ostrolucky · 2020-01-13T21:02:41Z

You can't swap arguments willy nilly, that would cause BC breaks.

Girgias · 2020-01-13T21:04:05Z

You can't swap arguments willy nilly, that would cause BC breaks.

There will be a full PHP major cycle for libraries to fix this.
And BC should not be kept for clearly nonsensical code IMHO.

nikic · 2020-01-13T21:12:25Z

function test($param = null, $param2) is strictly equivalent to function test($param, $param2). You can just drop the useless and confusing = null, there are no BC implications whatsoever for that case.

…tructors (see #4065) Description ----------- Fixes #4055 This PR swaps the first two parameters of the `AsContentElement` and `AsFrontendModule` constructors. This was not released yet, so no BC issue. Background info: As of PHP8.0 having optional parameters before mandatory ones is deprecated. Here is an explanation why the current code did not trigger a deprecation warning and behaved just like two mandatory parameters: php/php-src#5067 (comment) Commits ------- 9058274 fix order of parameters 6df2476 adjust tests 5bd86a5 use miscellaneous as the default type and fix variadic argument type hint 4356ed6 prevent PHP<8.0 jobs from autoloading test cases with PHP8 syntax 8a1d2d8 add 'mixed' type hint for variadic attributes 1f396f9 improve comment 942fc37 Update core-bundle/tests/DependencyInjection/ContaoCoreExtensionTest.php e413bf3 🦊

…tructors (see #4065) Description ----------- Fixes #4055 This PR swaps the first two parameters of the `AsContentElement` and `AsFrontendModule` constructors. This was not released yet, so no BC issue. Background info: As of PHP8.0 having optional parameters before mandatory ones is deprecated. Here is an explanation why the current code did not trigger a deprecation warning and behaved just like two mandatory parameters: php/php-src#5067 (comment) Commits ------- 90582748 fix order of parameters 6df2476e adjust tests 5bd86a50 use miscellaneous as the default type and fix variadic argument type hint 4356ed66 prevent PHP<8.0 jobs from autoloading test cases with PHP8 syntax 8a1d2d82 add 'mixed' type hint for variadic attributes 1f396f9f improve comment 942fc372 Update core-bundle/tests/DependencyInjection/ContaoCoreExtensionTest.php e413bf33 🦊

…rning With this default value, I get a warning like: Deprecated: Required parameter $meta_key follows optional parameter $object_id More information: php/php-src#5067

nikic added the Feature label Jan 9, 2020

nikic force-pushed the required-after-optional-warning branch from 9474411 to c5de589 Compare January 9, 2020 11:45

carusogabriel approved these changes Jan 9, 2020

View reviewed changes

BackEndTea reviewed Jan 9, 2020

View reviewed changes

nikic changed the title ~~Throw warning if required param follows optional~~ Deprecate required param after optional Jan 9, 2020

NikoGrano approved these changes Jan 22, 2020 •

edited

Loading

View reviewed changes

nikic added 2 commits February 5, 2020 10:30

Throw warning is required param follows optional

509482a

Don't throw deprecation for poor-mans nullable types

b63436a

nikic force-pushed the required-after-optional-warning branch from 60b7d5f to b63436a Compare February 5, 2020 09:45

php-pulls closed this in 3b08f53 Feb 18, 2020

carusogabriel added this to the PHP 8.0 milestone May 29, 2020

fvsch mentioned this pull request Nov 27, 2020

PHP8: Deprecation warning for required parameter after optional parameters getkirby/kirby#2956

Closed

iluuu1994 mentioned this pull request Feb 3, 2022

No deprecation warning is triggered for nullable optional parameters before required ones #8024

Closed

m-vo mentioned this pull request Feb 3, 2022

Fix order of parameters in AsContentElement and AsFrontendModule constructors contao/contao#4065

Merged

KapitanOczywisty mentioned this pull request Jun 19, 2023

null default-value parameter before required parameter is silently ignored #11485

Closed

iluuu1994 mentioned this pull request Mar 21, 2025

Reflection does not detect optional parameter when they are in an invalid order and PHP does not see this as syntax error. #18127

Closed

Conversation

nikic commented Jan 9, 2020

Uh oh!

carusogabriel left a comment

Choose a reason for hiding this comment

Uh oh!

BackEndTea Jan 9, 2020

Choose a reason for hiding this comment

Uh oh!

nikic commented Jan 9, 2020

Uh oh!

ostrolucky commented Jan 9, 2020

Uh oh!

Girgias commented Jan 9, 2020

Uh oh!

ostrolucky commented Jan 9, 2020

Uh oh!

vvval commented Jan 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Girgias commented Jan 13, 2020

Uh oh!

ostrolucky commented Jan 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Girgias commented Jan 13, 2020

Uh oh!

ostrolucky commented Jan 13, 2020

Uh oh!

Girgias commented Jan 13, 2020

Uh oh!

nikic commented Jan 13, 2020

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vvval commented Jan 13, 2020 •

edited

Loading

ostrolucky commented Jan 13, 2020 •

edited

Loading