/cc @benarent @shime
After looking at the diff from my previous pull request with fresh eyes today, I noticed a subtle problem: I was unintentionally using params rather than the hash argument.
Technically, the params vs hash difference won't make a difference in most cases, but it should be consistent with the rest of the method. I've tested this on a staging instance and it behaved as expected.
Thanks for merging the other pull request so quickly; my apologies for not noticing this problem earlier and including it in the previous request.
`params` is the (eventual) intent, but `hash` is clearer in context
yes, ben. this code is relevant to rails controller methods, so I don't think it makes difference if you use params instead of hash but it's cleaner in the method context.
is this variable assignment really necessary? can you please just use a one liner like ActionDispatch::Http::ParameterFilter.new(::Rails.application.config.filter_parameters).filter hash.
This is also not safe from exceptions, so please add the rescue statement. I would prefer if you added it after the end statement so it's more DRY. your thoughts on this?
thank you for contributing once again
Both sound like fine ideas. I'll push some changes, likely this morning.
One-liner, per @shime
Exception safety: worst case scenario, report the unfiltered hash
(m) Summarize the cases for future maintainers
I've made the changes you requested. Sorry for the delay; this has been an abnormal week.
I've tested this using a Rails 3.1.2 app, the same way I tested my previous patch. Although I wouldn't expect the rescue refactoring to cause an issue, please note that I don't have a Rails 2.x app handy for testing.
np, thank you