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

Changes to "route" parameter in Form::open in v6.1.1 #648

Closed
rogermui opened this issue May 18, 2020 · 6 comments
Closed

Changes to "route" parameter in Form::open in v6.1.1 #648

rogermui opened this issue May 18, 2020 · 6 comments

Comments

@rogermui
Copy link

With pull request #627, the way of passing multiple route parameters to Form::open changed under the guise of a bug fix from:
Form::open(['route' => ['route.name', 'id' => 1, 'foo' => 'bar']])
to:
Form::open(['route' => ['route.name', ['id' => 1, 'foo' => 'bar']]])

Can we maintain backwards compatibility?

@ChrisThompsonTLDR @mlantz

@ChrisThompsonTLDR
Copy link
Contributor

https://laravel.com/docs/7.x/routing#named-routes

If the named route defines parameters, you may pass the parameters as the second argument to the route function.

Form::open(['route' => ['route.name', 'id' => 1, 'foo' => 'bar']]) doesn't seem to follow that?

@rogermui
Copy link
Author

Yes, the ['route.name', 'id' => 1, 'foo' => 'bar'] syntax is undocumented. Yes, it doesn't match route().

But this syntax was supported deliberately with the use of array_slice and has been around since laravel/framework v4.0 (laravel/framework@2bc08fe) from 7 years ago. It should not be dropped for such reasons.

@rogermui
Copy link
Author

Please do not risk compromising 7 years of people's code to facilitate one developer's use cases. #627 didn't even update getControllerAction, which should be consistent with getRouteAction.

@Bouhnosaure
Copy link

Bouhnosaure commented May 19, 2020

I don't know what was going on until my deployment failed after the update.

I manage a lot of web app that are in production, and lately it seems that everything broke 😅 and i know my comment is not useful at all but i agree with @rogermui, i use collective for 5 years now and it's not safe to change those 'legacy' behaviors, at least this one, which is a real breaking change !

So, for my part, i was a huge fan of this library but i think this time i'll have to write my own forms or change the way url are managed with forms.

i'll revert to the 6.1.0 and change my 'route' to 'url' and use Laravel own route helper.

@ChrisThompsonTLDR
Copy link
Contributor

I apologize for breaking this package for so many people. I wasn't aware that it was being used in an undocumented way.

I attempted to be clear in #627 with what I was attempting to fix.

I agree that a pull request shouldn't break existing functionality.

@mlantz
Copy link
Member

mlantz commented May 19, 2020

Sorry all for causing any issues. I was doing reviews and should have dug deeper into this PR. Mistakes happen, v6.1.2 contains the PR which reverted the impacts.

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

No branches or pull requests

4 participants