-
Notifications
You must be signed in to change notification settings - Fork 617
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
Gemfile order with composite_primary_keys gem #396
Comments
This wasn't something I expected, but looking through the composite_primary_keys source code it looks like it monkey patches the ActiveRecord::Base.establish_connection method as well (at least version 8.x does). My guess is that when it's loaded after activerecord-import it loses our method definition. It looks like starting with composite_primary_keys 9.0.5 they changed the way they handle database connections, so it might not be an issue for projects using ActiveRecord 5. It would be nice if this is something that could be fixed in the composite_primary_keys project, but I would happily accept a PR to document this behavior. Thanks! |
Hey @codeodor, I see you are a composite_primary_keys contributor. Could you confirm (or deny) my suspicions here? 😄 |
I don't recall the specifics on the establish connection, but yes there is definitely a lot of monkeypatching going on, and it would not surprise me if that is the issue here. I used AR-import in the same project as CPK too, and I recall either having to drop it or rearrange the gemfile at some point. I think a better way would be to alias the methods to keep the chain, and we try to do that in places, but in some places it's not feasible because the change CPK needs to make is in the middle of the method, as opposed to around it. I'm not sure if that's the case in version 8, but if we moved it in 9, that's good news! I do try to review when we upgrade for AR to see if there's any we can remove. I haven't looked in to 5.1 yet, so I couldn't say whether what you noticed in 9 will remain the case for whatever version comes next to support AR 5.1. |
Thanks @codeodor! Ok sounds like some documentation for using the two gems together would be a good idea in this project's readme. |
@dukedave I realize this is pretty old and I'm not sure if this is still a problem for you anymore. If it is, would you be able to provide a sample project that demonstrates the error? |
@oniofchaos as far as I can remember it wasn't a problem so long as we kept the order flipped around in the I'm afraid I no longer work on the project which used the gem. |
That's fair. Do you think it makes sense to close the issue to keep the issue list clean? |
Deal 👍 |
This
Gemfile
snippet:Causes:
However, if I flip the
gem
lines around:Then
import
works correctly.I don't know if this is expected behavior, but it'd be nice to put in the readme if it is.
This appears unrelated to #111.
The text was updated successfully, but these errors were encountered: