Skip to content
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

[11.x] Removing unused var assignment in Illuminate Router #53575

Merged
merged 2 commits into from
Nov 19, 2024

Conversation

Carnicero90
Copy link
Contributor

It won't benefit any end user, it can't break any existing feature since the variable was unused, it doesn't make building applications easier: I just noticed that while debugging some stuff in the vendor of my application.

@osbre
Copy link
Contributor

osbre commented Nov 19, 2024

@Carnicero90 It was meant to help with readability. I think it came from the days when there were no named arguments in PHP. nowadays it should be:

$route->setAction($this->mergeWithLastGroup(
    $route->getAction(),
    prependExistingPrefix: false,
));

@Carnicero90
Copy link
Contributor Author

Aaah, I see. Here you go.

@rodrigopedra
Copy link
Contributor

Named arguments are not covered by Laravel's backwards compatibility guidelines. We may choose to rename function arguments when necessary in order to improve the Laravel codebase. Therefore, using named arguments when calling Laravel methods should be done cautiously and with the understanding that the parameter names may change in the future.

Reference: https://laravel.com/docs/11.x/releases#named-arguments

@taylorotwell taylorotwell merged commit 47f4309 into laravel:11.x Nov 19, 2024
31 checks passed
@Carnicero90
Copy link
Contributor Author

Named arguments are not covered by Laravel's backwards compatibility guidelines. We may choose to rename function arguments when necessary in order to improve the Laravel codebase. Therefore, using named arguments when calling Laravel methods should be done cautiously and with the understanding that the parameter names may change in the future.

Reference: https://laravel.com/docs/11.x/releases#named-arguments

Seems to be more of a user guide though. Still, it doesnt matter all that much, since this is a pretty useless pr overall.

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.

4 participants