Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #4947 (closed defect: duplicate)

Opened 3 years ago

Last modified 2 years ago

[PATCH] URL rewriter shoud support deeply nested parameters

Reported by: me@julik.nl Assigned to: minam
Priority: normal Milestone: 1.2
Component: ActionPack Version: 1.2.1
Severity: normal Keywords: url_for rewriter routes
Cc: Ulysses

Description

URL rewriter (as used in url_for) should support nested parameter hashes, it is required for situations like pagination where more form parameters need to be preserved

Attachments

nested_params.diff (2.7 kB) - added by me@julik.nl on 05/01/06 12:10:30.
Necessary changes
url_rewriter_patch2.diff (4.1 kB) - added by me@julik.nl on 05/09/06 15:57:39.
Patch v2
url_rewriter_v3.diff (4.8 kB) - added by me@julik.nl on 05/11/06 11:47:37.
Final (I hope)
updated_patch.diff (5.3 kB) - added by me@julik.nl on 06/11/06 00:15:18.
Updated patch agains the new routing code

Change History

05/01/06 12:10:30 changed by me@julik.nl

  • attachment nested_params.diff added.

Necessary changes

05/09/06 01:14:24 changed by bitsweat

  • summary changed from [PATCH] URL rewriter shoud support deeply nested parameters to [XPATCH] URL rewriter shoud support deeply nested parameters.
  • type changed from defect to enhancement.
  • version set to 1.1.1.
  • milestone set to 1.2.

Pending tests.

05/09/06 10:41:43 changed by me@julik.nl

What do you mean by "pending tests"? I just added a test case for deeply nested params, it's in the diff. Does it need more test cases? If so, then which ones should that be? Or the idea is that it should expand nested params within arrays too? I don't think it's possible with the current [] array designation.

05/09/06 15:28:06 changed by bitsweat

I'm not sure what it expands or how deeply and neither the test case nor the docs say. Do multilevel hashes work? Are hash keys and values correctly URL-encoded? Etc. Thanks for the patch!

05/09/06 15:57:39 changed by me@julik.nl

  • attachment url_rewriter_patch2.diff added.

Patch v2

05/09/06 16:02:18 changed by me@julik.nl

Ok, got the message. Yes, multilevel hashes do work, the depth is ulimited, unless you stuff hashes into arrays. Hash keys and values are URL-encoded (just like in the old rewriter infrastructure). This new one is totally compatible to the old one (the tests just wouldn't pass otherwise).

I also updated the comment. Maybe it should even be read like this:

+ # be added as a path element instead of a regular parameter pair. If parent is passed, it will return a flattened hash of + # parameters, with keys prefixed by the parent. Both keys and values are escaped. It will expand nested hashes of unlimited depth, + # but will not expand the hashes within arrays.

05/11/06 11:47:37 changed by me@julik.nl

  • attachment url_rewriter_v3.diff added.

Final (I hope)

05/11/06 12:00:59 changed by me@julik.nl

Did refactor it to be more concise and squashed an escaping bug along the way.

05/23/06 15:15:06 changed by anonymous

  • priority changed from high to normal.
  • summary changed from [XPATCH] URL rewriter shoud support deeply nested parameters to [PATCH] URL rewriter shoud support deeply nested parameters.

06/11/06 00:15:18 changed by me@julik.nl

  • attachment updated_patch.diff added.

Updated patch agains the new routing code

08/05/06 01:59:30 changed by bitsweat

  • owner changed from David to Ulysses.

(follow-up: ↓ 9 ) 08/23/06 09:18:44 changed by ulysses

  • owner changed from Ulysses to Jamis.

Jamis, do you want to take a look at this?

(in reply to: ↑ 8 ) 12/08/06 18:40:42 changed by tsmith

  • version changed from 1.1.1 to 1.2.0rc1.
  • type changed from enhancement to defect.

I have found a real application of this patch through debugging of my own that I believe moves this from an enhancement to a defect resolution. Let's say that you have:

1. A reason to use a form_for with the GET method for a particular model. 2. On the page after submit, you have a link_to which uses overwrite_params to use the same URL with an extra parameter.

Assume this model:

class Widget < ActiveRecord::Base

belongs_to :group

end

Placed in view like so:

<% form_for :widget, :url => { :action => :action },

:html => { :method => :get } do |form| %>

<%= form.collection_select :group_id, @groups, :id, :name %>

<% end %>

When you submit, the URL becomes:

http://localhost:3000/controller/action?widget%5Bgroup_id%5D=1

But if you have a link_to on this page reached by that URL after the submit that attempts to add a single parameter to the URL:

<%= link_to 'Expand',

{ :overwrite_params => { :expand => true } }

%>

The resulting URL is:

http://localhost:3000/controller/action?widget=group_id1&expand=true

This changes the query string in a way not intended but they override. It occurs because of the lack of sensitivity to the multilevel hash created when this URL is broken down, which is used behind the scenes in the link_to. This patch would fix this.

12/08/06 22:45:44 changed by bitsweat

  • cc set to Ulysses.
  • keywords changed from url_for rewriter routes to url_for rewriter routes 1.2regression.
  • owner changed from Jamis to minam.

12/20/06 18:42:12 changed by bitsweat

  • keywords changed from url_for rewriter routes 1.2regression to url_for rewriter routes.

Not a regression

02/19/07 16:41:26 changed by bigsmoke

  • version changed from 1.2.0rc1 to 1.2.1.

It would greatly help me if this patch was accepted. I'm currently doing some functionality which needs this and, as I'm using externals, I can't just integrate this patch into our source tree without creating our own vendor branch first.

(in reply to: ↑ description ) 03/04/07 11:48:33 changed by lastobelus

I second the need for this. Have encountered it several times. Another example is where one of the things a form submission does is changes a dynamic image on the page. Submitting form reloads the page but the image is drawn by pointing its src to an action, and the params need to be passed through.

03/04/07 12:44:12 changed by bitsweat

  • status changed from new to closed.
  • resolution set to duplicate.

Work has continued in #7047