[5.5] Fix missing table prefix in SQLiteGrammar compileDropColumn()

[tomlankhorst]

Fixes #22741

[taylorotwell]

Don't change so many tests for this. Just write one new test with a new grammar for this functionality.

[sisve]

Also; this is a change in behavior and should probably target the 5.6 release and be part of the list of upgrade notes.

[tomlankhorst]

This prefix issue might affect any single compile instruction as they are individually responsible for calling $this->wrapTable($blueprint) to correctly prefix the table.
@sisve I agree on the different behavior, yet it seems like a deviation from expected and intended behavior.

I've created a testDropColumn() test for SQLite that shows this prefix issue but it required me to mock and use some quite specific parts of the Doctrine DBAL. Would like to hear your feedback.

[sisve]

Absolutely, the prefix should be used in this case. However, existing migrations are already written with workarounds where people have hardcoded the prefix. I think it's too much to ask to have people go back and change their existing migrations in the middle of the 5.5 release. It would be easier to ask that in the 5.6 upgrade guide.

[taylorotwell]

Sorry to go around about the tests but really I would just write a Integration test that creates a :memory: sqlite table and creates a table with a prefix and verifies it was created with the prefix.

[tomlankhorst]

@sisve Well, to me it seems that dropColumn() is not working in any situation when SQLite with table prefixes are used. So any currently working situation would either:

1. Use no table prefixes
2. Use prefixes and use manual queries
3. Not work because $tableDiff and $tableDiff->removedColumns[] are using wholly different table names.

So, in these situations

1. The fix does not alter behavior
2. The fix does not alter behavior
3. The fix alters undocumented and potentially dangerous behavior

Image a situation in which one uses a main database without prefix and a test database that is prefixed with 'test_'. dropColumn() would mix up the databases which may have very unexpected results.

[tomlankhorst]

@taylorotwell I've rewritten the test to use an actual :memory: database.

[victorlap]

How is this test testing anything? I don't see any assert* functions being called?

[tomlankhorst]

@victorlap
The SQLite driver will throw an exception when the prefixes are not set properly.
Their absence marks the success of the test.
It might be good to additionally assert that hasColumn('name') is false.

... and that test shows that there is a prefixing issue in compileColumnListing as well. Fixed in new PR.