Migration doesnt work after update to 3.8.0 ( Throws an exception if addSql is used in post methods)

[ko4an4ik]

Q	A
Version	3.80

Support Question

We encountered the following issue - after the execution of composer update, some of our migrations stopped working correctly.

In FrozenMigration.php line 13:
                                                         
[Doctrine\Migrations\Exception\FrozenMigration]        
 The migration is frozen and cannot be edited anymore.  

This is related to this merge request: #1432

I might have missed this, but I expected that updating from doctrine/migrations 3.7.4 to 3.8.0 would not introduce any backward-incompatible changes.

In project, we don't have a direct dependency on doctrine/migrations, only on doctrine/doctrine-migrations-bundle. Could you please advise on how to properly version the doctrine/doctrine-migrations-bundle package to ensure that I don't get any backward-incompatible changes when running composer update?

Thank you for any response.

[derrabus]

cc @VincentLanglet @greg0ire

I'd like to replace the exception with a deprecation. Even if such calls don't make any sense, we shouldn't start throwing exceptions in a minor release on code that was "fine" for years.

[VincentLanglet]

We encountered the following issue - after the execution of composer update, some of our migrations stopped working correctly.

HI @ko4aisheretodev.
Do you have an example of the code written in your post method ?
I assume you have some addSql used in them ; you should know the SQL was never executed in such situation before, so
it's recommended to

• replacing the addSql call in postMethod
• or just removing them

I'd like to replace the exception with a deprecation. Even if such calls don't make any sense, we shouldn't start throwing exceptions in a minor release on code that was "fine" for years.

I understand your point, but I'm afraid people might miss the warning and the fact the code is not executed when running the migration.

I cannot look at the code now but maybe it's possible to

• introduce an internalMethod 'failOnFrozenMigration' on Migrations with false as default value
• introduce a similar option on migrationBundle config
• Throw an exception if failOnFrozenMigration = true, a warning if not.

This way:

• it's BC
• people can already get the exception behavior with a toggle on migrationBundle config

WDYT ?

[derrabus]

I understand your point, but I'm afraid people might miss the warning and the fact the code is not executed when running the migration.

They certainly will. But it's not our job to lecture them by making hundreds of migrations fail after a minor upgrade. Those might have been written years ago by devs that have left the company since then.

I don't want people to be scared of upgrading to a new minor.

• Throw an exception if failOnFrozenMigration = true, a warning if not.

Sure. Make that exception opt-in and I'm good. But it can't be the default behavior in a 3.x release.

[ko4an4ik]

Do you have an example of the code written in your post method ?

Yea, sure, minimal example is

final class Version20201231231231 extends AbstractMigration
{
    public function preUp(Schema $schema): void
    {
        parent::preUp($schema);
        $this->addSql('SET foreign_key_checks = 0;');
    }

    public function postUp(Schema $schema): void
    {
        parent::postUp($schema);
        $this->addSql('SET foreign_key_checks = 1;');
    }

    public function up(Schema $schema): void
    {
        $table = $schema->getTable('tbl');
        $table->addForeignKeyConstraint('tbl2', ['field_id'], ['id'], [], 'a_b');
    }

    public function down(Schema $schema): void
    {
        $table = $schema->getTable('tbl');
        $table->removeForeignKey('a_b');
    }

    public function isTransactional(): bool
    {
        return false;
    }
}

Also see that SET foreign_key_checks = 1 is not executed, but from your response, I understand that this is expected behavior (though it might not be obvious).

[VincentLanglet]

Also see that SET foreign_key_checks = 1 is not executed, but from your response, I understand that this is expected behavior (though it might not be obvious).

I agree it's not obvious. I got the issue recently that's why I opened the issue #1431.
This behavior already exist prior to 3.8.0, the difference now was that you got an error to avoid you having a bug/mistake.

Be aware that with such migration you never set foreign_key_checks back indeed.
You could change to $this->connection->executeStatement() or move the addSql in the up method.

Sure. Make that exception opt-in and I'm good. But it can't be the default behavior in a 3.x release.

I'll try to take a look bout this in the week. @derrabus (I understand this might be too long)

[VincentLanglet]

Maybe something like this 3.8.x...VincentLanglet:migrations:warning-instead @derrabus @greg0ire WDYT ?

And then the DoctrineMigrationBundle would add a addMethodCall in the DependencyFactory definition (?)

[javiereguiluz]

I recently faced this exception. In case it's useful for folks reading this, the following change fixed it:

    public function postUp(Schema $schema): void
    {
        // ...

-        $this->addSql('ALTER TABLE ... SET NOT NULL');
+        $this->connection->executeStatement('ALTER TABLE ... SET NOT NULL');
    }

[VincentLanglet]

I recently faced this exception. In case it's useful for folks reading this, the following change fixed it:

    public function postUp(Schema $schema): void
    {
        // ...

-        $this->addSql('ALTER TABLE ... SET NOT NULL');
+        $this->connection->executeStatement('ALTER TABLE ... SET NOT NULL');
    }

You have to know that with $this->addSql the 'ALTER TABLE ... SET NOT NULL' wasn't executed. I feel like doctrine made a good catch here.

Didn't find new time to work on 3.8.x...VincentLanglet:migrations:warning-instead though