Allow multiple migration directories and namespaces #856
Conversation
goetas
force-pushed
the
multiple-migrations-path
branch
from
c5db9e9 to
2d3e7e6
Compare
|
The difficould part of this PR is that |
That is correct. I believe that the version number no longer needs to be unique, but it must be unique within the migration namespace. Would you agree? |
I agree, but would this kind of change considered a BC break? Or the I would like to know this because if is a BC break then I would choose a different approach and target it to a possible 3.0... |
|
Another question: the "version" is used to decide with entry store in the migrations table, if we change that behavior, then the "migrations_table" will need to be migrated to a new format... And this can be an even bigger headache even for 3.0 |
|
Yes, that is always going to be a headache. I was already thinking about this when looking at #852: technically, it's an easy change, but changing the migrations table makes it very expensive and dangerous to do so. It would be nice if we could introduce migrations for the migrations table itself, but it's not easy to build. @jwage any idea here? Did we already have situations where we changed the migration table structure? |
Absolutely.
That is awesome, thank you very much! ❤️ |
|
@alcaeus I remember that 1-3 years ago Edit: after reading a bit seems it just got lost in discussions... |
|
Not that I know of - 2.0 was mainly the work of @jwage. I'm not very involved in this one. |
There was a problem hiding this comment.
I think this is a great first step... It's quite nice that you manage to provide a BC layer but if we decide to target v3.0 maybe we can be a bit braver and simplify things even further (like using namespaces and relying on composer autoloader).
Here are some thoughts/comments (just a braindump really), thanks for pushing this further 👍
| @@ -49,6 +49,9 @@ class Configuration | |||
| /** @var string */ | |||
| private $migrationsExecutedAtColumnName = 'executed_at'; | |||
|
|
|||
| /** @var string[][] */ | |||
There was a problem hiding this comment.
It's better to be more explicit about the keys here (since we're relying on it), so array<string, array<string, string>>.
I'm just wondering why do we need to set the path as both key and value (especially when only using the value). Simplifying that we could represent it as array<string, string[]>.
| @@ -49,6 +49,9 @@ class Configuration | |||
| /** @var string */ | |||
| private $migrationsExecutedAtColumnName = 'executed_at'; | |||
|
|
|||
| /** @var string[][] */ | |||
| private $migrationsDirectories = []; | |||
|
|
|||
| /** @var string|null */ | |||
| private $migrationsDirectory; | |||
There was a problem hiding this comment.
I'd suggest dropping $migrationsDirectory and $migrationsNamespace entirely. That simplifies getMigrationDirectories(), getMigrationsDirectory(), and getMigrationsNamespace()
The method setMigrationsDirectory() would do the logic $this->migrationsDirectories[$this->getMigrationsNamespace() ?? ''] = [$this->migrationsDirectory => $this->migrationsDirectory]; (we could also use int keys as mentioned before to simplify things).
setMigrationsNamespace() would be slightly more complicated to potentially transferring the configuration of the directory settings for the namespace.
| return; | ||
| } | ||
|
|
||
| $this->registerMigrationsFromDirectory($migrationsDirectory); | ||
| foreach ($migrationDirectories as $namespace => $paths) { |
There was a problem hiding this comment.
I think that exposes the internals of Configuration a bit too much, maybe we can provide a method there to encapsulate things in a better way. Like:
class Configuration
{
/**
* @return iterable<array<string|null>>
*/
public function iterateMigrationDirectories() : iterable
{
foreach ($this->migrationsDirectories as $namespace => $paths) {
foreach ($paths as $path) {
yield [$path, $namespace ?: null];
}
}
}
}Then we could do this here:
foreach ($this->configuration->iterateMigrationDirectories() as $directoryConfiguration) {
$this->registerMigrations(
$this->migrationFinder->findMigrations(...$directoryConfiguration)
);
}We would also be able to drop count($migrationDirectories) === 0 too since we would have an empty iterator when nothing is configured.
There was a problem hiding this comment.
If Configuration#getMigrationDirectories() is only used by this method we can simply no create it too 😄
| @@ -103,6 +106,19 @@ public function __construct( | |||
| $this->migrationsColumnLength = strlen($this->createDateTime()->format(self::VERSION_FORMAT)); | |||
| } | |||
|
|
|||
| public function addMigrationsPath(string $path, ?string $namespace = null) | |||
There was a problem hiding this comment.
I'd suggest always requiring the namespace.
Summary
This is a WIP pr for #853, is allows to define multiple paths (and namespaces) for migrations.
The generation and reporting are not yet implemented, I'm publishing the PR as work-in-progress to get early feedback.