Skip to content

Erroneous statement in ActionDispatch::ParamsParser #3728

Closed
whitequark opened this Issue Nov 23, 2011 · 6 comments

3 participants

@whitequark

The code at this line is never reached. Is this a bug? Should the hash be returned sometimes?

@kennyj
kennyj commented Nov 23, 2011

I've been chasing history.

I think that the below commit is the origin.

d2ed32d#actionpack/lib/action_controller/request.rb

I think that it is a mistake :-)

/cc @jeremy
do you remember it ?

@kennyj
kennyj commented Nov 23, 2011

@whitequark

I think that this is a bug. And we have two fix way.

1: Return hash. It seems that original source's behavior return hash.

d2ed32d#L7L311

2: Raise an error. Now behavior raise an error.

https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/params_parser.rb#L57

I think that now behavior is proper (therefore we should remove only the hash).
What do you think ?

@whitequark

@kennyj, I believe that the current behavior is correct, too: the hash itself does not makes much sense for me, and the parsing error should clearly not just be forwarded to action body, as the latter does not expects it and this behavior is not documented anywhere I know.

@kennyj
kennyj commented Nov 23, 2011

@whitequark

The next commit is my opinion.
55ed8d00def569e9ba44e4cadc369c7abfc630da

1: remove unreachable code.
2: re-raise original exception.

@whitequark

Just FYI: raise without arguments is just the same as raise e (in this context) or raise $! (generally). It's fine this way, through.

@kennyj
kennyj commented Nov 23, 2011

@whitequark, I didn't know it. Thanks!

I'll change commit message, and send pull request.

p.s.
When chasing history, I feel like a historian.

@josevalim josevalim closed this in f62f545 Nov 23, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.