Maintain indexes / triggers after altering sqlite tables

[MasterOdin]

PR fixes a bug where altering sqlite tables that had indexes or triggers would drop the indexes or triggers. This was because indexes and triggers are not contained in the CREATE TABLE sql that we used to create a table copy the indexes and triggers would be get copied over, and so on dropping the original table it would delete the indexes and triggers, and the rename would not bring them back.

This PR adds new post steps of recreating the indexes and triggers after we rename the temporary table we created back to its original name.

Fixes #1290

[dereuromark]

Should we finish up the 0.12 milestone with last release and then do 0.13?

[MasterOdin]

@dereuromark I thought that cakephp/migrations#517 would need to be merged before releasing 0.13, but I'm not sure the status of that PR.

[MasterOdin]

I do plan to finish this up with the caveat of disabling it for renaming or dropping columns as those requiring potentially rewriting the trigger or index. I figure it's at least a step in the right direction, and document these cases for now.

[dereuromark]

Shall we wait on this for the next patch release? Or release "master" already as is?

[MasterOdin]

This is not yet ready for release (as evidenced by the broken tests), and I have not had time to come back to this yet. We should not have this PR hold up a new release.

[ndm2]

Sorry for unsolicitedly sliding in here, but I need this in order to get stuff in cakephp/migrations fixed/tested 🥲

[MasterOdin]

@ndm2 Do you think the PR is ready to merge now? Given that all existing tests are passing, and two new tests added for coverage on this, I think should be safe?

[ndm2]

It looks pretty safe... famous last words.

This is assuming that the SQL in the master table is safe to run as-is, and that indices and triggers are always being scrubbed on table drops.

Maybe a look into temporary triggers is advised, I'm not sure how exactly they function and how they are stored?

[MasterOdin]

This follows the official method for doing these kinds of operations as described in the sqlite docs, so I'm not particularly concerned on getting invalid SQL from the sqlite_master table, or that triggers / indexes aren't removed on dropping a table.

From that doc, they do mention views, and not sure if we have a test case for that.

[ndm2]

Does Phinx support views at all? Altering a table could of course affect a view, but I don't think there's much that Phinx can do, as views are arbitrary select statements, and aren't tied to specific tables (which has the effect that they're also not being deleted when tables are dropped).

[MasterOdin]

Phinx doesn't natively support views, and I'm not sure what sqlite (or other DBs) do if you drop a table that a view references. In theory, with a SQL statement parser, it might be possible to apply migrations and such to view statements automatically when you rename a table or column or whatever, but that's out of scope for this bugfix.

[ndm2]

Maybe it wasn't that safe after all. I see this breaking when columns are being removed as part of a rollback, where it will try to re-add indices for removed columns :/

But I'm a bit irritated, as I thought that reversed plans would drop indices before dropping columns 🤔

[MasterOdin]

Can you open a new bug report with a migration that demonstrates that bug?

[ndm2]

Will do once I've figured if it's maybe something I or cakephp/migrations is doing wrong.