INPUT: "a[][x]=5&a[][y]=10"

OLD PARSER OUTPUT: {"a" => [{"x" => "5", "y" => "10"}]}
NEW PARSER OUTPUT: {"a" => [{"x" => "5"}, {"y" => "10"}]}

I'm not sure I agree with this new parser output. With this behavior, how is one supposed to pass an array of hashes to the web application, where each hash has more than 1 element? Would one be forced to write "a[0][x]=5&a[0][y]=10"?

That's correct. And the extra characters really won't kill you. Hell, you probably won't even notice them with the right helpers in place. Call me crazy, but IMO correctness and clarity trump query string length. :)

Explicit indices actually make a lot of sense in practice for this particular use-case. Take this example:

a[][b][][x]=5&a[][b][][y]=10&a[][b][][y]=15&a[][b][][x]=20

Now tell me: at a glance, what does this do?

With explicit indices, it becomes slightly more verbose but it also eliminates the ambiguity:

a[0][b][0][x]=5&a[0][b][0][y]=10&a[0][b][1][y]=15&a[0][b][1][x]=20

I see. Your format does make more sense for complex forms. I'm a bit worried about backwards compatibility but I guess the people at the mailing list have already told you that. :)

A backwards compatibility mode, perhaps?

I can probably include backport the "[]" syntax so that it behaves like the old parser, although I'm not sure how difficult that would be off the top of my head. In all honesty I'd prefer to avoid it, but perhaps the backwards compatibility argument is compelling enough to warrant this.

The other option is switching between the old and the new parser using a configuration parameter in the environment.rb Rails::Initializer config block. config.parser or something like that?

I'd probably prefer the latter to ensure maximum backwards compatibility for those who need/want it, but I'm open to suggestion.

The latter sounds good to me.

Here's a patch implementing pluggable UrlEncodedPairParsers. You can switch between parsers using the config.action_controller.pair_parser configuration setting in environment.rb. e.g.

config.action_controller.pair_parser = ActionController::UrlEncodedPairParser

This revision probably needs more tests. Most notably the individual quirks of the old and new parsers need to be tested independently.

We actually already have pluggable parameter parsing. Is there a reason that you couldn't use that?

ActionController::Base.param_parsers[Mime::XML] = Proc.new { |data| XmlSimple.xml_in(data, 'ForceArray' => false) }

Ah, I wasn't aware of that one.

I can't directly use it myself, but if I actually remove my changes to add the pair_parser option, this would provide a suitable way for people to revert back to the legacy parser if they needed to without any additional effort on my part.

ActionController::Base.param_parsers[Mime::URL_ENCODED_FORM] = Proc.new { |data| ... }

ActionController::Base.param_parsers[Mime::MULTIPART_ENCODED_FORM] = Proc.new { |data| ... }

For this to be truly useful, the code implementing the :url_encoded_form and :multipart_encoded_form strategies should be extracted out so as to be more easily reused by people wishing to do just that, but I think that might be a follow-up patch. Any strong feelings about that?

I'll upload an updated patch later tonight.

How about you make whatever changes you need to make your new parser work as a drop in plugin option.

That way we can push that change into 2.2 and encourage people to experiement with the new parser via plugins and edge changes.

Sure thing. Before I submit another patch, should the old parser still be the default?

Yes, I think that changing the parser is a risky prospect and I'd want it to be fully bedded-in over a 2.3 release cycle before we made the switch.

So I see two patches:

Make it easy to change the parser so the new one can be a plugin without much hacking

Integrate the new parser

The first one can go in now, and the second change is something that can happen alongside the work on replacing :accessible=>true to make sure we have a really well integrated way of dealing with those more complicated cases.

Sounds good. I'll shuffle things around a bit and submit the changes either tonight or tomorrow. Thanks for the support.

Alright, here's patch #1 Migrations will not run on a fresh database which restructures the guts of AbstractRequest to make it a little easier to reuse the code in there for custom UrlEncodedPairParsers. It might seem like overkill to provide a parse#{foo}with_parser method for all those methods, but it will come in handy when it's time to test new parser implementations (most of the tests use parse_query_parameters and parse_request_parameters

Also, the strategy.arity == 4 check feels a little bit dirty, but I couldn't think of a nicer way to pass through this information to Procs that want to override the parser for Mime::MULTIPART_FORM short of forcing them to parse that info for themselves. Which IMHO would suck.

Pending acceptance of this patch, I'll put together a patch to integrate a new parameter parser as you suggest. Should I get in touch with anybody else on the mailing list regarding these patches? The folks responsible for the :accessible => true stuff maybe?

In case it isn't immediately obvious from the patch, this will allow folks to override the default parsers for URL and MULTIPART encoded form data as follows:

ActionController::Base.param_parsers[Mime::URL_ENCODED_FORM] = Proc.new do |data|
  ar = ActionController::AbstractRequest
  ar.parse_url_encoded_form_parameters_with_parser(MyCustomParser, data)
end

ActionController::Base.param_parsers[Mime::MULTIPART_FORM] = Proc.new do |data, boundary, content_length, env|
  ar = ActionController::AbstractRequest
  ar.parse_multipart_encoded_form_parameters_with_parser(
    MyCustomParser, data, boundary, content_length, env)
end

Any feedback on this patch Michael?

I think we should just hold off on the changes till post 2.2, it doesn't seem this will make it too much easier for user's to test, and if it turns out I'm wrong we can include the hooks in a point release.

are you guys aware of http://dev.rubyonrails.org/ticke...?

this is a parameter parser currently in use in an app I work on for Sage Software. The last patch I uploaded may have a bug...not sure if I uploaded the fix.

I've simply never been given time to maintain this patch against rails edge.

we use it to manage deeply nested forms.

The existing parser semantics can't work for more than 1 level of nesting (think checkboxes).

problem is, prototype also needs to be patched to preserve the param order when marshalling a form (if it hasn't been in the last eight months)

btw, how does your parser do with nesting + checkboxes?

@Michael Johnston: does just fine with nesting and checkboxes, but ActionView::Helpers::FormHelper::InstanceTag#to_check_box_tag needs to be modified so that the hidden field is returned after the actual checkbox due to some assumptions the old code makes about the parser behavior (the parameter order thing, which it sounds like you encountered).

Unfortunately this is not yet in the latest patch -- I have an updated patch sitting in my local repo, but it needs a few unit tests.

FWIW, the patch(es) provided to date are no longer relevant with the move to Rack in 2.3 and beyond.

I'm not sure if anybody's still interested in the semantics or features provided by this patch, but if there's sufficient interest I can rework it for the newer versions of Rails. I'm still not sure I agree that the implemented semantics are ideal (using hashes for numerical indexes is not sane IMO), but at a glance it looks better than the old UrlEncodedPairParser stuff. Having said that, my concerns will probably mostly go away with Ruby 1.9 anyway.

Anyway, feel free to close this one off if there's no further interest in this stuff.