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

Rollback if and only if inside a transaction #1143

Merged
merged 2 commits into from
Apr 8, 2021

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Apr 3, 2021

Q A
Type bug
BC Break no
Fixed issues #1139

Summary

Similarly to what happens when attempting to commit an already closed
connection when combining PHP 8 and pdo_mysql, an error is thrown when
attempting to rollback. We use the same solution as for commit(), that
is we check that we actually are inside a transaction when possible to do so.

Fixes #1139

How can I test this?

composer config repositories.greg0ire vcs https://github.com/greg0ire/doctrine-migrations
composer require doctrine/migrations "dev-rollback-iff-in-transaction as 2.3.3"

@greg0ire greg0ire added the Bug label Apr 3, 2021
@greg0ire greg0ire requested a review from ostrolucky April 3, 2021 15:15
@greg0ire greg0ire marked this pull request as draft April 3, 2021 15:16
@greg0ire greg0ire removed the request for review from ostrolucky April 3, 2021 15:16
Similarly to what happens when attempting to commit an already closed
connection when combining PHP 8 and pdo_mysql, an error is thrown when
attempting to rollback. We use the same solution as for commit(), that
is we check that we actually are inside a transaction when possible to do so.

Fixes doctrine#1139
@greg0ire greg0ire force-pushed the rollback-iff-in-transaction branch from 67ea956 to 6d17f69 Compare April 3, 2021 15:18
You cannot get an integer from the CLI, so it makes no sense to use an
integer as the default option.
@greg0ire greg0ire marked this pull request as ready for review April 3, 2021 15:51
@greg0ire greg0ire requested a review from ostrolucky April 3, 2021 15:51
@greg0ire
Copy link
Member Author

greg0ire commented Apr 3, 2021

@AntoineRoue please review and test

@AntoineRoue
Copy link

@greg0ire I've just seen your message, is it ok or do you still need me to test ?

@greg0ire
Copy link
Member Author

greg0ire commented Apr 6, 2021

It's best if you test, if you can't we'll merge as is.

@AntoineRoue
Copy link

Ok no problem. I had issues with composer auth token yesterday but I think there is a recent update that fixes it. I'll try this evening.

@AntoineRoue
Copy link

AntoineRoue commented Apr 7, 2021

@greg0ire I managed to get your work, but when I run any doctrine command, I have the error
2021-04-07T19:12:59+00:00 [critical] Uncaught Error: Too few arguments to function Doctrine\Migrations\Configuration\Configuration::__construct(), 0 passed in var\cache\dev\ContainerF0kiXSY\getDoctrine_Migrations_DependencyFactoryService.php on line 23 and at least 1 expected exit status 255

@greg0ire
Copy link
Member Author

greg0ire commented Apr 7, 2021

I know that the last version of the migrations bundle is buggy, not sure if that is why you experience this. When you installed my fork, did any other package upgrade as well? In particular, did the migrations bundle upgrade?

@AntoineRoue
Copy link

AntoineRoue commented Apr 7, 2021

At first I could not install your package and the migration bundle package because of the dependency to "doctrine/migrations: ^3.1" in doctrine-migrations-bundle.
So I forked doctrine-migrations-bundle and created a branch from "3.1.x" to remove the dependency to "doctrine/migrations: ^3.1" (now that I think about it, maybe there is a composer option that ignores dependencies...)
Then I installed my 3.1.x no-dependency version of doctrine-migrations-bundle and your package. With this done, any doctrine command does not work.
EDIT : I just saw that somehow your branch rollback-iff... is 300 commits behind 3.1.x.

@greg0ire
Copy link
Member Author

greg0ire commented Apr 7, 2021

I just saw that somehow your branch rollback-iff... is 300 commits behind 3.1.x.

Yes, I'm targeting 2.3.x since it has the bug too

@greg0ire
Copy link
Member Author

greg0ire commented Apr 7, 2021

I just pushed a branch that is the result of the merge of 3.1.x and this one so that you can test.

composer config repositories.greg0ire vcs https://github.com/greg0ire/doctrine-migrations
composer require doctrine/migrations "dev-upmerge-rollback-iff-in-transaction as 3.1.1"

@AntoineRoue
Copy link

Oh I didn't realize that ! By the way, your first command should be composer config repositories.greg0ire vcs https://github.com/greg0ire/migrations.
I'll test this tomorrow evening.

@AntoineRoue
Copy link

Ok, we did it! I was able to test it, and your fix works.

@greg0ire greg0ire merged commit 5c7b119 into doctrine:2.3.x Apr 8, 2021
@greg0ire greg0ire deleted the rollback-iff-in-transaction branch April 8, 2021 19:57
@greg0ire
Copy link
Member Author

greg0ire commented Apr 8, 2021

Great, thanks @AntoineRoue , and good luck with that certification!

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

Successfully merging this pull request may close these issues.

Error "There is no active transaction" instead of Error about SQL syntax
4 participants