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 RouteUrlGenerator accepting empty string for required paramenter #30714

Conversation

MrCrayon
Copy link
Contributor

#30659 fixed RouteUrlGenerator not failing when a required parameter is not passed but it's still not failing if a required parameter is passed empty.

Route::get('/foo/{required}/test/{optional?}')->name('test-route');
route('test-route', ['required' => null, 'optional' => null]);
// UrlGenerationException

route('test-route', ['required' => '', 'optional' => null]);
// /foo//test

I don't think this could be considered an expected behaviour since normally required means set and not empty.

My PR checks if the required parameter is set and it's not an empty string, if this check fails it will look for default Parameter and as last step checks if parameter was optional.

@driesvints driesvints changed the title Fix RouteUrlGenerator accepting empty string for required paramenter [6.x] Fix RouteUrlGenerator accepting empty string for required paramenter Nov 29, 2019
@driesvints
Copy link
Member

@MrCrayon tests are failing and you probably want to add a test specifically for this scenario.

@MrCrayon
Copy link
Contributor Author

@driesvints yes, it's quite strange that route failing on tests it's ok on my end.

Route::get('foo/bar/{baz}')->name('foobar');
route('foobar', ['baz' => 'Taylor']);
// /foo/bar/Taylor

I'll add some tests and try to understand what's happening.

@MrCrayon
Copy link
Contributor Author

Test failing was the one with optional parameter passed without key:

Route::get('foo/bar/{baz?}')->name('foobar');
route('foobar', 'taylor');
// or
route('foobar', ['taylor']);

What I needed to do is removing empty parameter from $parameters and let the other methods take care of the rest.

@taylorotwell taylorotwell merged commit dfb4c1c into laravel:6.x Dec 2, 2019
@driesvints
Copy link
Member

@MrCrayon this broke a scenario where 0 is used as a route parameter: #30760

@Vercoutere
Copy link

Just wanted to add that this was a breaking change for us too, not because we were using 0 as a parameter, but because we were actually relying on an empty string passing through the system.

It allowed to use an optional subdomain with the following pattern:

Route::pattern('subdomain', '(^[a-z0-9\-]+\.)*');

Kind of bummed this doesn't work anymore as I don't see another straight forward way for us to implement the same behaviour.

@MrCrayon
Copy link
Contributor Author

@Vercoutere check is intended for required parameters but you should still be able to use an optional parameter
https://laravel.com/docs/6.x/routing#parameters-optional-parameters

@Vercoutere
Copy link

@MrCrayon that's right, but unfortunately optional parameters only seem to work when they are the last parameter in a given route. Using an optional parameter for a subdomain, followed by a domain parameter, followed by a URI with other parameters doesn't work.

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