Skip to content
This repository was archived by the owner on Aug 17, 2017. It is now read-only.

Params.fetch covers up any KeyError raised in block.#156

Open
d-Pixie wants to merge 1 commit into
rails:masterfrom
d-Pixie:fetch_covers_block_key_error
Open

Params.fetch covers up any KeyError raised in block.#156
d-Pixie wants to merge 1 commit into
rails:masterfrom
d-Pixie:fetch_covers_block_key_error

Conversation

@d-Pixie

@d-Pixie d-Pixie commented Jul 8, 2013

Copy link
Copy Markdown
Contributor

What?

Given:

params = ActionController::Parameters.new()
params.fetch(:missing_key) { some.default_action( :that_raises, KeyError ) }

I would expect to see the KeyError from the block, not a ActionController::ParameterMissing exception for :missing_key.

Why?

To ease in debugging one should not cover relevant exceptions. In this case the error is not that the requested key is missing and therefore the presented error message is a lie :)

Fix

Not implemented yet, I wanted to get some comments before coding this time. I'd guess we'd have to fix the rescue line in action_controller/parameters.rb line 83. Suggestions on how are welcome.

If this is something we want just let me know and I'll fix it.

@thomasfedb

Copy link
Copy Markdown
Contributor

@d-Pixie I think this needs to be fixed. Are you prepared to make the changes of should I?

@d-Pixie

d-Pixie commented Jul 30, 2013

Copy link
Copy Markdown
Contributor Author

I'll write a proposal patch today ...

@rafaelfranca

Copy link
Copy Markdown
Member

Any changes for this code should be submitted first to rails/rails.

@d-Pixie

d-Pixie commented Aug 1, 2013

Copy link
Copy Markdown
Contributor Author

@rafaelfranca - The patch is now available as a pull request in rails/rails. If/when accepted I'll backport it here ...

@d-Pixie

d-Pixie commented Aug 1, 2013

Copy link
Copy Markdown
Contributor Author

@thomasfedb - The code is in, comments are welcome.

@ClayShentrup

Copy link
Copy Markdown

This is still broken.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants