DatabaseSchema_sqlite severely broken [#851698]

Bad news: DatabaseSchema_sqlite is severely broken. Good news: with the upgrade tests, we know have some intense Schema tests :)

Are broken:

dropField() exits with FALSE when the field exists (oups!)
changeField() and alterTable() doesn't map the old field name to the new field name, so the data migration query fails (re-oups!)
changeField() doesn't change the name of a renamed field in the index / unique key definitions (re-re-oups!)
dropField() doesn't remove the name of the dropped field, so the data migration query fails (re-re-re-oups!)

Critical because the Upgrade path test fails on SQLite (because the Schema is broken).

Comment	File	Size	Author
#11	851698-sqlite-schema-broken.patch	13.73 KB	Damien Tournoud
#11
#8	851698-sqlite-schema-broken.patch	12.52 KB	Damien Tournoud
#8
#6	851698-sqlite-schema-broken.patch	9.14 KB	Damien Tournoud
#6
#1	851698-sqlite-schema-broken.patch	8.16 KB	Damien Tournoud
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Damien Tournoud CreditAttribution: Damien Tournoud commented 12 July 2010 at 14:44

Status:

Active

» Needs review

File	Size
851698-sqlite-schema-broken.patch	8.16 KB

Comment #2

Damien Tournoud CreditAttribution: Damien Tournoud commented 12 July 2010 at 14:47

Also, we are creating / destroying the indexes twice during a field rename operation: once for the data migration, once for the table rename. We should avoid creating the indexes when creating the temporary table. This affects performance but doesn't affect functionality.

Comment #3

12 July 2010 at 15:00

Status:

Needs review

» Needs work

The last submitted patch, 851698-sqlite-schema-broken.patch, failed testing.

Comment #4

Damien Tournoud CreditAttribution: Damien Tournoud commented 12 July 2010 at 15:04

Status:

Needs work

» Needs review

This one relies on the other DatabaseSchema_sqlite patches I posted earlier. It doesn't apply on top of a vanilla tree, but it can still be reviewed :)

MySQL is not affected anyway.

Comment #5

Damien Tournoud CreditAttribution: Damien Tournoud commented 12 July 2010 at 15:52

Some additional issues:

There doesn't seem to be a need for renaming the table twice in alterTable()
dropIndex() and dropUniqueKey() were broken: they deleted the index without proper schema prefixing

Comment #6

Damien Tournoud CreditAttribution: Damien Tournoud commented 12 July 2010 at 15:52

File	Size
851698-sqlite-schema-broken.patch	9.14 KB

Comment #7

12 July 2010 at 16:10

Status:

Needs review

» Needs work

The last submitted patch, 851698-sqlite-schema-broken.patch, failed testing.

Comment #8

Damien Tournoud CreditAttribution: Damien Tournoud commented 12 July 2010 at 17:05

Status:

Needs work

» Needs review

File	Size
851698-sqlite-schema-broken.patch	12.52 KB

Merged in the patch from #851166: SQLite schema implementation doesn't support qualified tables.

Comment #9

Stevel CreditAttribution: Stevel commented 12 July 2010 at 19:13

Still works on mysql according to testbot, and this patch makes the basic upgrade test work on sqlite, which is definitely a huge plus. One small thing though:

+++ includes/database/sqlite/schema.inc
@@ -20,8 +20,10 @@ class DatabaseSchema_sqlite extends DatabaseSchema {
+    return (bool) $this->connection->query("SELECT 1 FROM {$info['schema']}.sqlite_master WHERE type = :type AND name = :name", array(':type' => 'table', ':name' => $info['table']))->fetchField();

Using the brackets around {$info['schema']} is a bit confusing here: It's not clear whether they show up in the resulting string or not?
I think it would be better for clarity to use concatenation here to include the variable

51 critical left. Go review some!