Migrations with custom transactions don't work

[maxm86545]

BC Break Report

Q	A
BC Break	yes
Version	3.5.3 from 3.5.2
DB	PostgreSQL 14.*

Summary

I have a working migration in version 3.5.2:

<?php

declare(strict_types=1);

namespace DoctrineMigrations;

use Doctrine\DBAL\Schema\Schema;
use Doctrine\Migrations\AbstractMigration;

final class Version20220912000000 extends AbstractMigration
{
    public function up(Schema $schema): void
    {
        $this->addSql('START TRANSACTION ISOLATION LEVEL SERIALIZABLE');
        $this->addSql('LOCK TABLE ...);
        // ...
        $this->addSql('COMMIT');
    }

    public function down(Schema $schema): void
    {
        $this->addSql('START TRANSACTION ISOLATION LEVEL SERIALIZABLE');
        $this->addSql('LOCK TABLE ....');
        // ...
        $this->addSql('COMMIT');
    }

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

Command: php bin/console doctrine:migrations:migrate --all-or-nothing

Previous behavior

The migration is successful with isTransactional() = false.

Current behavior

The migration is failed.

isTransactional() = false:

  Context: attempting to execute migrations with all-or-nothing enabled                                                   
  Problem: migration DoctrineMigrations\Version20220912000000 is marked as non-transactional                              
  Solution: disable all-or-nothing in configuration or by command-line option, or enable transactions for all migrations

isTransactional() = true:

SQLSTATE[25001]: Active sql transaction: 7 ERROR:  SET TRANSACTION ISOLATION LEVEL must be called before any query

Is caused by #1296

UPDATE. As a solution, remove START TRANSACTION and COMMIT from my migration, use isTransactional() = true.

[greg0ire]

3.5.3 from 3.5.2

3.5.3 is not the latest 3.5 version :)

Let me know if 3.5.5 doesn't work for you, I will reopen this.

[maxm86545]

@greg0ire Since 3.5.3. Actual for 3.5.5.

[greg0ire]

Reopening, I'm assuming your saying you're reproducing that issue on 3.5.5

[greg0ire]

cc @agustingomes

[agustingomes]

@maxm86545

The PR I worked on solved a discrepancy in the documentation, as the documentation stated that specifying --all-or-nothing wraps multiple migrations in a single transaction.

From the looks of your migration, seems you are explicitly controlling the transactions from the migrations, and therefore, using --all-or-nothing now sets the correct value.

My recommendation, since you are explicitly controlling the transactions, is to not use --all-or-nothing as its behavior before 3.5.3 was not consistent with the documentation when no value was specified.

@greg0ire does my assessment make sense to you?

[maxm86545]

I have several migrations, I want to apply all or none. Since v3.5.3, I can't do it. I understand that you made a fix, but you removed the feature that I used. There is a variant: remove START TRANSACTION and COMMIT from my migration, use isTransactional() = true. But I'm not sure that the behavior will remain the same.

[greg0ire]

@agustingomes Oh right I didn't notice that OP indeed used --all-or-nothing. With that in mind, I don't think v3.5.2 acted as you expected @maxm86545

If you have 3 migrations, and the 2nd one is the one with the custom transaction in it, I believed what happened was only the 2nd migration was actually transactional.

I want to apply all or none.

With 3.5.2, I think that wasn't actually happening. With 3.5.2, it looks like you would end up with nested transactions, and you can't even do it because marking a migration as non-transactional while using --all-or-nothing does not make any sense, as rightfully pointed out by the error message.

@agustingomes I notice the documentation still mentions 0 and 1. Can you please update it? See https://github.com/doctrine/migrations/blob/3.5.x/docs/en/reference/configuration.rst

[agustingomes]

@greg0ire I haven't changed the docs, as what is documented currently is possible for 3.5.x, but we do have the PR #1305 which deprecates passing the values to the --all-or-nothing option for 3.6.x. Perhaps adding a documentation change to let readers be aware of the deprecation is a good idea?

[greg0ire]

It would be: no developer should use this in new projects.

Here is an example of how we usually do this: https://raw.githubusercontent.com/doctrine/orm/2.14.x/docs/en/reference/yaml-mapping.rst

[greg0ire]

Also, it should be removed from 4.0.x, along with the corresponding code. I just merged 3.6.x into 4.0.x in case that was a blocker.