Skip to content

[Messenger] ensure exception on rollback does not hide previous exception #59103

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

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

nikophil
Copy link
Contributor

@nikophil nikophil commented Dec 5, 2024

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
License MIT

Hello,

this is kinda related to doctrine/orm#11230 and doctrine/orm#11646

I propose to use the same logic in DoctrineTransactionMiddleware than in doctrine/orm#11646 towards rollback: currently if an error occurs in the rollback (for example, the infamous SAVEPOINT DOCTRINE_2 does not exist), the previous error is hidden, because the error should not occur in the catch.

ping @ro0NL

EDIt: failure in CI is unrelated to this PR

Copy link
Member

@xabbuh xabbuh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fabpot
Copy link
Member

fabpot commented Dec 18, 2024

The changes done in Doctrine are slightly different, is it on purpose?

@nikophil nikophil force-pushed the fix/messenger-transaction branch 2 times, most recently from c8726c3 to 0c7ece5 Compare December 18, 2024 10:42
@nikophil nikophil force-pushed the fix/messenger-transaction branch from 0c7ece5 to f38d4b0 Compare December 18, 2024 10:42
@nikophil
Copy link
Contributor Author

nikophil commented Dec 18, 2024

@fabpot I've added the isTransactionActive(), as stated by @ro0NL

the other difference I can see with the fix in the ORM is that they close the entity manager, but I don't think we want to do this here.

@fabpot
Copy link
Member

fabpot commented Dec 18, 2024

Thank you @nikophil.

@fabpot fabpot merged commit c427887 into symfony:6.4 Dec 18, 2024
10 of 11 checks passed
This was referenced Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants