-
Notifications
You must be signed in to change notification settings - Fork 108
Order of optional parameters #899
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@samsonasik Could you look into this? |
|
I will try |
| $executeClassMethod->params = array_merge( | ||
| array_filter($executeClassMethodParams, fn(Param $param) => is_null($param->default)), | ||
| array_filter($executeClassMethodParams, fn(Param $param) => !is_null($param->default)), | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@florisluiten this seems cause errors in tests fixture, see
https://github.com/rectorphp/rector-symfony/actions/runs/20064007047/job/57548394124?pr=899
Please verify other expected output fixtures, you can test locally:
composer install
vendor/bin/phpunit
|
@florisluiten I've updated the rule, so it can keep correct attribute per line formatting. Hopefully it will fix the comma issue. |
In PHP, optional parameters should be declared after required parameters, otherwise they will be implicitly treated as required parameters.
|
Hi guys, thanks for helping me out here. I rebased on master and cherry-picked the commits. However, the tests still fail with the trailing comma's. Did I perhaps do something wrong? I did notice that in one situation, the comma between parameters is missing as well: So I must conclude that my original thought that these comma's were automatically applied is incorrect. I'm now trying to read into how this all works, hoping that better understanding makes debugging easier. |
|
On the middle params, comma is needed. To debug, you probably needs to create temporary variable, the clear up params, then add one by one params on sort. |
|
Thanks for the pointers! I'm a couple of hours later into this thing and I still haven't figured out how to do this... I give up 😞 |
Expected output and potential fix for issue with InvokableCommandInputAttributeRector