Add handling for adding/modifying columns with FIRST/AFTER

[JanJakes]

Add handling for adding/modifying columns with FIRST/AFTER, such as:

ALTER TABLE _tmp_table ADD COLUMN new_first_column VARCHAR(255) NOT NULL DEFAULT '' FIRST;
ALTER TABLE _tmp_table ADD COLUMN new_column VARCHAR(255) NOT NULL DEFAULT '' AFTER id;
ALTER TABLE _tmp_table CHANGE id id int(11) NOT NULL FIRST;
ALTER TABLE _tmp_table CHANGE id id int(11) NOT NULL AFTER name;

SQLite doesn't support FIRST/AFTER, so this implementation simply ignores these. Previously, for ADD COLUMN statements, it ended up with SQLSTATE[HY000]: General error: 1 near "FIRST": syntax error. and SQLSTATE[HY000]: General error: 1 near "AFTER": syntax error.; for CHANGE statements, this was already ignored and I only added a test.

[brandonpayton]

It would be good to also make assertions about the resulting table structure like this:

sqlite-database-integration/tests/WP_SQLite_Translator_Tests.php

Lines 885 to 907 in dea3efd

	$this->assertQuery( 'DESCRIBE _tmp_table;' );
	$results = $this->engine->get_query_results();
	$this->assertEquals(
	array(
	(object) array(
	'Field' => 'name',
	'Type' => 'varchar(20)',
	'Null' => 'NO',
	'Key' => '',
	'Default' => '',
	'Extra' => '',
	),
	(object) array(
	'Field' => 'column',
	'Type' => 'int',
	'Null' => 'YES',
	'Key' => '',
	'Default' => null,
	'Extra' => '',
	),
	),
	$results
	);

[JanJakes]

👍 Added in c4efdfd.

[brandonpayton]

👍 Let's add them after each assertQuery() here to check each is doing what is expected.

[JanJakes]

OK! Added in 4615adc.

[brandonpayton]

What kind of SQL syntax is this comma check intended to address? Could you provide an example?

[JanJakes]

@brandonpayton Without the check, it can consume too much, up to the following column definitions.

E.g., for a query like this one:

ALTER TABLE _tmp_table
ADD COLUMN new1 VARCHAR(255),
ADD COLUMN new2 VARCHAR(255) FIRST

You would get a SQLSTATE[HY000]: General error: 1 near ",": syntax error., since you've consumed up to FIRST from the second column when processing the first column.

[brandonpayton]

Ah, I see. I hadn't noticed that the translator loops to support the comma-separated MySQL ALTER TABLE syntax.

Let's add a test for comma-separated syntax as well.

[brandonpayton]

Thanks for explaining!

[JanJakes]

Added a test in 31ecd65.

[brandonpayton]

Thanks for this PR, @JanJakes!

I left a comment about the tests and a question about part of the implementation.

[JanJakes]

@brandonpayton Thanks! I added a commit and tried the to explain the need for the comma check.

[brandonpayton]

IIUC, something must not be working properly because the column order in the tests do not appear to be changing.

[brandonpayton]

Shouldn't new_first_column be first now?

[brandonpayton]

Shouldn't new_column be after id now, so the resulting column order would be:

• new_first_column
• id
• new_column
• name

[brandonpayton]

After this change, I think the resulting column order should be:

• id
• new_first_column
• new_column
• name

Am I missing something?

[brandonpayton]

After this change, I think the resulting column order should be:

• new_first_column
• new_column
• name
• id

Am I missing something?

[brandonpayton]

After this change, shouldn't the resulting column order be the following?

• new2
• id
• new1
• new3

[JanJakes]

@brandonpayton Please, see the description of the PR. As it says, "SQLite doesn't support FIRST/AFTER, so this implementation simply ignores these".

Actually implementing the order would mean changing simple ADD COLUMN rewriting to CHANGE-like copy-table behavior.

[brandonpayton]

@brandonpayton Please, see the description of the PR. As it says, "SQLite doesn't support FIRST/AFTER, so this implementation simply ignores these".

🤦‍♂️ Right. I read that originally but lost the detail while multi-tasking.

These changes look reasonable to me and test well.