Skip to content
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.

Sign up for GitHub

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

Jump to bottom

disable_ddl_transaction! is a misleading name #21044

Closed
PhilCoggins opened this issue Jul 27, 2015 · 4 comments
Closed

disable_ddl_transaction! is a misleading name #21044

PhilCoggins opened this issue Jul 27, 2015 · 4 comments
Labels
activerecord docs

Comments

@PhilCoggins
Copy link
Contributor

disable_ddl_transaction! is used in AR migrations to prevent your migration from being wrapped in a transaction. The name is misleading because your entire migration will have transactions disabled, not just for DDL SQL statements.

If I have a migration that contains a mixture of DDL and DML SQL, and declare disable_ddl_transaction! at the top of my migration, I would still expect DML to be wrapped in a transaction, but this is not the case, as the DML SQL statements are still executed outside of a transaction.

If the desired behavior is to disable only DDL transactions, then I would consider this a bug, but I believe the intent of this method is to prevent your entire migration from running in a transaction, and thus the method is poorly named. Perhaps disable_transaction! would be more appropriate?
@mgodwin
Copy link
Contributor

mgodwin commented Aug 4, 2015

👍 I agree.

I also find it misleading because you're not disabling all transactions for the duration of the entire migration, just that the migration as a whole is wrapped in a transaction. (You can still start up transactions/ddl transactions on your own). The rails guides and the rdoc don't really mention that fact anywhere and more generally say:

Disable DDL transactions for this migration.

Which would imply to me that you can't run any DDL transactions at all, which isn't true 😕

I think clarifying the docs could go a long way or renaming the method could help. Some suggestions I have could be: without_transaction! or manual_transactions! - I think the latter is more clear. I'm not sure that the ddl part is necessary since you won't get transaction support if the adapter doesn't support ddl transactions anyways, right?

@senny
Copy link
Member

senny commented Aug 10, 2015

I agree that the name could be better. The ddl_ part was chosen to make it clear that this only affects adapters supporting ddl transactions. For example to a MySQL adapter user without_transaction! is not clear as there is no wrapping transaction.

👍 on making the docs more clear. I'm not sure we have to change the name though.

/cc @rafaelfranca @carlosantoniodasilva

@carlosantoniodasilva
Copy link
Member

I think the documentation on the [ActiveRecord::Migration module that talks about Transactional Migrations]http://api.rubyonrails.org/classes/ActiveRecord/Migration.html) explains better the intention of the method, including the "transaction wrapping". With this in mind, I'm leaning towards improving the method documentation a bit as well, rather than a method name change 👍

@rafaelfranca
Copy link
Member

Same here 👍 for doc change.

senny added a commit that referenced this issue Aug 21, 2015
@senny
better docs for disable_ddl_transaction!. Closes #21044.
4d2b6db
@senny senny closed this as completed in 3088460 Aug 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
activerecord docs
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@senny @carlosantoniodasilva @rafaelfranca @mgodwin @PhilCoggins