I feel the inline documentation in settings.php is misguiding. The snip that would need fix is below,

Some database engines support transactions. In order to enable transaction support for a given database, set the 'transactions' key to TRUE. To disable it, set it to FALSE.
Note that the default value varies by driver. For MySQL, the default is FALSE since MyISAM tables do not support transactions.

While transaction is enabled by default, the comment line gives an impression as if the 'transactions' key to be set in $databases in order to enable it. This can be rephrased as below,

...Transaction support is enabled by default. To disable it, set it to FALSE...

This one was actually posted to drupal stackexchange as support question, url is here http://drupal.stackexchange.com/questions/7239/does-my-drupal-7-site-hav...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Tor Arne Thune’s picture

Issue tags: +Documentation

I agree that it's misleading. How would you write the whole paragraph?

Sivaji_Ganesh_Jojodae’s picture

I am not a native English speaker anyway I am giving it a try.

I believe the following copy makes better sense to me,

Some database engines support transactions. Drupal tries to enable it by default. Note that the default value varies by driver. For MySQL with MyISAM engine, the default is FALSE since MyISAM tables do not support transactions. For any other special case, to disable it, set the 'transactions' key to FALSE.

Crell’s picture

Hm. That doesn't read quite right. Subscribing, hopefully I can get back to this at some point...

pounard’s picture

Code in includes/database/mysql/database.inc line 23:

    // This driver defaults to transaction support, except if explicitly passed FALSE.
    $this->transactionSupport = !isset($connection_options['transactions']) || ($connection_options['transactions'] !== FALSE);

But the settings.php says:

Note that the default value varies by driver. For MySQL, the default is FALSE since MyISAM tables do not support transactions.

The settings.php says that the MySQL driver won't support transaction until you explicitely set it to TRUE, which is not what happens since it's enabled as default. I did gave me this erroneous idea and led me to wrong decisions.

The right comment (in settings.php) should be maybe something like this:

 * Transaction support is enabled per default for all drivers that supports it,
 * including MySQL. To explictely disable it, set the 'transaction' key to
 * FALSE.
 * Note that some drivers such as MyISAM MySQL engine don't support it and will
 * proceed silentely even if enabled. If you experience transaction related
 * crashes with such driver, set the 'transaction' key to FALSE.

EDIT: I was just rephrasing the original post and stack exchange question. While this is a really specific detail, its consequencies can be huge: people messing with transactions often mess with critical code, it should be quickly.

Crell’s picture

pounard, can you provide a patch to fix the docblock?

pounard’s picture

Status: Active » Needs review
FileSize
1.17 KB

Did the patch over the 7.x branch. Note that cherry-picking it in the 8.x will be easy.

pounard’s picture

I tested it, and the patch also apply cleanly over the 8.x branch. Seems that the default.settings.php file has not been modified yet on 8.x (except one other line actually referencing garland).

Crell’s picture

Status: Needs review » Needs work
+++ b/sites/default/default.settings.php
@@ -73,11 +73,12 @@
+ * Transaction support is enabled per default for all drivers that supports it,

Grammar quibbles: "enabled by default", not "per". Also "that support it", since we're talking about more than one driver.

+++ b/sites/default/default.settings.php
@@ -73,11 +73,12 @@
+ * Note that some drivers such as MyISAM MySQL engine don't support it and will
+ * proceed silentely even if enabled. If you experience transaction related
+ * crashes with such driver, set the 'transaction' key to FALSE.

Technically speaking, MyISAM isn't a driver but an engine. Calling it a driver will confuse people, I think, since we're already using driver to refer to MySQL vs. SQLite, etc.

I'd suggest instead:

"Note that some configurations of MySQL, such as the MyISAM engine, don't support transactions and will silently ignore transaction rollbacks." (and then the rest of the paragraph, using "configuration" rather than "driver".

pounard’s picture

Status: Needs review » Needs work

Configuration here doesn't fit IMHO but else yes I agree the sentence is not great.
Will fix tomorow, thanks for review.

EDIT: After a second reading your proposal is just right, fixed it in the patch below.

pounard’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
Crell’s picture

+++ b/sites/default/default.settings.php
@@ -73,11 +73,13 @@
+ * Note that some configurations of MySQL, such as the MyISAM engine don't

Need a comma after "engine" to match the one after "MySQL".

+++ b/sites/default/default.settings.php
@@ -73,11 +73,13 @@
+ * support it and will proceed silentely even if enabled. If you experience

"silently" is misspelled.

Otherwise this is RTBC. (/grammar-nazi)

pounard’s picture

Status: Needs work » Needs review
FileSize
1.2 KB

Fixed, still apply on 7.x and 8.x branches cleanly.

If we'd still were in the Middle Age, I'd definitely kill you in a duel. Hopefully for you I just manually edit the patch file, so it doesn't take me long to do and I can still manage my anger.

Crell’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Reviewed & tested by the community

Fortunately I have no glove at hand with which to slap you across the face. :-)

Setting to 8.x first, but should apply to both.

catch’s picture

Status: Reviewed & tested by the community » Needs work
explictely

Should be 'explicitly'.

pounard’s picture

/me slaps catch in the face with white gloves.

Will fix that.

pounard’s picture

Status: Needs work » Needs review
FileSize
1.2 KB

Done.

Damien Tournoud’s picture

+ * Note that some configurations of MySQL, such as the MyISAM engine, don't
+ * support it and will proceed silently even if enabled. If you experience
+ * transaction related crashes with such configuration, set the 'transaction'
+ * key to FALSE.

This is really misleading. Setting 'transaction' to FALSE doesn't do anything useful except skipping the BEGIN TRANSACTION; COMMIT statements, that are going to be ignored anyway when affecting MyISAM tables.

I would remove the whole paragraph. Messing with this setting is not going to be useful to anyone.

pounard’s picture

May be the sentence is not perfect, I would probably agree if you come up with something better, but we definitely never cannot predict the behavior of an old MySQL engine such as MyISAM (in fact we cannot really thrust MySQL at all IMHO, but that's another topic) when using stuff such as BEGIN and COMMIT. I would definitely keep saying that one way or another.

jcisio’s picture

According to the code, it should be "transactions" with an ending "s" if you want transaction to be really disabled. This patch adds two "s" in the patch #20.

I don't have problem with the wording.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

#20 sounds good to me.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x.

Automatically closed -- issue fixed for 2 weeks with no activity.

EvanDonovan’s picture

Version: 8.x-dev » 7.x-dev
Issue summary: View changes
Status: Closed (fixed) » Patch (to be ported)

Comment here by Berdir indicates this needs backported to 7.x: http://drupal.stackexchange.com/questions/104921/drupal-7-mysql-innodb-a...

colan’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.59 KB

Patch attached for D7.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work
Crell’s picture

Status: Needs work » Reviewed & tested by the community

Probably just testbot goofing off last night...

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

  • David_Rothstein committed 433fe74 on 7.x
    Issue #1221772 by pounard, colan, jcisio | sivaji: Fixed Transaction...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.