search

Faveod / arel-extensions

Extending Arel
MIT License
142 stars 24 forks source link

Rails 7 testing and fixes a double quotation bug #54

Closed atruskie closed 2 years ago

atruskie commented 2 years ago

Hi!

I found a bug when I upgraded to Rails 7. I found it pretty hard to try and work out what was going on so I just fixed it.

I also had to add support files for testing Rails 7. Sorry for the multi-concerned PR but it didn't really make sense including the fix without the support files needed to verify the bug.

You can cherry pick either commit if it's an issue.

I'm not sure my fix is the best fix. Another option I thought of was stripping quotes inside the Arel::Nodes::As class so the current visitor could continue to always quote the column. However, I couldn't work out how to do it easily.

Someone else encountered this issue: https://github.com/rails/rails/issues/44099 Rails introduced the problem in https://github.com/rails/rails/commit/7e6e9091e55c3357b0162d44b6ab955ed0c718d5 by quoting an alias name. Related to: https://github.com/Faveod/arel-extensions/issues/38#issuecomment-750975055

akimd commented 2 years ago

Hi there, Thanks a lot for this contribution! We will soon integrate it. However our CI was a bit lame, and we spent some time to improve it. Which created conflicts with your changes. Could you please rebase your PR on top of the recent CI changes? Thanks in advance.

atruskie commented 2 years ago

Done, though I just noticed that @stackmystack beat me to it! Sorry for the late response!