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...
Comments
Comment #1
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedI agree that it's misleading. How would you write the whole paragraph?
Comment #2
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedI am not a native English speaker anyway I am giving it a try.
I believe the following copy makes better sense to me,
Comment #3
Crell CreditAttribution: Crell commentedHm. That doesn't read quite right. Subscribing, hopefully I can get back to this at some point...
Comment #4
pounardCode in
includes/database/mysql/database.inc
line 23:But the settings.php says:
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:
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.
Comment #5
Crell CreditAttribution: Crell commentedpounard, can you provide a patch to fix the docblock?
Comment #6
pounardDid the patch over the 7.x branch. Note that cherry-picking it in the 8.x will be easy.
Comment #7
pounardI 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).
Comment #8
Crell CreditAttribution: Crell commentedGrammar quibbles: "enabled by default", not "per". Also "that support it", since we're talking about more than one driver.
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".
Comment #9
pounardConfiguration here doesn't fit IMHO but elseyes 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.
Comment #10
pounardComment #11
Crell CreditAttribution: Crell commentedNeed a comma after "engine" to match the one after "MySQL".
"silently" is misspelled.
Otherwise this is RTBC. (/grammar-nazi)
Comment #12
pounardFixed, 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.
Comment #13
Crell CreditAttribution: Crell commentedFortunately I have no glove at hand with which to slap you across the face. :-)
Setting to 8.x first, but should apply to both.
Comment #15
catchShould be 'explicitly'.
Comment #16
pounard/me slaps catch in the face with white gloves.
Will fix that.
Comment #17
pounardDone.
Comment #18
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis is really misleading. Setting
'transaction'
to FALSE doesn't do anything useful except skipping theBEGIN 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.
Comment #19
pounardMay 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.
Comment #20
jcisio CreditAttribution: jcisio commentedAccording 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.
Comment #21
Crell CreditAttribution: Crell commented#20 sounds good to me.
Comment #22
Dries CreditAttribution: Dries commentedCommitted to 8.x.
Comment #24
EvanDonovan CreditAttribution: EvanDonovan commentedComment here by Berdir indicates this needs backported to 7.x: http://drupal.stackexchange.com/questions/104921/drupal-7-mysql-innodb-a...
Comment #25
colanPatch attached for D7.
Comment #26
Crell CreditAttribution: Crell commentedComment #28
Crell CreditAttribution: Crell commentedProbably just testbot goofing off last night...
Comment #30
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks!