Skip to content

[CodeQuality] Change behaviour of OptionalParametersAfterRequiredRector to fill null default value when previous param is optional#7222

Merged
TomasVotruba merged 6 commits intomainfrom
change-behaviour
Sep 6, 2025
Merged

[CodeQuality] Change behaviour of OptionalParametersAfterRequiredRector to fill null default value when previous param is optional#7222
TomasVotruba merged 6 commits intomainfrom
change-behaviour

Conversation

@samsonasik
Copy link
Member

@afaibyshev @calebdw @TomasVotruba this is possible solution for

Fixes rectorphp/rector#9317

instead of swap param/argument, fill it with default value null instead.

…or to fill null default value when previous param is optional
@samsonasik samsonasik marked this pull request as draft September 6, 2025 10:18
@samsonasik
Copy link
Member Author

I will verify the usage when on types as well.

@samsonasik
Copy link
Member Author

I add for param typed dd8cca8

@samsonasik
Copy link
Member Author

I add requirement for PhpVersionFeature::NULLABLE_TYPE for it, so ensure mark to nullable on param always work 7eb8a4e

@samsonasik samsonasik marked this pull request as ready for review September 6, 2025 11:37
@samsonasik
Copy link
Member Author

All checks have passed 🎉 @TomasVotruba it is ready for review.

@TomasVotruba
Copy link
Member

Looks good 👍

@TomasVotruba TomasVotruba merged commit 89accb6 into main Sep 6, 2025
49 checks passed
@TomasVotruba TomasVotruba deleted the change-behaviour branch September 6, 2025 13:19
@samsonasik
Copy link
Member Author

samsonasik commented Sep 7, 2025

This seems can be improved for more reasonable default value. I am thinking of add null default value should only on Nullable, UnionType and/or non-scalar, otherwise, use its basic type value to avoid next user use invalid, eg nullable and use as arithmetic task.

MixedType: Ok to add null default value

  • $a --> null
  • mixed $a = null

Nullable/Union with nullable: Ok to add null default value

?AnyTipe --> null
A|B|null --> null

Scalar:

  • int --> 0
  • float -> 0.0
  • bool --> false
  • string --> ""

Array

  • array --> []

Union + Scalar: use first type basic value

  • int+float --> 0
  • int + string --> 0
  • bool + int/float --> false
  • etc

Object type + union: nullable

  • stdClass|DateTime = null|stdClass|DateTime
  • stdClass = ?stdClass

This will require some proper maps, I will look into it.

@samsonasik
Copy link
Member Author

samsonasik commented Sep 7, 2025

After some thinking, just remove previous params default is better, since even on named argument usage, the value is always null if not defined, see

https://3v4l.org/0jdGl

I will look into it..., to see if there is drawback, eg; on override method, etc

@samsonasik
Copy link
Member Author

On PHP 8.0, it seems not fatal error when optional is not filled before required, see https://3v4l.org/0jdGl#v8.0.0

got:

Deprecated: Required parameter $req follows optional parameter $op in /in/0jdGl on line 3
1
foo

On PHP 8.1, it seems cause fatal error when optioanl is not filed before required, see https://3v4l.org/0jdGl#v8.1.12

got:

Deprecated: Optional parameter $op declared before required parameter $req is implicitly treated as a required parameter in /in/0jdGl on line 3

Fatal error: Uncaught ArgumentCountError: run(): Argument #1 ($op) not passed in /in/0jdGl:3
Stack trace:
#0 /in/0jdGl(12): run(NULL, 'foo')
#1 {main}
  thrown in /in/0jdGl on line 3

so add default reasonable value by type seems be better, I will look more deep into it :)

@samsonasik
Copy link
Member Author

I created new PR to fill reasonable default value, so default value on previous params keep working

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Parameters was swapped only for source

3 participants