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

Wrong behaviour between named parameters and query string on URL generation #10659

Closed
igorsantos07 opened this Issue Oct 19, 2015 · 15 comments

Comments

Projects
None yet
6 participants
@igorsantos07
Copy link

igorsantos07 commented Oct 19, 2015

I have a route with no arguments. When I call action('EventController@getSearch', ['place' => 'My City']) I expect the place to be concatenated as a querystring in my URL. Instead, it gets added as a URL parameter (such as event/search/My City instead of event/search?place=My+City).

Also, the URL helpers won't urlencode() parameters. Is this an expected behaviour?

@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Oct 19, 2015

What version?

@rkgrep

This comment has been minimized.

Copy link
Contributor

rkgrep commented Oct 19, 2015

Your route won't work this way, event/search/My City will generate 404 error.
You should define optional argument if it is not required: /event/search/{place?}

@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Oct 19, 2015

Your route won't work this way, event/search/My City will generate 404 error.

Isn't @igorsantos07 saying that he's getting that behaviour but thinks it's wrong?

@rkgrep

This comment has been minimized.

Copy link
Contributor

rkgrep commented Oct 19, 2015

Sorry. I misunderstood the issue.

@igorsantos07

This comment has been minimized.

Copy link
Author

igorsantos07 commented Oct 19, 2015

But I don't want my arguments in this case in the URL. I want them as query
strings. That's why my method has no argument.

My route is defined as Route::controller(), and this is Laravel 5.1,
updated a couple of days ago.
On 19 Oct 2015 04:47, "Roman Kinyakin" notifications@github.com wrote:

Your route won't work this way, event/search/My City will generate 404
error.
You should define optional argument if it is not required:
/event/search/{place?}


Reply to this email directly or view it on GitHub
#10659 (comment)
.

@igorsantos07

This comment has been minimized.

Copy link
Author

igorsantos07 commented Oct 19, 2015

Actually the wrong URL (/events/search/My city) does work, as my method has
no arguments. But that's not the URL I expected to get feeding an
associated array to action() and using a no-args method :(
On 19 Oct 2015 05:19, "Igor Santos" igorsantos07@gmail.com wrote:

But I don't want my arguments in this case in the URL. I want them as
query strings. That's why my method has no argument.

My route is defined as Route::controller(), and this is Laravel 5.1,
updated a couple of days ago.
On 19 Oct 2015 04:47, "Roman Kinyakin" notifications@github.com wrote:

Your route won't work this way, event/search/My City will generate 404
error.
You should define optional argument if it is not required:
/event/search/{place?}


Reply to this email directly or view it on GitHub
#10659 (comment)
.

@c4pone

This comment has been minimized.

Copy link

c4pone commented Oct 22, 2015

@igorsantos07 I tried to reproduce the behavior you were talking about, but I got the desired result:

event/search?place=My+City

I defined the route as following:

Route::get('event/search', ['as' => 'event', 'uses' => 'EventController@getSearch']);

Can you post your route definition ?

@igorsantos07

This comment has been minimized.

Copy link
Author

igorsantos07 commented Oct 22, 2015

Sounds even more like a bug now!

Originally, my route definition was simply Route::controller('event', 'EventController');. Using action('EventController@getSearch', ['place' => 'My City']) yields /event/search/My City.

However, if I include Route::get('event/search', ['as' => 'event', 'uses' => 'EventController@getSearch']); the line after that controller definition, the same action() call correctly generates the URL as per your comment, @c4pone. I don't even have to change it to route() or any of its arguments.

Just make things clearer, my action signature is public function getSearch() - no args at all.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Oct 22, 2015

Originally, my route definition was simply Route::controller('event', 'EventController');. Using action('EventController@getSearch', ['place' => 'My City']) yields /event/search/My City.

You need to delete the cached routes file.

@darthmaim

This comment has been minimized.

Copy link

darthmaim commented Oct 22, 2015

Thats because Route::controller generates routes with 5 optional arguments at the end (use artisan route:list to see the generated route): (event/search/{one?}/{two?}/{three?}/{four?}/{five?}. Thats just how controllers work, if you want to add place as query parameter, you have to use Route::get.

There was a PR to fix this, by generating the wildcard parameters of controller methods using reflection: #9756

@igorsantos07

This comment has been minimized.

Copy link
Author

igorsantos07 commented Oct 22, 2015

I really can't understand how such "basic" feature like this was postponed with arguments like "use resources for what's not a resource" or "too much to maintain, I prefer to have developers hardcode their routes".

Reflections are used in other frameworks to have a more meaningful and understandable API matching between methods and routes. It makes little to no sense to have five hardcoded arguments bound to actions. It makes even less sense to have to repeat some routes that should have query methods - as they're usually more meaningful - as they've been described before using the Route::controller call.

I would even add here that this poses a real bug to application routing behaviour. As this is a search, it has optional arguments that must be sent over the URL - so the search results are shareable. This generates those two issues:

  • everywhere I generate a shortcut link to a search result throughout the system, I'll have to remember the parameters order - or else, the search implementation will get screwed up
  • if I create a search form (searchs are usually made through forms, right?), I'll actually have to find out my parameters in a different way, as they'll be named instead of indexed? WTF?
@igorsantos07

This comment has been minimized.

Copy link
Author

igorsantos07 commented Oct 22, 2015

@GrahamCampbell I think you misunderstood me :) I said "my route was" because I added the second ::get() call as your example, not because my routes changed and they were cached.

@jaketoolson

This comment has been minimized.

Copy link

jaketoolson commented Oct 23, 2015

@GrahamCampbell at it again, closing things before they are resolved. @igorsantos07 just add a bunch of extra white space to your PR, then he'll surely try harder to understand.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Oct 23, 2015

There was a PR to fix this, by generating the wildcard parameters of controller methods using reflection: #9756

The PR was already rejceted, thus this is a nofix, so can be closed.

@igorsantos07

This comment has been minimized.

Copy link
Author

igorsantos07 commented Oct 25, 2015

There was indeed a reason for @GrahamCampbell to close the issue. However, my questionings raised new problems that should be addressed - probably, by ressurrecting the PRs related to static params.

But it seems these kind of bugs are not buggy enough for the maintainers to care, as it's better to just "hold off PRs" instead of discussing and fixing issues ¯_(ツ)_/¯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment