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

Already on GitHub? Sign in to your account

Reindex bind params for sub-queries #13136

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
Contributor

pftg commented Dec 2, 2013

On building where values in where! with relation as hash constraint
added reindexing of bind params positions to be linked with corresponded
bind values

Fixes #12753

Contributor

pftg commented Dec 6, 2013

Owner

rafaelfranca commented Dec 6, 2013

ack. I'll take a look later

@ghost ghost assigned rafaelfranca Dec 6, 2013

@rafaelfranca rafaelfranca modified the milestones: 4.0.5, 4.0.4 Mar 10, 2014

Contributor

fabiokr commented Mar 12, 2014

This fixed it for me 👍

Contributor

fabiokr commented Mar 12, 2014

@pftg One thing though, it seems this issue milestone is 4.0.5, but it was based on the latest code.

Contributor

pftg commented Mar 13, 2014

@fabiokr as soon it will be merged in master, it will be back-ported by core team.

@rafaelfranca rafaelfranca modified the milestones: 4.0.6, 4.0.5 Mar 17, 2014

@pftg pftg Reindex bind params for sub-queries
On building where values in `where!` with relation as hash constraint
added reindexing of bind params positions to be linked with corresponded
bind values

Fixes #12753
1d02c17
Contributor

pftg commented May 22, 2014

@tenderlove I found that master has been updated with similar logic like this branch (taking in account of old binds), I rebased it, and now investigating your changes.

This PR fix reindexing of binds at least for PG, and current master version still has problem.

Please take a look, I moved assigning of bind values from build_where, I think build_where should only calculate values, but saving should be in other's places.

Also there are others places with bind value index bug, I'd like to fix them, after we will finish with this PR.

Thanks in advance

Contributor

brocktimus commented Aug 20, 2014

/cc @rafaelfranca any chance of this making it into 4.1 or 4.2?

Owner

eileencodes commented Aug 20, 2014

I'm not sure if anything else needs to be done but as it stands currently tests aren't passing and it needs a rebase before it can be merged.

Contributor

brocktimus commented Aug 20, 2014

Damn, I had a quick try to rebase myself but I can't tell whats going on in the areas that conflict. I have confirmed that the bug #12753 still exists on master for what its worth.

Contributor

pftg commented Aug 20, 2014

@brocktimus there is much to do, because fix was added before new active record comes. Will try to do it on those weekends

@rafaelfranca rafaelfranca modified the milestones: 4.0.10, 4.1.7 Aug 21, 2014

Contributor

brocktimus commented Sep 4, 2014

I think this is fixed by 2e6625f 👍 , although I dont think it'll be back portable becuase the binding code changed so much with the introduction of Adequate Record.

Contributor

brocktimus commented Sep 4, 2014

I spoke too soon and my retest was bad. Added more details to initial issue #12753.

@rafaelfranca rafaelfranca modified the milestones: 4.1.9, 4.0.13 Nov 19, 2014

@rafaelfranca rafaelfranca modified the milestones: 4.0.13, 4.1.9 Jan 2, 2015

Member

sgrif commented Feb 9, 2015

This has been handled elsewhere in the code.

@sgrif sgrif closed this Feb 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment