Migrations 3.0 #858

goetas · 2019-09-15T09:59:04Z

Q	A
Type	improvement
BC Break	yes
Fixed issues	#857 #853 #726 #805 #742 #854

Summary

This PR proposes what could become doctrine/migrations 3.0. (Some code and ideas are borrowed from #162)

This is a "work in progress", so any feedback is appreciated.

Exiting user-facing features are have been kept as they are ( some BC breaks are present of course).

Summary of changes:

The "version" is not anymore a number or date, but the FQCN of the migration class.
Multiple namespaces are allowed (and paths). This allows to provide third party migrations, plugin-provided migrations and and dependent migrations as well (as described in Migration dependencies #853)
Extracted the "metadata table" logic in its own interface as in [WIP] 2.0 Prototype #162
Most of the logs/prints not use psr/log
Used dependency injection in most of the "core" code parts (v1 and v2 have the god classes as Configuration, MigrationRepository and Version that were doing all possible kind of things and having circular dependencies between each other )

Tests have not been updated, but the provided prototype has most of the features already working.
Rollup and Diff commands have not been updated yet.

I realize that is a huge PR and hard to review, but the configuration and migration-repository class was passed all over the places, and as soon as I started to remove methods from it, each other class had to be changed...

Todo:

migrate all functionalities
update tests
phpstan
prepare migration path for 3.0 update (see Auto migrate the version table goetas/migrations#1)
document migration path

lib/Doctrine/Migrations/Metadata/AvailableMigration.php

OskarStark · 2019-09-15T18:26:36Z

lib/Doctrine/Migrations/Metadata/AvailableMigration.php

+    /** @var AbstractMigration */
+    private $migration;
+
+    public function __construct(Version $version, AbstractMigration $migration)


I am for making the ctor private and a have a static named constructor to be immutable.

Also I am for version() and migration() without the get prefix.

Same should be applied to the other value objects

lib/Doctrine/Migrations/Version/ExecutionResult.php

OskarStark · 2019-09-16T06:38:43Z

lib/Doctrine/Migrations/Metadata/ExecutedMigration.php

+     */
+    public $executionTime;
+
+    public function __construct(Version $version, ?DateTime $executedOn, ?int $executionTime)


ExecutionTime::fromMilliseconds(...) ?

Hmm.. . And that method what should do? Just return back the same value?

Yes but its self explanatory. Looking at $executionTime variable name is not helpful, unless it would be named $executionTimeInMs oder $executionTimeInMilliseconds.

it could also provide a ExecutionTime->inSeconds() which could be helpful when outputting it to the user? 🤔

What you are suggesting is a value object instead of the integer, right?

final class ExecutionTime { /** var int */ private $time; private function __construct(int $time) { $this->time = $time; { public function fromMilliseconds(int $time): self { return new self($time); } public function toMilliseconds(): int { return $time; } public function toSeconds(): // int or float here { return $time / 1000; } }

Something like this, yes and then use it like this:

public function __construct(Version $version, ?DateTime $executedOn, ?ExecutionTime $executionTime)

lib/Doctrine/Migrations/Metadata/ExecutedMigration.php

OskarStark · 2019-09-16T07:30:42Z

lib/Doctrine/Migrations/Metadata/TableMetadataStorage.php

+                $this->platform->getDateTimeFormatString(),
+                $row['executed_on']
+            );
+            $migration  = new ExecutedMigration($version, $executedOn, $row['execution_time']? intval($row['execution_time']) : null);


And here:

Suggested change

$migration = new ExecutedMigration($version, $executedOn, $row['execution_time']? intval($row['execution_time']) : null);

$migration = new ExecutedMigration($version, $executedOn, $row['execution_time']? ExecutionTime::fromMilliseconds(intval($row['execution_time'])) : null);

jwage · 2019-09-16T15:11:08Z

This is great. Love to see someone tearing this a part more! Can we document the BC breaks and update some of the docs to reflect the changes? I think it would help me understand what has changed so far from the users perspective and would allow me to provide feedback. Let me know when you are ready for a thorough review.

goetas · 2019-09-16T15:16:32Z

This is great. Love to see someone tearing this a part more! Can we document the BC breaks and update some of the docs to reflect the changes? I think it would help me understand what has changed so far from the users perspective and would allow me to provide feedback. Let me know when you are ready for a thorough review.

Sounds great! I have discussed this a bit with @alcaeus , currently I'm spending on this PR 5-8 h/week. I was planning in documenting the changes a bit. As soon as I have something will ping you here!

stof · 2019-09-16T15:36:51Z

The "version" is not anymore a number or date, but the FQCN of the migration class.

Is there a migration plan available for that ? This involves at least 2 things regarding BC:

it will probably require to change the schema of the migration table (to allow unlimited length on the version field. How would that schema update be handled ?
what would happen for an existing project using migrations, where the DB contains dates as versions for already applied migrations. Can these be mapped back to new versions to recognize them as already migrated ?

Also, how will the ordering of migrations work ? Currently, when you have multiple unexecuted migrations, they are applied based on their version using date sorting. But once their version is the FQCN, we need to have a different sorting algorithm, and you don't describe what it is AFAICT.

goetas · 2019-09-16T15:47:33Z

it will probably require to change the schema of the migration table (to allow unlimited length on the version field. How would that schema update be handled ?
what would happen for an existing project using migrations, where the DB contains dates as versions for already applied migrations. Can these be mapped back to new versions to recognize them as already migrated ?

For both of this questions have the same solution. This part of code will be responsible for doing that migration. Is not yet coded, but is already on my todo-list.

Also, how will the ordering of migrations work ? Currently, when you have multiple unexecuted migrations, they are applied based on their version using date sorting. But once their version is the FQCN, we need to have a different sorting algorithm, and you don't describe what it is AFAICT.

The current sorting algorithm used in v2 for migrations is ksort($migrations,SORT_STRING) and that will be the same for FQCN, and should produce the same results (in v2 the version is just the part of the class name after Version*). So if migrations are in the same namespace there is no need for any change in the sorting algorithm.
If migrations use multiple namespaces (available only in this proposed 3.0), then you are responsible to provide your sorting algorithm to manage dependencies between namespaces.
In an ideal world, composer dependency graph could be used to provide out-of-the-box advanced sorting, but this is too app-specific and out of the scope for now.

stof · 2019-09-16T15:52:35Z

n an ideal world, composer dependency graph could be used to provide out-of-the-box advanced sorting, but this is too app-specific and out of the scope for now.

I don't even see how that's possible. your own migrations will still need interleaving with the dependency migrations, as some of your migrations will have been written before the upgrade of one of the dependencies bring a new migration there. A dependency graph is not the solution here.

goetas · 2019-09-16T16:09:11Z

If you have single migrations that depend on other migrations from other packages, you have to manage it on your own with a custom sorting algorithm. I doubt that will be possible to find a decent way to sort migrations out of the box and cover most of the cases, thus custom sorting should allow users to handle it on their own. This pr does not propose a way to manage migration dependencies, it just allows you to sort them in any other you prefer. Did i miss maybe your point or it explains the problem?

…

On Mon, 16 Sep 2019, 17:52 Christophe Coevoet, ***@***.***> wrote: n an ideal world, composer dependency graph could be used to provide out-of-the-box advanced sorting, but this is too app-specific and out of the scope for now. I don't even see how that's possible. your own migrations will still need interleaving with the dependency migrations, as some of your migrations will have been written before the upgrade of one of the dependencies bring a new migration there. A dependency graph is not the solution here. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#858?email_source=notifications&email_token=AAF5UJ4F7CFJXUNEZO5W5NTQJ6TULA5CNFSM4IWZ62MKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6ZTSIY#issuecomment-531839267>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAF5UJ2X2ZSUDYDGVSCUSVDQJ6TULANCNFSM4IWZ62MA> .

lib/Doctrine/Migrations/DependencyFactory.php

goetas · 2019-09-18T16:32:58Z

Getting closer to something that works :)

lib/Doctrine/Migrations/Metadata/AvailableMigrationsList.php

goetas · 2019-09-19T15:55:23Z

Current status: ~90% of functionalities have been migrated (--range* feature on the migrate command is still missing).

I have also migrated most of the tests, wrote some new and deleted a lot of other. This is some critical issue. Since the previous version had 3-4 god classes tightly couples doing most of the job (Version, Configuration, MigrationRepository), there were a bunch of tests checking that the interaction between those are correct.
In the new implementation, the size of those classes has been cut by ten in some cases, and logic has been split in more independent classes. Because of that, the previously exiting tests became much more of a problem rather than a benefit.. thus I have deleted them.

My plan in the next days is to restore the test coverage to something around 95%, before that I plan to update the documentation with the user-facing changes. Documenting the code changes is a bit more difficult...

what about using https://github.com/Roave/BackwardCompatibilityCheck ? (here the GIST with the changes detected by it)

/cc @jwage @alcaeus

@OskarStark Thanks for the revirews, as soon as I will have a more polished architecture, will start addressing the comments.

jwage · 2019-09-19T16:51:24Z

@goetas Sounds like a good plan to me. We also need to at some point look in to how this affects all the 3rd party integrations, like the DoctrineMigrationsBundle for Symfony for example.

goetas · 2019-09-19T16:54:40Z

We also need to at some point look in to how this affects all the 3rd party integrations, like the DoctrineMigrationsBundle for Symfony for example.

That is on my todo-list (since the project that i'm working on will need the bundle too)

goetas · 2019-09-19T19:24:20Z

@jwage d916a07 documents the user-facing changes.

Some of the documented things are not yet fully implemented (home to have some time next week to discuss them with @alcaeus )

goetas · 2019-09-22T10:52:36Z

I've analyzed the current master and this branch with PhpDependencyAnalysis to track dependencies between classes in this doctrine/migrations.

The configuration file I've used is:

mode: 'usage'
source: './lib'
filePattern: '*.php'
formatter: 'PhpDA\Writer\Strategy\Svg'
target: './complex-cycle.svg'
groupLength: 3
visitorOptions:
  PhpDA\Parser\Visitor\Required\DeclaredNamespaceCollector: {minDepth: 2, sliceLength: 6, excludePattern: '/^((?!.*(Doctrine\\Migrations))|(?=.*Exception)).*$/'}
  PhpDA\Parser\Visitor\Required\MetaNamespaceCollector: {minDepth: 2, sliceLength: 4, excludePattern: '/^((?!.*(Doctrine\\Migrations))|(?=.*Exception)).*$/'}
  PhpDA\Parser\Visitor\Required\UsedNamespaceCollector: {minDepth: 2, sliceLength: 4, excludePattern: '/^((?!.*(Doctrine\\Migrations))|(?=.*Exception)).*$/'}

The results are the following https://gist.github.com/goetas/e5ee7546496e673360df5657ec5b38bd .

Red line in the graph are circular dependencies between classes. As you can see, in v3 all circular deps are gone.

goetas · 2019-09-23T15:29:22Z

STATIC_ANALYSIS (phpstan) still missing to be green... could be pretty challenging make it work!

Any help?

stof

you need to fix the autoload_directories in phpstan.neon.dist, as the _regression folder is gone. That's what's breaking phpstan entirely right now.

lib/Doctrine/Migrations/Configuration/Connection/Loader/ConnectionHelperLoader.php

lib/Doctrine/Migrations/Configuration/Connection/Loader/NoConnectionLoader.php

lib/Doctrine/Migrations/Configuration/Loader/XmlFileLoader.php

lib/Doctrine/Migrations/DependencyFactory.php

stof · 2019-09-23T15:43:18Z

lib/Doctrine/Migrations/Generator/FileBuilder.php

@@ -65,6 +46,9 @@ public function buildMigrationFile(

    private function getVersionUpdateQuery(string $version, string $direction) : string
    {
+        // @todo this is hard to implement since MetadataStorage abstracts the migrations table


but without this, things are broken as the metadata storage is not updated with executing this file. So this must be solved.
It might mean that the MetadataStorage needs to expose a method returning the needed queries.

Does it make sense then to have metadata storage interface? If it is going to be only one implementation, what about dropping the interface and typehint the concrete implementation?

well, the metadata storage interface could have a method returning the queries necessary to update the storage (which may be no queries if the storage cannot be updated in SQL).

But then metadata storage interface will be bound to SQL, having other "storages" will make no sense... right? or in that case the extra method for the queries will just return NULL? )

I have discussed this with @alcaeus . This will be covered in a separate PR/ticket and there can be explored a solution for the issue

lib/Doctrine/Migrations/Metadata/AvailableMigrationsList.php

lib/Doctrine/Migrations/Metadata/Storage/TableMetadataStorage.php

lib/Doctrine/Migrations/MigrationPlanCalculator.php

lib/Doctrine/Migrations/Tools/Console/Command/AbstractCommand.php

goetas · 2019-09-24T15:57:13Z

Only 111 errors left :-)

lib/Doctrine/Migrations/Configuration/Configuration.php

goetas · 2019-10-01T07:31:28Z

PHPStan 0 errors! 🎉 🎉 🎉

…of having a long list

OskarStark · 2019-11-30T20:17:41Z

Congratulations 🍾🎉🎊🎈

Karimgasmi47 · 2020-01-24T16:53:04Z

lib/Doctrine/Migrations/Tools/Console/Command/DiffCommand.php

@@ -100,12 +98,12 @@ public function execute(
        InputInterface $input,


Can we also change the visibility from "public" to "protected" on the Command class to match the declaration with Symfony/Console ?
If you agree, I may open a PR

Sure! Did not know was protected

ostrolucky · 2023-05-03T11:03:10Z

I must say decision to use FQCN instead of version number is causing us issues. We have deployed branch with doctrine/migrations 3.x to our staging environment. It migrated the table. Now other branches with older version of doctrine/migrations fail because they don't recognize versions as executed anymore. For the next time, there should be a BC layer in currently maintained branch which recognizes both formats.

This was referenced Sep 15, 2019

[WIP] Allow custom sorting (used to implement migration dependencies) #855

Closed

Allow multiple migration directories and namespaces #856

Closed

OskarStark approved these changes Sep 15, 2019

View reviewed changes

OskarStark reviewed Sep 16, 2019

View reviewed changes

lib/Doctrine/Migrations/Version/ExecutionResult.php Show resolved Hide resolved

OskarStark reviewed Sep 16, 2019

View reviewed changes

goetas force-pushed the v30 branch from a5b26b2 to dca9bc4 Compare September 17, 2019 14:04

OskarStark reviewed Sep 17, 2019

View reviewed changes

lib/Doctrine/Migrations/DependencyFactory.php Outdated Show resolved Hide resolved

OskarStark reviewed Sep 18, 2019

View reviewed changes

lib/Doctrine/Migrations/Metadata/AvailableMigrationsList.php Outdated Show resolved Hide resolved

goetas force-pushed the v30 branch from 0d19e88 to 021946b Compare September 23, 2019 15:26

stof requested changes Sep 23, 2019

View reviewed changes

stof reviewed Sep 24, 2019

View reviewed changes

goetas force-pushed the v30 branch from e9ca017 to 473d1bb Compare September 24, 2019 14:48

goetas force-pushed the v30 branch from 66b8361 to cf1ef06 Compare September 30, 2019 05:58

leonhusmann reviewed Sep 30, 2019

View reviewed changes

lib/Doctrine/Migrations/Configuration/Configuration.php Outdated Show resolved Hide resolved

goetas added 16 commits November 30, 2019 19:54

refer to https://github.com/Roave/BackwardCompatibilityCheck instead …

af4e64c

…of having a long list

classgenerator test

e45fc05

rename AliasResolver into DefaultAliasResolver

1ff3659

rename AbstractCommand into DoctrineCommand

a813aa0

removed cs rules for outdated classes

880f52a

renamed ConfigurationHelperInterface into ConfigurationHelper

2251920

renamed ConnectionLoaderInterface into ConnectionLoader

5f9f2dc

removed cs rules for outdated classes

f1bab63

renamed ParameterFormatter into InlineParameterFormatter

f76de15

renamed FileBuilderInterface into ConcatenationFileBuilder

eb01c0d

renamed ExecutorInterface into Executor

c184be2

renamed Migrator into DbalMigrator

a27d617

add typehint on version class

c72b0fc

fixed typo in MetadataStorageConfiguration class

cca6f7c

re sort imports after class renaming

4626cbd

ensure there is always some metadata configuration set

5a09688

goetas force-pushed the v30 branch from 3f849f1 to 5a09688 Compare November 30, 2019 18:56

alcaeus approved these changes Nov 30, 2019

View reviewed changes

alcaeus merged commit d74785f into doctrine:master Nov 30, 2019

This was referenced Dec 1, 2019

Tight coupling in Version, Configuration and MigrationRepository #857

Closed

Use dependency injection for console commands #726

Closed

dir_name does not support multiple directories #805

Closed

goetas mentioned this pull request Jan 3, 2020

Fix migration direction calculation fails. #854

Closed

Karimgasmi47 reviewed Jan 24, 2020

View reviewed changes

goetas added this to the 3.0.0 milestone Mar 7, 2020

goetas added the BC Break label Mar 8, 2020

Vaalyn mentioned this pull request Sep 21, 2020

Add possibility to add migrations from plugins via event dispatcher AzuraCast/AzuraCast#3168

Merged

KorvinSzanto mentioned this pull request Feb 5, 2024

Update to doctrine/migrations ^3 concretecms/concretecms#11926

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrations 3.0 #858

Migrations 3.0 #858

goetas commented Sep 15, 2019 •

edited

Loading

OskarStark Sep 15, 2019 •

edited

Loading

OskarStark Sep 16, 2019

goetas Sep 16, 2019

OskarStark Sep 16, 2019

goetas Sep 16, 2019

OskarStark Sep 16, 2019 •

edited

Loading

OskarStark Sep 16, 2019 •

edited

Loading

jwage commented Sep 16, 2019

goetas commented Sep 16, 2019

stof commented Sep 16, 2019

goetas commented Sep 16, 2019

stof commented Sep 16, 2019

goetas commented Sep 16, 2019 via email

goetas commented Sep 18, 2019

goetas commented Sep 19, 2019 •

edited

Loading

jwage commented Sep 19, 2019

goetas commented Sep 19, 2019

goetas commented Sep 19, 2019

goetas commented Sep 22, 2019

goetas commented Sep 23, 2019

stof left a comment

stof Sep 23, 2019

goetas Sep 23, 2019

stof Sep 24, 2019

goetas Sep 24, 2019

goetas Oct 1, 2019

goetas commented Sep 24, 2019

goetas commented Oct 1, 2019

OskarStark commented Nov 30, 2019

Karimgasmi47 Jan 24, 2020

goetas Jan 24, 2020

ostrolucky commented May 3, 2023

	$migration = new ExecutedMigration($version, $executedOn, $row['execution_time']? intval($row['execution_time']) : null);
	$migration = new ExecutedMigration($version, $executedOn, $row['execution_time']? ExecutionTime::fromMilliseconds(intval($row['execution_time'])) : null);

		@@ -100,12 +98,12 @@ public function execute(
		InputInterface $input,

Migrations 3.0 #858

Migrations 3.0 #858

Conversation

goetas commented Sep 15, 2019 • edited Loading

Summary

OskarStark Sep 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OskarStark Sep 16, 2019 • edited Loading

Choose a reason for hiding this comment

OskarStark Sep 16, 2019 • edited Loading

Choose a reason for hiding this comment

jwage commented Sep 16, 2019

goetas commented Sep 16, 2019

stof commented Sep 16, 2019

goetas commented Sep 16, 2019

stof commented Sep 16, 2019

goetas commented Sep 16, 2019 via email

goetas commented Sep 18, 2019

goetas commented Sep 19, 2019 • edited Loading

jwage commented Sep 19, 2019

goetas commented Sep 19, 2019

goetas commented Sep 19, 2019

goetas commented Sep 22, 2019

goetas commented Sep 23, 2019

stof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goetas commented Sep 24, 2019

goetas commented Oct 1, 2019

OskarStark commented Nov 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ostrolucky commented May 3, 2023

goetas commented Sep 15, 2019 •

edited

Loading

OskarStark Sep 15, 2019 •

edited

Loading

OskarStark Sep 16, 2019 •

edited

Loading

OskarStark Sep 16, 2019 •

edited

Loading

goetas commented Sep 19, 2019 •

edited

Loading