Deprecate AC::Parameters#== with Hash#23733
Conversation
There was a problem hiding this comment.
Since I cannot access other’s @parameters ivar in a clean way, I had to resort to calling to_h on both objects. Is this sufficient for equality comparison?
|
@bquorning looks like the build is broken. |
bquorning
force-pushed
the
actioncontroller-parameters-equality
branch
from
b9b9f26 to
4d1890a
Compare
|
I split the one commit into three, and hopefully fixed the build too. Time will tell. |
bquorning
force-pushed
the
actioncontroller-parameters-equality
branch
from
4d1890a to
49ee962
Compare
|
cc @sikachu |
There was a problem hiding this comment.
could also make protected attr_reader :parameters so other instances can access.
or just do permitted == other.permitted && @parameters == other.instance_variable_get("@parameters"), more dirty but saves on object allocation
There was a problem hiding this comment.
A protected getter – brilliant idea. I’ve pushed the changes.
bquorning
force-pushed
the
actioncontroller-parameters-equality
branch
from
49ee962 to
97df883
Compare
|
I think this is good. @rafaelfranca @matthewd would you mind merging this in? |
There was a problem hiding this comment.
Do we need to check if the parameter is permitted? I think we only care if the internal parameters are the same.
There was a problem hiding this comment.
I think we do. Old code called super, which means it checks both @parameters and @permitted ivars.
There was a problem hiding this comment.
And why didn't we continue to call super? If I get the comment above right super never checked the ivars.
There was a problem hiding this comment.
Old code called
super, which means it checks both@parametersand@permittedivars.
The #== method and its super call didn’t exist in Rails 4.
There was a problem hiding this comment.
There’s also a @converted_arrays ivar – do we need to check that as well?
There was a problem hiding this comment.
params = ActionController::Parameters.new(a: 1).permit(:a)
assert_equal { a: 1 }, params.to_h
There was a problem hiding this comment.
I need some time to think about this. Please don’t merge just yet.
There was a problem hiding this comment.
After a second thought we should not recommend people to change their tests to assert(params1 == ActionController::Parameters.new(a: 1)). In that case they should use to_h. So yeah, let's keep permitted? there so we assert the object is the same.
There was a problem hiding this comment.
@jacobat You’ve convinced me, too (again). People can use to_h and even to_unsafe_h for comparing with unpermitted instances.
I’ve updated the code.
bquorning
force-pushed
the
actioncontroller-parameters-equality
branch
from
97df883 to
6e2a65b
Compare
There was a problem hiding this comment.
this could also become attr_writer :permitted :D
Creating a protected getter method for `@parameters`.
bquorning
force-pushed
the
actioncontroller-parameters-equality
branch
from
6e2a65b to
3044036
Compare
…equality Deprecate AC::Parameters#== with Hash
A protected getter for @parameters was introduced in rails#23733. Thus, we can stop reading the instance variable directly.
PR #23733 was supposed to deprecate and remove the ability to compare Hash objects with AC::Parameters objects. Unfortunately it seems that we still accidentally support that. This PR adds a deprecation warning so that we can remove it in the future.
9 participants
As @schneems mentioned in #23711: “ActionController::Parameters is not a hash, so we should not expect it to be equal to one.” I think it would be best to deprecate that behavior before Rails 5 is released.
This also fixes #22820 – comparison between AC::Parameters instances.