This project is archived and is in readonly mode.
[Patch] Correctly handle instance_eval in association proxies
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.
-
mat- Tag changed from 2-3-stable, association_proxy to 2-3-stable, association_proxy, patch
-
George Ogata (Patch)+1
Surely what was intended by the original author.
-
Michael Koziarski- 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
- 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 [Patch] Correctly handle instance_eval in association proxies state:committed]
Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/2f1ded306728ce8ff614e09a113398... -
Repository
(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 [Patch] Correctly handle instance_eval in association proxies state:committed]
Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/49e943c4f0ac3459bd53023167aaa0... -
Jeremy Kemper- Milestone set to 2.3.6
-
Michael Koziarski
- 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?
-
mat
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
Of course it's possible that nothing else needs changing, in which case awesome!
-
Michael Koziarski
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.
-
-
Jeremy Kemper
Original patch broke continuous integration: http://mri186.ci.rubyonrails.org:3333/builds/rails-2-3-stable-ruby-...
-
Rizwan Reza- Tag changed from 2-3-stable, association_proxy, patch to 2-3-stable, association_proxy, bugmash, patch
-
-
-
-
-
Santiago Pastorino
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
- State changed from open to stale