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

Fix for ActiveRecord 4.2 and Arel changes #354

Merged
merged 5 commits into from Feb 5, 2015

Conversation

Projects
None yet
8 participants
@danielrhodes
Contributor

danielrhodes commented Dec 20, 2014

The ActiveRecord fix is pretty straightforward.

In the new Arel version (commit where it breaks is mentioned), the BindParam class now subclasses Node instead of SqlLiteral which means that none of the String methods will work. It's unclear if the needed reordering for Postgres (mentioned in the comment) is now handled by Arel, or if that replacement needs to occur elsewhere.

danielrhodes added some commits Dec 20, 2014

Update relation_extensions.rb
Change for API update on ActiveRecord 4.2 (commit 08579e4078454c6058f1289b58bf5bfa26661376)
@Kriechi

This comment has been minimized.

Kriechi commented Dec 23, 2014

👍 works for me!

@nathansamson

This comment has been minimized.

nathansamson commented Jan 5, 2015

Note that this fix breaks ActiveRecord 4.1 compatibiltiy. (I upgraded to rails 4.2, needed this fix, then downgraded again because of some other problems), had to revert this version back to stock squeel 1.2.2

Backwards compatibility fix for Active Record 4.1
Checks the size of the changed method to determine which arguments to
provide.
@danielrhodes

This comment has been minimized.

Contributor

danielrhodes commented Jan 8, 2015

@nathansamson I added a fix for this. Let me know if it works for you (although it seems like you took the right steps to resolve your problem).

@nathansamson

This comment has been minimized.

nathansamson commented Jan 8, 2015

Their are extra test cases failing on Travis CI for Rails 4.1 & PostgreSQL adapter (exactly my use case), so it still feels you introduced a problem...

@yule

This comment has been minimized.

yule commented Jan 14, 2015

Perhaps it would be better if #d665b08 would be be better applied to active_record/4.2/relation_extensions.rb rather than conditionally in 4.1?

@danielrhodes

This comment has been minimized.

Contributor

danielrhodes commented Jan 14, 2015

@yule I wondered the same thing. I'm not very familiar with the code base so I'm not exactly sure why active_record/4.1/relation_extensions.rb was being used instead of active_record/4.2/relation_extensions.rb on Rails 4.2. Perhaps somebody else could enlighten me? @nathansamson I'll look into that.

@bigxiang

This comment has been minimized.

Contributor

bigxiang commented Jan 15, 2015

@danielrhodes Thanks for the fix, could you please create a new branch (not master) and push it again? So I could fix some minor spec errors after merging your branch.

@arpitadhundia

This comment has been minimized.

arpitadhundia commented Feb 2, 2015

Any update on when a new version of Squeel will be released with this fix?

@marnen

This comment has been minimized.

marnen commented Feb 4, 2015

Yes, a release would be great.

@bigxiang bigxiang merged commit e7f0168 into activerecord-hackery:master Feb 5, 2015

1 check failed

continuous-integration/travis-ci The Travis CI build could not complete due to an error
Details
@bigxiang

This comment has been minimized.

Contributor

bigxiang commented Feb 5, 2015

Thank @danielrhodes . The new squeel 1.2.3 has been released. Please enjoy it :)

@marnen

This comment has been minimized.

marnen commented Feb 5, 2015

Thank you!

@nadnoslen

This comment has been minimized.

nadnoslen commented Feb 5, 2015

Thanks guys ;)

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