search

doctrine / orm

Doctrine Object Relational Mapper (ORM)
https://www.doctrine-project.org/projects/orm.html
MIT License
9.86k stars 2.5k forks source link

Original exception is swallowed due rollback exception #11423

Closed ro0NL closed 2 months ago

ro0NL commented 2 months ago

Bug Report

Q A
BC Break no
Version 2.19

Summary

When rollback fails (eg. due SAVEPOINT DOCTRINE_2 does not exist) at https://github.com/doctrine/orm/blob/bdc41e2b5ea3020cf4660a12b5a985b94fbcd042/src/UnitOfWork.php#L485 the original exception is swallowed, which breaks debugging.

Current behavior

Swallowed exception

How to reproduce

Expected behavior

Try/catch rollback and log its exception, so the original exception is actually thrown.

greg0ire commented 2 months ago

log its exception

What if the logging fails? :stuck_out_tongue:

Right now there isn't a logger available in the Unit of Work, so I'm not sure how we could go about this…

ro0NL commented 2 months ago

i think this is related to #7545 in a way, but i dont know for sure, because the original exception is swallowed

regardless, the original exception should not be swallowed

depending on a nullable PSR logger contract seems fine?

try/catching log failures is an implementation detail per logger, we should consider it safe-calls

greg0ire commented 2 months ago

I had 2 unworded thoughts I saw mentioned by Ocramius in #7545:

Reading the other threads, I see that deferred constraints are involved, is that your case as well?

ro0NL commented 2 months ago

from https://github.com/doctrine/dbal/issues/3423

we lose information about what error happened in the first place

this is the bug

ro0NL commented 2 months ago

I see that https://github.com/doctrine/dbal/issues/3423 are involved, is that your case as well?

i dont know, because the original exception is swallowed

we could throw an exception that mentions both exceptions

anything goes :) as long as we expose the original exception

the rollback failing might be much worse and should have priority over the other exception

i dont know, because the original exception is swallowed

this could be some post-flush event exception

greg0ire commented 2 months ago

After following the rabbit hole, I think the current work on what I think might be the root cause is #4846

i dont know, because the original exception is swallowed

Are you using Postgres?

anything goes :) as long as we expose the original exception

Sound good, feel free to send a PR.

ro0NL commented 2 months ago

is it considered a bugfix for 2.19?

greg0ire commented 2 months ago

Hmmm… I guess it could be seen as a DX improvement, but one that could help us understand bugs in Doctrine. Why do you ask? Are you stuck on 2.x for now?

ro0NL commented 2 months ago

Are you stuck on 2.x for now?

yup

ro0NL commented 2 months ago

i believe it should be done like this https://github.com/etrias-nl/php-toolkit/blob/94a6486ab2dab405201751affc23bdb95fce422d/src/Messenger/Middleware/DoctrineTransactionMiddleware.php#L58-L74

i hope someone takes care of it on 2.x

ro0NL commented 2 months ago

closing in favor of #7545, which has the bug label