Complete the implementation of DatabaseSchema::addField() for SQLite [#1004068]

Comment	File	Size	Author
#8	1004068-sqlite-addtable.patch	6.2 KB	damien tournoud
#7	1004068-sqlite-addtable.patch	6.2 KB	damien tournoud
#5	1004068-sqlite-addtable.patch	6.29 KB	damien tournoud
#2	foo.patch	1.66 KB	dmitrig01
#1	foo.patch	1.82 KB	dmitrig01

Comment #1

dmitrig01 commented 21 December 2010 at 23:00

Status:

Active

» Needs review

Status	File	Size
new	foo.patch	1.82 KB

Fixes that issue (pgsql is different). There is still one fail on sqlite for SchemaTestCase, but it fails with or without the patch (oddly, it fails on my computer but nt on ci.drupal.org)

Log in or register to post comments

Comment #2

dmitrig01 commented 22 December 2010 at 05:12

Status	File	Size
new	foo.patch	1.66 KB

This patch doesn't need tests, as the tests already exist, they fail, and this fixes them. There is another fail in the same test case that's being terribly hard to debug as it only fails my local machine and not ci.drupal.org, and it's very obscure. But unrelated to this patch -- I think there's something wrong with the test.

Log in or register to post comments

Comment #3

dmitrig01 commented 22 December 2010 at 05:13

btw, the only difference between #2 and #1 is one line comment change

Log in or register to post comments

Comment #4

dmitrig01 commented 22 December 2010 at 08:47

Title:	Tests attempt to create not-null fields with no default values	» SQLite ignores initial values
Component:	simpletest.module	» sqlite database

Also to note, most of this code was copy+pasted from mysql driver.

Log in or register to post comments

Comment #5

damien tournoud commented 22 December 2010 at 12:24

Title:

SQLite ignores initial values

» Complete the implementation of DatabaseSchema::addField() for SQLite

Status	File	Size
new	1004068-sqlite-addtable.patch	6.29 KB

#2 made the schema test pass on SQLite, but it is not exactly the proper way to do that.

We can add a field directly to a SQLite table using ALTER TABLE if and only if: it is not a PRIMARY KEY, it is not both NOT NULL and without a default value (more precise constraints are described in http://www.sqlite.org/lang_altertable.html). In all the other cases, we have to fallback to the slower method of creating a new table and migrating the data over.

To simplify, I considered that we use the slower method whenever we have to add indexes or we have a NOT NULL column without a default value. I implemented the rest of the DatabaseSchema::addField() and that passes tests on ci.

Log in or register to post comments

Comment #6

dmitrig01 commented 22 December 2010 at 16:57

I read over the code, tried it out and ran the test - looks good. I'd RTBC it but I'm ineligible.

Log in or register to post comments

Comment #7

damien tournoud commented 22 December 2010 at 21:30

Status	File	Size
new	1004068-sqlite-addtable.patch	6.2 KB

A few improvements from chx: we don't need to workaround bugs in addField() and addExpression() anymore, they have been long fixed. Also fixed a comment typo.