#3412 [Patch] Correctly handle instance_eval in association proxies - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#3412 ✓stale

[Patch] Correctly handle instance_eval in association proxies

Reported by mat | October 22nd, 2009 @ 03:28 PM | in 2.3.10

In the current stable 2.3, ActiveRecord::Associations::AssociationProxy#method_missing calls yield() if a block is given, causing the block to always be evaluated in its calling context. However, in the case of instance_eval, correct behavior requires that the block be passed directly to the @target, rather than being evaluated inside a different block. Incidentally, this also simplifies the code slightly.

This patch is against the 2-3-stable branch.

instance-eval-in-association-proxy.patch 2.7 KB

Comments and changes to this ticket

You flagged this item as spam.
mat October 22nd, 2009 @ 03:51 PM
- Tag changed from “2-3-stable, association_proxy” to “2-3-stable, association_proxy, patch”
You flagged this item as spam.
George Ogata (Patch) December 2nd, 2009 @ 05:30 PM
+1

Surely what was intended by the original author.
Michael Koziarski December 2nd, 2009 @ 08:36 PM
- State changed from “new” to “wontfix”
These changes were added for performance reasons, If you're interested in the changeset and memory savings git blame will show them to you and alexander dymo's blog has a good example of the real-life difference it made.

At present the misbehaving instance_eval is a trade-off for savings in memory and GC churn. If you think that trade-off isn't worth it, then that's the discussion to have on the mailing list.
Repository December 2nd, 2009 @ 08:48 PM
- State changed from “wontfix” to “committed”
(from [2f1ded306728ce8ff614e09a113398328e7fe6ca]) Fix instance_eval calls to association proxies

In the current stable, ActiveRecord::Associations::AssociationProxy#method_missing calls yield() if a block is given, causing the block to always be evaluated in its calling context. However, in the case of instance_eval, correct behavior requires that the block be passed directly to the @target, rather than being evaluated inside a different block. Incidentally, this also simplifies the code slightly.

[#3412 state:committed]

Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/2f1ded306728ce8ff614e09a113398...
Repository December 2nd, 2009 @ 08:48 PM
(from [49e943c4f0ac3459bd53023167aaa08fc8e46733]) Fix instance_eval calls to association proxies

In the current stable, ActiveRecord::Associations::AssociationProxy#method_missing calls yield() if a block is given, causing the block to always be evaluated in its calling context. However, in the case of instance_eval, correct behavior requires that the block be passed directly to the @target, rather than being evaluated inside a different block. Incidentally, this also simplifies the code slightly.

[#3412 state:committed]

Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/49e943c4f0ac3459bd53023167aaa0...
Jeremy Kemper December 2nd, 2009 @ 08:49 PM
- Milestone set to “2.3.6”
Michael Koziarski December 2nd, 2009 @ 08:56 PM
- State changed from “committed” to “open”
Given we've decided to apply this anyhow, I feel it's important we investigate every other case where this patch introduced changes. The original is here:

http://github.com/rails/rails/commit/13a60b83a470fb90f91a073b0540f7...

Odds are each one of those causes the problem you've reported and we should fix them all! Could you take a stab at a patch for this?
You flagged this item as spam.
mat December 2nd, 2009 @ 08:56 PM
Checked out the patch - given the performance implications, it seems the logical step would be to explicitly implement #instance_eval, which uses named blocks, and keep the optimized #method_missing implementation. Shall I submit another patch?
Michael Koziarski December 2nd, 2009 @ 08:57 PM
Of course it's possible that nothing else needs changing, in which case awesome!
Michael Koziarski December 2nd, 2009 @ 08:58 PM
heh, lighthouse doesn't scale for parallel discussions!

Yea, see if you can rejig the patch to just fix instance_eval and leave the others in place.
You flagged this item as spam.
mat December 2nd, 2009 @ 10:01 PM
Here 'tis.
- explicitly-define-associationproxy-instance-eval.patch 1.8 KB
Jeremy Kemper December 7th, 2009 @ 01:16 AM
Original patch broke continuous integration: http://mri186.ci.rubyonrails.org:3333/builds/rails-2-3-stable-ruby-...
Rizwan Reza May 16th, 2010 @ 02:41 AM
- Tag changed from “2-3-stable, association_proxy, patch” to “2-3-stable, association_proxy, bugmash, patch”
[bulk edit]
Jeremy Kemper May 23rd, 2010 @ 05:54 PM
- Milestone changed from “2.3.6” to “2.3.7”
[bulk edit]
Jeremy Kemper May 24th, 2010 @ 09:40 AM
- Milestone changed from “2.3.7” to “2.3.8”
[bulk edit]
Jeremy Kemper May 25th, 2010 @ 11:45 PM
- Milestone changed from “2.3.8” to “2.3.9”
[bulk edit]
Jeremy Kemper August 30th, 2010 @ 02:28 AM
- Milestone changed from “2.3.9” to “2.3.10”
- Importance changed from “” to “”
[bulk edit]
Santiago Pastorino February 2nd, 2011 @ 04:50 PM
This issue has been automatically marked as stale because it has not been commented on for at least three months.

The resources of the Rails core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.

Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.
Santiago Pastorino February 2nd, 2011 @ 04:50 PM
- State changed from “open” to “stale”