Migrator issue on multiple run on the same connection

[pabloelcolombiano]

This is a (multiple allowed):

• [x] bug

• [ ] enhancement

• [ ] feature-discussion (RFC)

• CakePHP Version: 4.3.0

• Migrations plugin version: 3.2.0

• Database server MySQL 8.07.3EXACT RELEASE VERSION

• Platform / OS: Docker

What you did

I need to run the migrator on my core, and in my TestPlugin.

According to the doc, I should be able to do:

$migrator = new Migrator();
$migrator->run(
  ['connection' => 'test'],
  ['plugin' => 'TestPlugin']
);

This is not the case, but instead I should do:

$migrator = new Migrator();
$migrator->run();
$migrator->run(['plugin' => 'TestPlugin']);

The problem with this approach, is that the following happens:

1. $migrator->run(); new migrations are found on the test connection, we drop the tables of connection test and run the core migrations creating some tables.
2. $migrator->run(['plugin' => 'TestPlugin']); new migrations are found in the TestPlugin migrations, we drop the tables of connection test and run the TestPlugin migrations.

As a result, the tables created in 1. got dropped, and only the ones created in 2. are left.

Expected Behavior

I think we should stick to the solution as described in the docs, which is IMO the only way to ensure that step 2. does not erase step one.

An alternative would be to do: $migrator = new Migrator(); $migrator->addMigration(['connection' => 'test']); $migrator->addMigration(['plugin' => 'TestPlugin']); $migrator->run();

I can submit PR if required.

[markstory]

According to the doc, I should be able to do:

My bad, I missed updating the documentation. I'll have that updated shortly.

I think we should stick to the solution as described in the docs, which is IMO the only way to ensure that step 2. does not erase step one.

We did miss the need to run migrations for multiple plugins on the same connection when we were simplifying the internals. Would something like

$migrator = new Migrator();
$migrator->run(['connection' => 'test', 'app' => true, 'plugin' => ['TestPlugin', 'OtherPlugin'])

Work for you? The API you've proposed looks like it would remove parameters from run() which would count as a breaking change for us. The above should be backwards compatible and lets users define multiple plugins + the application migrations to run. If the plugins key is present and the app key is missing it is assumed that app=false. If both app and plugins are present then we can assume that app=true.

[pabloelcolombiano]

Thanks for the quick update. Your solution would work. I do not see any obvious case though where app would be set to false. The migrator wants to get as many migrations as provided to run, so I'd go for a app=true by default. We'll have a $migrator->run() per connection, that's ok.

There will be some edge cases where the app or a plugin has its migrations in another directory. With the solution taken, this will not be possible to combine, which the

$migrator = new Migrator();
$migrator->run(
  ['connection' => 'test', 'source' => 'some/directory'],
  ['plugin' => 'TestPlugin']
);

solution would offer.

[raul338]

I too faced the problem.

I belive it is important to specify the migrations order (including app).

Ex.

1. Run pluginA
2. Run pluginB
3. Run App migrations
4. Run pluginC

how could the order be set including the app migrations?

(In my case, i have a "base" plugin that extends ACL, it modifies the ACL tables)

[markstory]

I do not see any obvious case though where app would be set to false. The migrator wants to get as many migrations as provided to run, so I'd go for a app=true by default.

Wouldn't you want app=false when you're running migrations for plugins on a secondary connection? If a plugin has its own database I don't know if you'd also want the app tables there.

how could the order be set including the app migrations?

Couldn't the proposed app flag handle that?

[pabloelcolombiano]

I do not mind having app to false per default, either way, true or false by default will work. You'll arguably have more users with migrations in app and in a plugin, both on the same connection; than migrations in app in one database, and in a plugin in a secondary database.

Regarding the order, it will be complicated to manage that. How would you generate the example provided by @raul338 ? I see no possibilities with the current implementation.

[markstory]

Regarding the order, it will be complicated to manage that. How would you generate the example provided by @raul338 ? I see no possibilities with the current implementation.

Yeah, that isn't possible :thinking.

What if we scrap the app option and instead go with something like

$migrator->run(['connection' => 'test', 'sources' => ['PluginA', 'PluginB', 'app', 'PluginC']]);

// Load only a single plugin, the app source will also be used.
$migrator->run(['connection' => 'test', 'plugin' => 'PluginA']);

This would keep backwards compatibility as well, and the new sources list gives users control over the order of operations.

[pabloelcolombiano]

Though this would technically cover some use cases, I have two concerns there:

1. The key source is quite confusing, as it is used by migrations for indicating a specific directory.
2. There is no possibility to use the legacy source option, e.g. to do so:

   $migrator = new Migrator();
   $migrator->run(
   ['plugin' => 'PluginA']
   ['source' => 'some/directory'],
   ['plugin' => 'PluginB'],
   );

The release is still fresh, isn't there a possibility to publish an erratum in a 3.2.1, or something, saying that the migrator as implemented in the 3.2.0 has some design issues? We could also have a new method, say runMigrations(), keeping run as it is for BC but deprecating it.

[pabloelcolombiano]

Or, as mentioned above, something like:

$migrator = new Migrator();
$migrator->add(['connection' => 'test']);
$migrator->add(['plugin' => 'TestPlugin', 'source' => 'some/diretory']);
$migrator->runAll();

[markstory]

The key source is quite confusing, as it is used by migrations for indicating a specific directory.

Good point, I forgot about the source option when running migrations.

Or, as mentioned above, something like:

I'm not a fan of the stateful method as it creates more opportunities for incorrect usage. For example you could call add() after runAll() and have a silent failure.

We could also have a new method, say runMigrations(), keeping run as it is for BC but deprecating it.

I like this approach, we could use the runAll() name you had earlier:

$migrator->runAll([
  ['connection' => 'test'],
  ['plugin' => 'TestPlugin']
]); 

This method could also support the $truncateTables parameter that run has to skip truncation for the migration collections that were run.

[pabloelcolombiano]

Nice one, making a feature out of a bug, I like it!

So we'll have $migrator->run(array $config); to run a single set of migrations (already implemented).

And we add $migrator->runMay(array[] $configs); to run multiple sets of migrations. I find runMany sounds a little more like Cake.

Everything stateless.

Let me take this over if you want, I'll send a PR on both the plugin and the docs.