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

link_to_next_page does not generate proper links with nested params as of version 1.0.0 #873

Closed
5t111111 opened this issue Mar 8, 2017 · 3 comments · Fixed by #874
Closed

Comments

@5t111111
Copy link
Contributor

5t111111 commented Mar 8, 2017

As of version 1.0.0, link_to_next_page(and link_to_previous_page) helper seems not to generate proper links when it receives nested params.

Given the situation that you want a nested param which has query for its key. despite that it should be represented as a sort of query[genre]=crustcore in a link href path, these helpers generate something like query=genre=crustcore. As a result of that, params which are available in controllers have not got parsed correctly.

I have never encountered this issue when using paginate helper.

Minimal example of code to reproduce

Assuming there is @albums = Album.all.page(params[:page]) in your controller:

<%= link_to_next_page @albums, 'Next Page', params: params %>

Then when you add ?query[genre]=crustcore to the current url shown in your web browser, the above example generates:

on version 1.0.x

<a rel="next" href="/?page=2&query=genre%3Dcrustcore">Next Page</a>

on version 0.17.0

<a rel="next" href="/?page=2&query%5Bgenre%5D=crustcore">Next Page</a>

This is the expected link.

@yuki24
Copy link
Member

yuki24 commented Mar 8, 2017

There is a test case that checks the exact same case you described above. Could you add a test that fails and send a pull request so we can replicate your issue locally?

Also, as of c0619af, both the link_to_next_page and link_to_previous_page methods automatically add params to the generated URLs so you don't have to do so.

@5t111111
Copy link
Contributor Author

5t111111 commented Mar 8, 2017

Thanks!

Let me get jumping to the conclusion. When I remove params argument as you explain, the problem has gone.

Also, I have tried to move this line to the top of the method for checking, the problem has been solved as well.

So, undoubtedly the cause of the problem is passing params as argument, and this issue seems to be incorrect use of the api rather than a kind of bug (in my case, passing params is to explicitly permit them) .

However, even if it is the issue caused by unusual usage, I think this behavior is not intended, and I also found that the problem is complex than I thought.

I've tried to write fail tests by adding test cases with problematic params, but every test has passed. Then after further investigation, the problem occurs only when nested ActionController::Parameters instance are passed to the method as params argumemt.

What makes the problem complicated is that even once nested inner params' key is directly accessed in controllers or views, that automatically changes from Hash to ActionController::Parameters.

# in SomeController#index
p param
#=><ActionController::Parameters {"query"=>{"genre"=>"aaa"}, "controller"=>"albums", "action"=>"index"} permitted: false>

p params[:query]
#=> <ActionController::Parameters {"genre"=>"aaa"}

p param
#=> <ActionController::Parameters {"query"=><ActionController::Parameters {"genre"=>"aaa"} permitted: false>, "controller"=>"albums", "action"=>"index"} permitted: false>

For the moment I'm not sure this is intended by Rails' ActonController, at least it makes difficult to point out the cause of the problem.

Anyway, I'll try to write a test case to represent the above behavior when I have time (maybe weekend). But if you think there's nothing needs to be done about this issue. Plesase go ahead and close this.

@5t111111
Copy link
Contributor Author

5t111111 commented Mar 9, 2017

I've found the root cause of the problem and added some tests that fail.
The pull request #874 also contains a fix for the issue.

@yuki24 yuki24 added the Bug label Aug 11, 2017
@yuki24 yuki24 modified the milestones: 1.2.0, 1.1.0 Sep 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants