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

[6.x] Fix routing bug that causes missing parameters to be ignored #30659

Merged
merged 2 commits into from Nov 22, 2019

Conversation

@GuidoHendriks
Copy link
Contributor

GuidoHendriks commented Nov 22, 2019

I noticed there's a bug with creating a URL. When there's a named parameter that's not in the route, it gets added to the query string. But when there's no other parameters, the missing parameter gets ignored.

For example, I had a route similar to this:

Route::delete('/foo/{bar}/test')->name('test-route');

When using it like this:

route('test-route', ['foo' => 'bar'])

Previously this would be the result: /foo/bar/test. But since the change that the name should match, this is no longer the case. Then it should fail and throw an error because there's a missing parameter, but instead it removes the parameter token and leaves an invalid URL: /foo//test?foo=bar.

@driesvints driesvints changed the title Fix routing bug that causes missing parameters to be ignored [6.x] Fix routing bug that causes missing parameters to be ignored Nov 22, 2019
@driesvints

This comment has been minimized.

Copy link
Member

driesvints commented Nov 22, 2019

Isn't this the same as #30536 ?

Also: we can't merge this on the current release branch for people who are expecting the current behavior.

@GuidoHendriks

This comment has been minimized.

Copy link
Contributor Author

GuidoHendriks commented Nov 22, 2019

With this route - with a required parameter:

Route::delete('/foo/{required-parameter}/test')->name('test-route');

How can both this:

route('test-route') // error

And this:

route('test-route', ['unrelated-parameter' => 123]) // /foo//test?unrelated-parameter=123

Both be the expected behaviour? Is the intention to enforce or not to enforce required parameters?

@taylorotwell taylorotwell merged commit f610830 into laravel:6.x Nov 22, 2019
2 checks passed
2 checks passed
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Nov 22, 2019

After reviewing with @driesvints we agree this is a bug. Thanks!

@GuidoHendriks GuidoHendriks deleted the GuidoHendriks:fix-routing-bug branch Nov 22, 2019
@driesvints

This comment has been minimized.

Copy link
Member

driesvints commented Nov 22, 2019

@GuidoHendriks

This comment has been minimized.

Copy link
Contributor Author

GuidoHendriks commented Nov 22, 2019

Thanks guys. Happy to help where I can, it’s the least in can do. 👍

maxddev added a commit to maxddev/laravel-translation-manager that referenced this pull request Nov 30, 2019
Laravel 6.6.0 fixed a routing bug that causes missing parameters to be ignored (laravel/framework#30659), breaking this package
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.