Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unused method_missing definition #15694

Merged
merged 1 commit into from
Jun 13, 2014

Conversation

sgrif
Copy link
Contributor

@sgrif sgrif commented Jun 13, 2014

We always define attribute methods in the constructor or in init_with.

# see if we've created the method we're looking for.
def method_missing(method, *args, &block) # :nodoc:
self.class.define_attribute_methods
if respond_to_without_attributes?(method)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this method still required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's used within ActiveModel

@matthewd
Copy link
Member

Should respond_to? change correspondingly?

@sgrif
Copy link
Contributor Author

sgrif commented Jun 13, 2014

Updated

We always define attribute methods in the constructor or in `init_with`.
@egilburg
Copy link
Contributor

❤️ not using method_missing if we can avoid to, but I wonder if original implementation used it for lazy loading reasons and if in some apps eagerly defining them may break.

@sgrif
Copy link
Contributor Author

sgrif commented Jun 13, 2014

It's eager on calling new, which would have done it eagerly in 99% of cases, anyway.

@egilburg
Copy link
Contributor

Right. But e.g. someone may have included a of module defining a virtual attribute into their class before instantiating it.

If doing so was never part of public API, I guess it's up to user to deal with. But it may be nice to have release notes warning upgrading users of this change.

@sgrif
Copy link
Contributor Author

sgrif commented Jun 13, 2014

someone may have included a of module defining a virtual attribute into their class before instantiating it.

That will still work. We have tests doing exactly that.

rafaelfranca added a commit that referenced this pull request Jun 13, 2014
Remove unused `method_missing` definition
@rafaelfranca rafaelfranca merged commit 2324495 into rails:master Jun 13, 2014
@sgrif sgrif deleted the sg-method-missing branch June 14, 2014 01:17
chancancode added a commit to chancancode/rails that referenced this pull request Jun 14, 2014
84cf156 (PR rails#15694) introduced a subtle regression. There are actually three
distinct entry points to creating an AR object – via .new (i.e. #initialize),
via #init_with (e.g. from YAML or database queries) and via .allocate.

With the patch in 84cf156, attribute methods and respond_to? will not work
correctly when objects are allocated directly, without going through either

The reason this test case didn't catch the regression was that the `Topic`
class is shared between test cases, so by the time this test case is ran the
attribute methods are very likely to be defined.

Switching to use a fresh anonymous class in the test to ensure we surface this
problem in the future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants