Record when a migration was executed. #675

jwage · 2018-05-12T03:29:55Z

Q	A
Type	feature
BC Break	no
Fixed issues	#331

Summary

Log when a migration was executed.

TODO

Implement MigrationTable, MigrationTableStatus and MigrationTableUpdater class.
If the table does not exist, default notnull to true.
Show the executed at date in the console commands output.
Update documentation.

localheinz

👍

localheinz · 2018-05-15T11:02:31Z

lib/Doctrine/Migrations/Configuration/AbstractFileConfiguration.php

@@ -23,6 +23,7 @@
        'migrations_namespace',
        'table_name',
        'column_name',
+        'created_at_column_name',


Maybe executed_at_column_name, and consequently, a rename of the corresponding methods (and fields), would be more appropriate?

After all, this reflects the time of when the migration was executed, rather then when the migration (file) was created.

What do you think?

localheinz

Nice! 👍

localheinz · 2018-05-15T16:32:08Z

lib/Doctrine/Migrations/MigrationTableManager.php

+                continue;
+            }
+
+            $table->dropColumn($column->getName());


If I understood correctly, then on the happy path we would not need to drop any columns from the definition - what do you think about modifying to

diff --git a/lib/Doctrine/Migrations/MigrationTableManager.php b/lib/Doctrine/Migrations/MigrationTableManager.php index 175c2da..721116c 100644 --- a/lib/Doctrine/Migrations/MigrationTableManager.php +++ b/lib/Doctrine/Migrations/MigrationTableManager.php @@ -189,11 +189,9 @@ class MigrationTableManager // remove columns from the table definition that we don't care about // so we don't try to drop those columns foreach ($table->getColumns() as $column) { - if (in_array($column->getName(), $columnNames, true)) { - continue; + if (!in_array($column->getName(), $columnNames, true)) { + $table->dropColumn($column->getName()); } - - $table->dropColumn($column->getName()); } return $table;

?

That goes against early-exit rule in conditions/loops in Doctrine CS. :)

Majkl578 · 2018-05-15T17:19:30Z

lib/Doctrine/Migrations/Configuration/Configuration.php

@@ -42,6 +42,9 @@ class Configuration
    /** @var int */
    private $migrationsColumnLength = 255;

+    /** @var string */
+    private $migrationsExecutedAtColumnName = 'executedAt';


Should probably be executed_at to match table name and also avoid case sensitivity mess.

Majkl578 · 2018-05-15T17:20:14Z

lib/Doctrine/Migrations/DependencyFactory.php

    {
-        return $this->getDependency(MigrationTableCreator::class, function () {
-            return new MigrationTableCreator(
+        return $this->getDependency(MigrationTableManager::class, function () {


Closure needs a return type.

Majkl578 · 2018-05-15T17:21:03Z

lib/Doctrine/Migrations/MigrationFileBuilder.php

@@ -53,15 +64,24 @@ private function getVersionUpdateQuery(string $version, string $direction) : str
    {
        if ($direction === VersionDirection::DOWN) {
            $query = "DELETE FROM %s WHERE %s = '%s';\n";


Unnecessary variable.

Majkl578 · 2018-05-15T17:21:08Z

lib/Doctrine/Migrations/MigrationFileBuilder.php

        }

+        $query = "INSERT INTO %s (%s, %s) VALUES ('%s', %s);\n";


Unnecessary variable.

Majkl578 · 2018-05-15T17:21:38Z

lib/Doctrine/Migrations/MigrationTableManager.php

+/**
+ * @internal
+ */
+class MigrationTableManager


Any better name? 😟
https://stackoverflow.com/questions/1866794/naming-classes-how-to-avoid-calling-everything-a-whatevermanager

Any reason to not be final? We should try to let people extend abstractions and not concrete implementations

I was wracking my brain trying to think of an applicable name. Maybe MigrationTableCreator? It also updates the table too so that name isn't 100% clear. I feel like MigrationTableManager is a relevant name as it is managing the migration table :) Open to other ideas though for something better.

@lcobucci I had everything final to begin with, but that makes unit testing impossible. I think we need a standard solution for how to handle this in Doctrine projects before we start enforcing final everywhere.

Majkl578 · 2018-05-15T17:23:07Z

lib/Doctrine/Migrations/MigrationTableManager.php

+
+    public function isMigrationTableUpToDate() : bool
+    {
+        if ($this->migrationTableUpToDate === null) {


Inverting the condition will decrease the depth of this method.

Majkl578 · 2018-05-15T17:23:42Z

lib/Doctrine/Migrations/MigrationTableManager.php

+        }
+
+        if ($this->isMigrationTableCreated()) {
+            if ($this->isMigrationTableUpToDate() === false) {


Strict comparison seems unnecessary for boolean-only method.

Majkl578 · 2018-05-15T17:25:54Z

lib/Doctrine/Migrations/Version.php

@@ -194,4 +199,17 @@ public function markVersion(string $direction) : void
            );
        }
    }
+
+    private function getExecutedAt() : DateTime


DateTimeImmutable please

Also, I don't think we need a method for this if we only care about the DB value

Majkl578 · 2018-05-15T17:26:03Z

lib/Doctrine/Migrations/Version.php

+
+    private function getExecutedAtDatabaseValue() : string
+    {
+        return Type::getType('datetime')->convertToDatabaseValue(


datetime_immutable please

About the scrutinizer issue, I've submitted an issue and they're about to release a new version that works properly with assert() so it might be an idea to use it here

Majkl578 · 2018-05-15T17:27:00Z

lib/Doctrine/Migrations/MigrationTableManager.php

+    {
+        return new Column(
+            $this->executedAtColumnName,
+            Type::getType('datetime'),


datetime_immutable please

lcobucci

@jwage great job, it would be significantly easier to review if we had smaller commits though

lcobucci · 2018-05-15T18:19:09Z

lib/Doctrine/Migrations/MigrationTableManager.php

+
+    public function isMigrationTableCreated() : bool
+    {
+        if ($this->migrationTableCreated === null) {


Inverting the condition would simplify things here as well

lcobucci · 2018-05-15T18:19:57Z

lib/Doctrine/Migrations/MigrationTableManager.php

+        if ($this->migrationTableCreated === null) {
+            $this->configuration->connect();
+
+            if ($this->schemaManager->tablesExist([$this->tableName])) {


Maybe this is simpler than this if/else:

$this->migrationTableCreated = $this->schemaManager->tablesExist([$this->tableName]);

lcobucci · 2018-05-15T18:21:50Z

lib/Doctrine/Migrations/MigrationTableManager.php

+    private $platform;
+
+    /** @var string */
+    private $tableName;


I think it might be time to have a class to represent the migration table, which encapsulates: name, column name (needs a better name), column length (better name as well?), and executed at column name.

Good idea. I will make that change.

lcobucci · 2018-05-15T18:25:14Z

lib/Doctrine/Migrations/MigrationTableManager.php

+
+    public function createMigrationTable() : bool
+    {
+        $this->configuration->validate();


Maybe this is a bit too much for this method? I mean, the validation of the configuration and the check for dry-run mode could be done in a higher level. This would simplify things and maybe even remove the need of passing the configuration to this class

lcobucci · 2018-05-15T18:26:10Z

lib/Doctrine/Migrations/MigrationTableManager.php

+        return $this->migrationTableUpToDate;
+    }
+
+    public function createMigrationTable() : bool


Any reason to have this returning a boolean? It would be amazing if we could keep away from the internal classes and rely on exceptions for error handling

Possibly. I will make an issue to revisit this and maybe implement exceptions for these cases instead of returning bool.

lcobucci · 2018-05-15T18:26:52Z

lib/Doctrine/Migrations/MigrationTableManager.php

+/**
+ * @internal
+ */
+class MigrationTableManager


Any reason to not be final? We should try to let people extend abstractions and not concrete implementations

lcobucci · 2018-05-15T18:28:02Z

lib/Doctrine/Migrations/MigrationTableManager.php

+        return true;
+    }
+
+    public function getMigrationsColumn() : Column


This is a good candidate for extraction (the class I've mentioned earlier 😄)

lcobucci · 2018-05-15T18:31:30Z

lib/Doctrine/Migrations/Version.php

@@ -194,4 +199,17 @@ public function markVersion(string $direction) : void
            );
        }
    }
+
+    private function getExecutedAt() : DateTime


Also, I don't think we need a method for this if we only care about the DB value

lcobucci · 2018-05-15T18:32:16Z

lib/Doctrine/Migrations/Version.php

+
+    private function getExecutedAtDatabaseValue() : string
+    {
+        return Type::getType('datetime')->convertToDatabaseValue(


About the scrutinizer issue, I've submitted an issue and they're about to release a new version that works properly with assert() so it might be an idea to use it here

lcobucci · 2018-05-15T18:34:25Z

tests/Doctrine/Migrations/Tests/Configuration/AbstractFileConfigurationTest.php

@@ -34,6 +35,89 @@ public function testLoadChecksCurrentWorkingDirectory() : void
        chdir($cwd);
    }

+    public function testSetConfiguration() : void
+    {
+        $fileConfiguration = $this->getMockBuilder(TestAbstractFileConfiguration::class)


This really hurts my eyes, but thanks for being pragmatic and facing this beast... Maybe using $this->createPartialMock() might simplify things a bit

Majkl578 · 2018-05-15T23:55:05Z

lib/Doctrine/Migrations/Configuration/Configuration.php

+
+    public function getQuotedMigrationsExecutedAtColumnName() : string
+    {
+        return $this->getDependencyFactory()


To be honest, this thing is just a service locator now... Wiring everything should imho be done in the SF Bundle / Zend Module and/or in a factory/builder object.

It can be done, but not without a big BC break. The only place it is being used like a service locator is in places where I wanted to maintain BC. Like here. Also, these are all internal services so it feels weird to put the burden on the end user to wire all these classes up that they should not even know about or have access to.

To be clear, I am referring to developers who don't use Symfony, Zend, etc. and have their app integrated with Doctrine DBAL and Migrations. Today all that is required to configure migrations is setting up one service that is passed your DBAL connection:

$configuration = new Configuration($connection); // configure your migrations $configuration->...;

In 2.0 this is the same. All the existing setups will just continue to work. All the services that make up the internals are not meant to be public facing services. If I put the wiring in the framework integrations, wouldn't this mean that when I refactor the internals, it'd be a BC break in the framework integrations due to the wiring needing to be updated?

Majkl578 · 2018-05-15T23:58:55Z

lib/Doctrine/Migrations/MigrationTableStatus.php

+        $this->upToDate = true;
+
+        foreach ($this->migrationTable->getColumnNames() as $columnName) {
+            if (! $table->hasColumn($columnName)) {


can be inverted with continue

Majkl578 · 2018-05-15T23:59:41Z

lib/Doctrine/Migrations/MigrationTableUpdater.php

+
+        $queries = $fromSchema->getMigrateToSql($toSchema, $this->platform);
+
+        foreach ($queries as $query) {


Shouldn't this actually run inside transaction sice there are multiple queries?

They are DDL statements. I am honestly not sure of which databases today support rollbacks for DDL statements. Do you know? I guess it doesn't hurt and if the database supports it, it will work.

From my experience, PostgreSQL definitely does, MySQL definitely does not.
More vendors documented here: https://wiki.postgresql.org/wiki/Transactional_DDL_in_PostgreSQL:_A_Competitive_Analysis

I added the transaction, it won't hurt 👍

stof · 2018-05-16T09:44:15Z

lib/Doctrine/Migrations/MigrationTableUpdater.php

+
+        $queries = $fromSchema->getMigrateToSql($toSchema, $this->platform);
+
+        $this->connection->beginTransaction();


this should be done only if the platform supports transational DDL IMO (which DBAL knows)

DBAL doesn't know this currently. It only has the supportsTransactions() method. We discussed submitting an enhancement to the DBAL for this though. For now though, we figured having this here for everyone wouldn't hurt. Thoughts?

ah indeed, I though it was something known by DBAL and used by doctrine/migrations, but doctrine/migrations always wraps the migration in a transaction, even if it would not be able to rollback it fully due to non transactional DDL (and migrations can ask to disable the usage of the transaction).

So yeah, keep it like that.

jwage added Enhancement WIP labels May 12, 2018

jwage added this to the 2.0 milestone May 12, 2018

jwage self-assigned this May 12, 2018

jwage force-pushed the migration-created-at branch from 52cb187 to 093347f Compare May 12, 2018 03:34

localheinz approved these changes May 12, 2018

View reviewed changes

jwage force-pushed the migration-created-at branch 15 times, most recently from 9299575 to ab0b4bd Compare May 15, 2018 01:28

jwage requested review from Majkl578, lcobucci, mikeSimonson and stof May 15, 2018 01:33

localheinz reviewed May 15, 2018

View reviewed changes

jwage force-pushed the migration-created-at branch 4 times, most recently from fa07fd9 to 6fa5668 Compare May 15, 2018 15:46

localheinz approved these changes May 15, 2018

View reviewed changes

Majkl578 reviewed May 15, 2018

View reviewed changes

jwage force-pushed the migration-created-at branch 2 times, most recently from ce34441 to 89f1c1b Compare May 15, 2018 18:31

lcobucci suggested changes May 15, 2018

View reviewed changes

jwage force-pushed the migration-created-at branch 8 times, most recently from 8881f9c to 4a347e5 Compare May 15, 2018 23:24

jwage requested review from lcobucci and Majkl578 May 15, 2018 23:25

jwage force-pushed the migration-created-at branch from 4a347e5 to 95485c6 Compare May 15, 2018 23:32

Majkl578 reviewed May 16, 2018

View reviewed changes

jwage force-pushed the migration-created-at branch 2 times, most recently from 46305a3 to 203db81 Compare May 16, 2018 01:38

stof reviewed May 16, 2018

View reviewed changes

jwage force-pushed the migration-created-at branch 4 times, most recently from c7d6f03 to e08e9b1 Compare May 16, 2018 16:18

Record when a migration was executed.

fb26b48

jwage force-pushed the migration-created-at branch from e08e9b1 to fb26b48 Compare May 16, 2018 16:20

jwage merged commit ead074a into master May 16, 2018

jwage deleted the migration-created-at branch May 16, 2018 18:29

jwage removed the WIP label May 19, 2018

bobvandevijver mentioned this pull request Feb 4, 2019

Updating migrations table using Oracle fails #788

Open


		$queries = $fromSchema->getMigrateToSql($toSchema, $this->platform);

		foreach ($queries as $query) {


		$queries = $fromSchema->getMigrateToSql($toSchema, $this->platform);

		$this->connection->beginTransaction();

Record when a migration was executed. #675

Record when a migration was executed. #675

Conversation

jwage commented May 12, 2018 • edited Loading

Summary

TODO

localheinz left a comment

Choose a reason for hiding this comment

localheinz May 15, 2018 • edited Loading

Choose a reason for hiding this comment

localheinz 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

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

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

lcobucci 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwage May 16, 2018 • 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

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

jwage commented May 12, 2018 •

edited

Loading

localheinz May 15, 2018 •

edited

Loading

jwage May 16, 2018 •

edited

Loading