From 3fbbca69f8ed66b03cc6862c98d5f267372eafb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Paris?= Date: Wed, 28 Apr 2021 18:39:46 +0200 Subject: [PATCH] Make transactions optional This introduces a new configuration key that allows the user to generate a method override that disables transaction. --- docs/en/explanation/implicit-commits.rst | 84 +++++++++++++++++++ docs/en/reference/configuration.rst | 14 ++++ docs/en/reference/migration-classes.rst | 3 +- .../Configuration/Configuration.php | 14 ++++ .../Migration/ConfigurationArray.php | 3 + .../Configuration/Migration/XmlFile.php | 7 ++ .../Migrations/Generator/Generator.php | 11 ++- .../Tests/Configuration/_files/config.json | 3 +- .../Tests/Configuration/_files/config.php | 1 + .../Tests/Configuration/_files/config.yml | 1 + .../Tests/Generator/GeneratorTest.php | 47 +++++++++++ 11 files changed, 185 insertions(+), 3 deletions(-) create mode 100644 docs/en/explanation/implicit-commits.rst diff --git a/docs/en/explanation/implicit-commits.rst b/docs/en/explanation/implicit-commits.rst new file mode 100644 index 0000000000..feca5a797c --- /dev/null +++ b/docs/en/explanation/implicit-commits.rst @@ -0,0 +1,84 @@ +Implicit commits +================ + +Since PHP8, if you are using some platforms with some drivers such as +MySQL with PDO, you may get an error that you did not get before when +using this library: ``There is no active transaction``. It comes from +the fact that some platforms like MySQL or Oracle do not support DDL +statements (``CREATE TABLE``, ``ALTER TABLE``, etc.) in transactions. + +The issue existed before PHP 8 but is now made visible by e.g. PDO, +which now produces the above error message when this library attempts to +commit a transaction that has already been commited before. + +Consider the following migration. + +.. code-block:: php + + public function up(Schema $schema): void + { + $users = [ + ['name' => 'mike', 'id' => 1], + ['name' => 'jwage', 'id' => 2], + ['name' => 'ocramius', 'id' => 3], + ]; + + foreach ($users as $user) { + $this->addSql('UPDATE user SET happy = true WHERE name = :name AND id = :id', $user); + } + + $this->addSql('CREATE TABLE example_table (id INT AUTO_INCREMENT NOT NULL, title VARCHAR(255) DEFAULT NULL, PRIMARY KEY(id))'); + } + +When you run that migration, what actually happens with some platforms +is you get the updates inside an implicitly commited transaction, then +the ``CREATE TABLE`` happens outside that transaction, and then there is +an attempt to commit an non-existent transaction. + +In that sort of situation, if you still wish to get the DML statements +inside a transaction, we recommend you split the migration in 2 +migrations, as follows. + +.. code-block:: php + + final class Version20210401193057 extends AbstractMigration + { + public function up(Schema $schema): void + { + $users = [ + ['name' => 'mike', 'id' => 1], + ['name' => 'jwage', 'id' => 2], + ['name' => 'ocramius', 'id' => 3], + ]; + + foreach ($users as $user) { + $this->addSql('UPDATE user SET happy = true WHERE name = :name AND id = :id', $user); + } + } + } + + final class Version20210401193058 extends AbstractMigration + { + public function up(Schema $schema): void + { + $this->addSql('CREATE TABLE example_table (id INT AUTO_INCREMENT NOT NULL, title VARCHAR(255) DEFAULT NULL, PRIMARY KEY(id))'); + } + + public function isTransactional(): bool + { + return false; + } + } + +Please refer to the manual of your database platform to know if you need +to do this or not. + +At the moment, this library checks if there is an active transaction +before commiting it, which means you should not encouter the error +described above. It will not be the case in the next major version +though, and you should prepare for that. + +To help you deal with this issue, the library features a configuration +key called ``transactional``. Setting it to ``false`` will cause new +migrations to be generated with the override method above, making new +migrations non-transactional by default. diff --git a/docs/en/reference/configuration.rst b/docs/en/reference/configuration.rst index 437095d98d..c6eb19e55f 100644 --- a/docs/en/reference/configuration.rst +++ b/docs/en/reference/configuration.rst @@ -38,6 +38,7 @@ Now, in the root of your project place a file named ``migrations.php``, ``migrat ], 'all_or_nothing' => true, + 'transactional' => true, 'check_database_platform' => true, 'organize_migrations' => 'none', 'connection' => null, @@ -58,6 +59,7 @@ Now, in the root of your project place a file named ``migrations.php``, ``migrat 'MyProject\Component\Migrations': ./Component/MyProject/Migrations all_or_nothing: true + transactional: true check_database_platform: true organize_migrations: none @@ -90,6 +92,7 @@ Now, in the root of your project place a file named ``migrations.php``, ``migrat true + true true none @@ -112,6 +115,7 @@ Now, in the root of your project place a file named ``migrations.php``, ``migrat }, "all_or_nothing": true, + "transactional": true, "check_database_platform": true, "organize_migrations": "none", @@ -136,6 +140,9 @@ Here are details about what each configuration option does: +----------------------------+------------+------------------------------+----------------------------------------------------------------------------------+ | all_or_nothing | no | false | Whether or not to wrap multiple migrations in a single transaction. | +----------------------------+------------+------------------------------+----------------------------------------------------------------------------------+ +| transactional | no | true | Whether or not to wrap migrations in a single transaction. | +| | | | | ++----------------------------+------------+------------------------------+----------------------------------------------------------------------------------+ | migrations | no | [] | Manually specify the array of migration versions instead of finding migrations. | +----------------------------+------------+------------------------------+----------------------------------------------------------------------------------+ | check_database_platform | no | true | Whether to add a database platform check at the beginning of the generated code. | @@ -226,6 +233,13 @@ All or Nothing Transaction When using the ``all_or_nothing`` option, multiple migrations ran at the same time will be wrapped in a single transaction. If one migration fails, all migrations will be rolled back +Using or not using transactions +------------------------------- + +By default, migrations are transactional, meaning code in a migration +is wrapped in a transaction. +Setting ``transactional`` to ``false`` will disable that. + From the Command Line ~~~~~~~~~~~~~~~~~~~~~ diff --git a/docs/en/reference/migration-classes.rst b/docs/en/reference/migration-classes.rst index 9ea2638821..707150a61d 100644 --- a/docs/en/reference/migration-classes.rst +++ b/docs/en/reference/migration-classes.rst @@ -75,7 +75,8 @@ Override this method if you want to disable transactions in a migration. It defa statement, and before running it. Make sure to read the manual of your database platform to know what is actually happening. ``isTransactional()`` does not guarantee that statements are wrapped - in a single transaction. + in a single transaction. To learn more about this, read the + :ref:`dedicated explanation <../explanation/implicit-commits>`. getDescription ~~~~~~~~~~~~~~ diff --git a/lib/Doctrine/Migrations/Configuration/Configuration.php b/lib/Doctrine/Migrations/Configuration/Configuration.php index 6bc46b4136..500d9b91db 100644 --- a/lib/Doctrine/Migrations/Configuration/Configuration.php +++ b/lib/Doctrine/Migrations/Configuration/Configuration.php @@ -41,6 +41,9 @@ final class Configuration /** @var bool */ private $allOrNothing = false; + /** @var bool */ + private $transactional = true; + /** @var string|null */ private $connectionName; @@ -193,6 +196,17 @@ public function isAllOrNothing(): bool return $this->allOrNothing; } + public function setTransactional(bool $transactional): void + { + $this->assertNotFrozen(); + $this->transactional = $transactional; + } + + public function isTransactional(): bool + { + return $this->transactional; + } + public function setCheckDatabasePlatform(bool $checkDbPlatform): void { $this->checkDbPlatform = $checkDbPlatform; diff --git a/lib/Doctrine/Migrations/Configuration/Migration/ConfigurationArray.php b/lib/Doctrine/Migrations/Configuration/Migration/ConfigurationArray.php index a7bc5e9f49..4c48f67235 100644 --- a/lib/Doctrine/Migrations/Configuration/Migration/ConfigurationArray.php +++ b/lib/Doctrine/Migrations/Configuration/Migration/ConfigurationArray.php @@ -61,6 +61,9 @@ public function getConfiguration(): Configuration 'all_or_nothing' => static function ($value, Configuration $configuration): void { $configuration->setAllOrNothing(is_bool($value) ? $value : BooleanStringFormatter::toBoolean($value, false)); }, + 'transactional' => static function ($value, Configuration $configuration): void { + $configuration->setAllOrNothing(is_bool($value) ? $value : BooleanStringFormatter::toBoolean($value, true)); + }, 'check_database_platform' => static function ($value, Configuration $configuration): void { $configuration->setCheckDatabasePlatform(is_bool($value) ? $value : BooleanStringFormatter::toBoolean($value, false)); }, diff --git a/lib/Doctrine/Migrations/Configuration/Migration/XmlFile.php b/lib/Doctrine/Migrations/Configuration/Migration/XmlFile.php index cb2b8458ef..89322a19d8 100644 --- a/lib/Doctrine/Migrations/Configuration/Migration/XmlFile.php +++ b/lib/Doctrine/Migrations/Configuration/Migration/XmlFile.php @@ -47,6 +47,13 @@ public function getConfiguration(): Configuration ); } + if (isset($config['transactional'])) { + $config['transactional'] = BooleanStringFormatter::toBoolean( + $config['transactional'], + true + ); + } + if (isset($config['migrations_paths'])) { $config['migrations_paths'] = $this->getDirectoriesRelativeToFile( $config['migrations_paths'], diff --git a/lib/Doctrine/Migrations/Generator/Generator.php b/lib/Doctrine/Migrations/Generator/Generator.php index dcccb338d8..e99805e705 100644 --- a/lib/Doctrine/Migrations/Generator/Generator.php +++ b/lib/Doctrine/Migrations/Generator/Generator.php @@ -58,7 +58,7 @@ public function down(Schema $schema): void { // this down() migration is auto-generated, please modify it to your needs - } + } } TEMPLATE; @@ -98,6 +98,15 @@ public function generateMigration( '' => $className, '' => $up !== null ? ' ' . implode("\n ", explode("\n", $up)) : null, '' => $down !== null ? ' ' . implode("\n ", explode("\n", $down)) : null, + '' => $this->configuration->isTransactional() ? '' : <<<'METHOD' + + + public function isTransactional(): bool + { + return false; + } +METHOD + , ]; $code = strtr($this->getTemplate(), $replacements); diff --git a/tests/Doctrine/Migrations/Tests/Configuration/_files/config.json b/tests/Doctrine/Migrations/Tests/Configuration/_files/config.json index 91adee5bf3..9ae197b51b 100644 --- a/tests/Doctrine/Migrations/Tests/Configuration/_files/config.json +++ b/tests/Doctrine/Migrations/Tests/Configuration/_files/config.json @@ -10,5 +10,6 @@ "executed_at_column_name" : "doctrine_migration_executed_at_column_test", "execution_time_column_name" : "doctrine_migration_execution_time_column_test" }, - "all_or_nothing" : true + "all_or_nothing" : true, + "transactional" : true } diff --git a/tests/Doctrine/Migrations/Tests/Configuration/_files/config.php b/tests/Doctrine/Migrations/Tests/Configuration/_files/config.php index 11f49dd536..40ff39fe5c 100644 --- a/tests/Doctrine/Migrations/Tests/Configuration/_files/config.php +++ b/tests/Doctrine/Migrations/Tests/Configuration/_files/config.php @@ -15,5 +15,6 @@ 'migrations' => ['Foo', 'Bar'], 'all_or_nothing' => true, + 'transactional' => true, 'check_database_platform' => false, ]; diff --git a/tests/Doctrine/Migrations/Tests/Configuration/_files/config.yml b/tests/Doctrine/Migrations/Tests/Configuration/_files/config.yml index 2781bb7dff..b1990061ec 100644 --- a/tests/Doctrine/Migrations/Tests/Configuration/_files/config.yml +++ b/tests/Doctrine/Migrations/Tests/Configuration/_files/config.yml @@ -10,5 +10,6 @@ migrations_paths: DoctrineMigrationsTest: . migrations : ["Foo", "Bar"] all_or_nothing: true +transactional: true check_database_platform: true diff --git a/tests/Doctrine/Migrations/Tests/Generator/GeneratorTest.php b/tests/Doctrine/Migrations/Tests/Generator/GeneratorTest.php index d0f7fcde51..19f7f19d08 100644 --- a/tests/Doctrine/Migrations/Tests/Generator/GeneratorTest.php +++ b/tests/Doctrine/Migrations/Tests/Generator/GeneratorTest.php @@ -4,10 +4,14 @@ namespace Doctrine\Migrations\Tests\Generator; +use Doctrine\DBAL\Connection; use Doctrine\Migrations\Configuration\Configuration; use Doctrine\Migrations\Generator\Generator; use InvalidArgumentException; use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; +use Test\VersionNonTransactional1234; +use Test\VersionNonTransactional1235; use function class_exists; use function file_get_contents; @@ -95,6 +99,49 @@ public function testCustomTemplateThrowsInvalidArgumentExceptionWhenTemplateEmpt unlink($customTemplate); } + public function testItCanGenerateNonTransactionalMigrations(): void + { + $this->configuration->setTransactional(false); + $path = $this->migrationGenerator->generateMigration( + 'Test\\VersionNonTransactional1234', + '// up', + '// down' + ); + + self::assertFileExists($path); + + include $path; + + self::assertTrue(class_exists('Test\VersionNonTransactional1234')); + self::assertFalse((new VersionNonTransactional1234( + $this->createStub(Connection::class), + $this->createStub(LoggerInterface::class) + ))->isTransactional()); + + unlink($path); + } + + public function testItCanGenerateTransactionalMigrationsByDefault(): void + { + $path = $this->migrationGenerator->generateMigration( + 'Test\\VersionNonTransactional1235', + '// up', + '// down' + ); + + self::assertFileExists($path); + + include $path; + + self::assertTrue(class_exists('Test\VersionNonTransactional1235')); + self::assertTrue((new VersionNonTransactional1235( + $this->createStub(Connection::class), + $this->createStub(LoggerInterface::class) + ))->isTransactional()); + + unlink($path); + } + protected function setUp(): void { $this->configuration = new Configuration();