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

Slight behaviour change after #1131 #1142

Open
albe opened this issue Apr 3, 2021 · 3 comments
Open

Slight behaviour change after #1131 #1142

albe opened this issue Apr 3, 2021 · 3 comments

Comments

@albe
Copy link

albe commented Apr 3, 2021

Since the change of #1131 an odd behaviour change has occured to us (when switching from 3.1.0 to 3.1.1 without any further changes): Transaction commit failed because the transaction has been marked for rollback only.

Driver: PDO PGSQL

See
#1131 (comment)
and
neos/neos-development-collection#3311 (comment)

Summary:

  • when a migration that fails is executed before (auto-migrate) another code executes statements and tries to persist, the commit fails with above error message
  • this only happens with 3.1.1 and only for PDO PGSQL and any PHP version, though the bug that Stop throwing exceptions on commit in PHP8+pdo_mysql combo  #1131 fixes is only related to PDO MySQL+PHP8
  • hence as of 3.1.1 a failing migration leaves a (rollback) transaction open after invoking Migrator::migrate() (hence DbalMigrator::executeMigrations()), likely because an according commit() is now missing that would have been caught and rolled back
@albe
Copy link
Author

albe commented Apr 3, 2021

/cc @ostrolucky @greg0ire

Indeed a very edge-case (running auto-migrations is bad and we only did this for automated behaviour testing), and maybe you want to decide to keep it at this. But the change in behaviour is documented here now :)

@ostrolucky
Copy link
Member

It's not clear to me how to reproduce this

@albe
Copy link
Author

albe commented Apr 3, 2021

Technically roughly this (I don't know how that would/should look sanely in vanilla - just going into what we are doing beneath our abstraction layer(s)):
Run SchemaTool::createSchema() to setup your database with latest schema (but without marking migrations as having executed in the MetadataStorage - yes, this is wrong). Then in your (pseudo)code:

// build $planCalculator and $migrator instances with dependencies
...
try {
    // This will try to execute migrations that fail, because the schema is already up to date and e.g. `ALTER TABLE X CHANGE no_longer_existing_column_in_latest_schema ...` fails
    $migrator->migrate($planCalculator->getPlanUntilVersion('latest'), new MigratorConfiguration());
} catch (DBALException $exception) {
    $dontTruncate = true;
} catch (\PDOException $exception) {
}
// truncate all tables to empty them for next test run, but that shouldn't have an effect to the error case
...
$someEntityRepository->add($myNewTestEntity);
$entityManager->flush();

The important thing is that the migration attempt and the entityManager flush happen within one execution, i.e. there is a left-over transaction still open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants