Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Database Migration does set current version in DB #98

Open
JohannesTyra opened this issue Feb 21, 2023 · 6 comments · May be fixed by #103 or #139
Open

Database Migration does set current version in DB #98

JohannesTyra opened this issue Feb 21, 2023 · 6 comments · May be fixed by #103 or #139

Comments

@JohannesTyra
Copy link

If u run php symfony doctrine:migrate the tash will so the migration_version table to the current version.

That does not happen anymore.
The value (version) does not change.

Its in the Doctrine/Migration.php: https://github.com/FriendsOfSymfony1/doctrine1/blob/master/lib/Doctrine/Migration.php#L350

                $this->_connection->commit();
                $this->setCurrentVersion($to);

Does not work anymore.

I changed it to:

              $this->setCurrentVersion($to);
              $this->_connection->commit();
                

That works!

@JohannesTyra
Copy link
Author

After $this->_connection->commit(); the method $this->setCurrentVersion($to); will be not be executed.

@JohannesTyra
Copy link
Author

Exec throws an error $this->_connection->commit();

>> doctrine  Migrating from version 113 to 114
>> doctrine  executing migration : 114, class: Version114


PDOException: There is no active transaction in vendor/friendsofsymfony1/doctrine1/lib/Doctrine/Transaction.php:417
Stack trace:
#0 vendor/friendsofsymfony1/doctrine1/lib/Doctrine/Transaction.php(417): PDO->commit()
#1 vendor/friendsofsymfony1/doctrine1/lib/Doctrine/Transaction.php(279): Doctrine_Transaction->_doCommit()
#2 vendor/friendsofsymfony1/doctrine1/lib/Doctrine/Connection.php(1450): Doctrine_Transaction->commit()
#3 vendor/friendsofsymfony1/doctrine1/lib/Doctrine/Migration.php(351): Doctrine_Connection->commit()

@JohannesTyra
Copy link
Author

That works:


                try {
                  $this->_connection->commit();
                } catch (Exception $e){
                  echo $e;
                }
                
                $this->setCurrentVersion($to);

@thePanz
Copy link
Member

thePanz commented Feb 21, 2023

That works:

>                 try {
                  $this->_connection->commit();
                } catch (Exception $e){
                  echo $e;
                }
                
                $this->setCurrentVersion($to);

That would set the new version even if the migration failed I guess 🤔

@JohannesTyra
Copy link
Author

JohannesTyra commented Feb 22, 2023

@thePanz It was just a quick fix to get it work ;)

The question is: Why this happens: "PDOException: There is no active transaction in xxx".
What change causes this?

@agaluf
Copy link

agaluf commented Dec 14, 2023

This error happens in PHP 8+. See Implicit Commits in PHP8.

The problem happens in Migration.php, method "migrate()" (rows 319 to 355). Specifically, row 319 starts a transaction, then does migrations by calling $this->_doMigrate($to) in row 328. However, since this calls ALTER TABLE in each migration, the transaction is closed, which means that by the time our code reaches row 350 to $this->_connection->commit(), we no longer have a transaction to commit and our code explodes, never setting the version in the database.

There are several solutions to this problem, depending on how one wants to approach it, but I believe it would be the cleanest to check if a transaction exists after migration and if not, to start a new one, essentially replicating what happens implicitly in PHP 5-7. For example:

diff --git a/lib/Doctrine/Migration.php b/lib/Doctrine/Migration.php
index 615c792ec..1d8b0c742 100644
--- a/lib/Doctrine/Migration.php
+++ b/lib/Doctrine/Migration.php
@@ -326,6 +326,9 @@ class Doctrine_Migration
             }
 
             $this->_doMigrate($to);
+            if (!$this->_connection->getDbh()->inTransaction()) {
+                $this->_connection->beginTransaction();
+            }
         } catch (Exception $e) {
             $this->addError($e);
         }

Note that this means we could perform one migration successfully, fail the second one and be stuck with an incorrect migration version. However, this is already the case, as PHP 5-7 already do this. The correct approach would be to alter the tables in a migration, then update the version, then start the next migration, which would require a refactoring of this method.

Side note, this bug may most obviously appear in migrations, but could also happen otherwise, for example if our code calls to create a new table in the database on the fly.

thePanz added a commit that referenced this issue Jun 27, 2024
thePanz added a commit that referenced this issue Jun 27, 2024
thePanz added a commit that referenced this issue Jun 27, 2024
thePanz added a commit that referenced this issue Jun 27, 2024
thePanz added a commit that referenced this issue Jun 27, 2024
thePanz added a commit that referenced this issue Jun 27, 2024
thePanz added a commit that referenced this issue Jun 27, 2024
thePanz added a commit that referenced this issue Jun 27, 2024
thePanz added a commit that referenced this issue Jun 27, 2024
thePanz added a commit that referenced this issue Jun 27, 2024
thePanz added a commit that referenced this issue Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants