Fix migrations being run when nothing changes

[pabloelcolombiano]

Fixes #533

[othercorey]

Can you add a test case for this?

[pabloelcolombiano]

The shouldDropTables method is protected, therefore it cannot be tested as such.

In my original implementation, the migrator had a protected connectionsWithModifiedStatus property, with a public getter. This enables to test if a connection was found as dirty and should be dropped, or not. The test looked like this.

If you are O.K. with adding this property and a getter as described, I would then be able to test this, and also the two cases where a connection should be dropped.

Or do you see an alternative way to test the feature?

[pabloelcolombiano]

There is actually a way to test the feature without changing the src code. Simply by performing an insertion before running the migrator, and seeing if the connection gets truncated or not. On my way implementing that...

[markstory]

Could a test for this be to add a new source directory. Then partially run those migrations, and then use migrator and check its log output? That isn't the best test but it covers the user facing behavior at least.

[pabloelcolombiano]

The path I am taking right now is to run the migrator, set the end date of the entry in the phinx log to 'Foo', and re-run the migrator. If the end_date is 'Foo', the migrations were not re-run. if the end_date is now, the migrations were re-run.
Three scenarios:

• all migrations up -> end_date == 'Foo'
• some migrations down -> end_date == now()
• some migrations missing -> end_date = now()

[pabloelcolombiano]

The problem I am facing now, is that if we run:

$migrator->runMany([
   ['plugin' => 'Migrator',],
   ['plugin' => 'Migrator', 'source' => 'Migrations2',],
], false);

the first set of migrations sees the migrations of the second set as missing. So the migrations get re-run again.
Scratching my head...

[pabloelcolombiano]

I think in this configuration, with several sets of migrations targeting the same migration table, there is no way to avoid that migrations get re-run every time. But this is really a hedge case. This will not re-run the migrations:

$migrator->runMany([
   ['plugin' => 'Migrator',],
   ['source' => '../../Plugin/Migrator/config/Migrations2',],
], false);

[othercorey]

Could a test for this be to add a new source directory. Then partially run those migrations, and then use migrator and check its log output? That isn't the best test but it covers the user facing behavior at least.

Running an integration test for this seems reasonable. We don't always need to test the single function that should affect it.

[pabloelcolombiano]

Running an integration test for this seems reasonable. We don't always need to test the single function that should affect it.

Just to make sure we are on the same page here:
Are any additional tests or actions needed here?
If so, please detail precisely which additional scenario need to be covered. If the tests provided do not match your expectations from an "integration test", let me know which scenario would.

[markstory]

Thank you 🎉