Sentry bundle breaks doctrine migrations.

[vuryss]

Environment

PHP 8.0
Symfony 5.2
Doctrine Migrations 3.1
sentry-symfony 4.1

Steps to Reproduce

Having the above combination completely breaks the Doctrine migrations unless we add to each migrations a method indicating that the migration is not transactional.
Doctrine had this issue before (doctrine/migrations#1104) but they fixed it with this check:

https://github.com/doctrine/migrations/blob/96c64fa17477e503da8245f14541d70d7ae76091/lib/Doctrine/Migrations/Tools/TransactionHelper.php#L39

However, sentry decorates the connection with:

Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingDriverConnection

which is obviously not a PDO instance, so the aforementioned check fails and we get a nasty error:

In TracingDriverConnection.php line 163:
                                  
  [PDOException]                  
  There is no active transaction  

Expected Result

Well obviously I expect migrations to work with and without the sentry bundle :)

Actual Result

In TracingDriverConnection.php line 163:
                                  
  [PDOException]                  
  There is no active transaction  

[ste93cry]

I think there is nothing we can do here for the moment, since the check done in the Doctrine repo is clearly intended to work only for PDO-based drivers. @greg0ire is there any reason for that check to be like that? Maybe we could make the inTransaction() method part of the Doctrine\DBAL\Driver\Connection interface so that the check in TransactionHelper does not have to care whether or not the connection is a PDO connection, but it just has to ask whether the transaction is active or not. Each driver connection can then report back either true or false based on specific conditions. Otherwise, any attempt to wrap the driver's connection will cause the same issue we're having here

[greg0ire]

I think the reason was we needed a fix that would not require bumping our dependency on the DBAL, but you are right about inTransaction() possibly being the right API: doctrine/dbal#4544 (comment)

[ste93cry]

Understood. Do you think it would be feasible to implement this API in both version 2.x and 3.x? And do you think there would be anyone in the Doctrine team interested in doing this?

[greg0ire]

Do you think it would be feasible to implement this API in both version 2.x and 3.x?

Feasible, sure, but I think you and I both know it will not be accepted: doctrine/dbal#4495

And do you think there would be anyone in the Doctrine team interested in doing this?

Maybe Sergei will? I will open a new issue so that we can investigate this is indeed doable.

[ste93cry]

Feasible, sure, but I think you and I both know it will not be accepted

I completely forgot we already had this discussion for another thing 😅 I'm not sure what falls under the "upgrade path" category mentioned in that PR, but I see unfeasible to stick with that reason until the ORM does not support DBAL 3.x. If the feature is not backported to the DBAL 2.x, I hope we could find a (dirty?) way to support it in the Doctrine Migrations package

[greg0ire]

Issue opened at doctrine/dbal#4616

but I see unfeasible to stick with that reason until the ORM does not support DBAL 3.x

let's see how things look once it's implemented in 3.x? It will still probably be useful in the future.

[ste93cry]

Yea, having it in 3.x would already be a great starting point. However, our bundle and any other code that wraps the driver connection is not working properly with doctrine/migrations as of now and since I bet it will take a bit until things get implemented in the DBAL maybe we could implement a workaround until that time?

[greg0ire]

I suppose you could copy/paste TransactionHelper in this bundle?

UPD: ah nevermind, that won't help

[greg0ire]

Wait actually it will? You could use your copy of that helper here:

sentry-symfony/src/Tracing/Doctrine/DBAL/TracingDriverConnection.php

Line 153 in 107f60c

return $this->decoratedConnection->commit();

migrations calls its TransactionHelper, which detects this is not a PDOConnection, calls, TracingDriverConnection::commit(), which calls your copy of TransactionHelper to figure out what to do. Sounds good?

[ste93cry]

Yes, however I'm wondering if it would be safe to do such change because we would change the behaviour of the commit() method of the connection, wouldn't we?

[greg0ire]

Ah right you want to do that only in the context of migrations somehow, is that the issue? You could do that by inspecting the stack trace but ewww… anyway, even if you had inTransaction at your disposal, it would change that particular issue, would it?

[ste93cry]

Ah right you want to do that only in the context of migrations somehow, is that the issue?

Yes. In reality, the only thing we care about is that with or without this bundle things must work the same, of course. Instrumentation of the DBAL for tracing purposes should be transparent to the user and should not affect the behaviour of the application. The issue at the moment appears in the context of migrations, but to be honest I didn't test what happens if I try to execute a DDL SQL statement using the DBAL directly. My concern is that while the migrations, thanks to the usage of that helper, silently "swallow the error" (or don't make it happen), the DBAL instead would throw the exception due to the fact that there is no similar mechanism in that package. As a consequence of this, we have two distinct behaviour, and if I use the helper inside the driver connection then I'm changing the behaviour not only in the context of migrations but also of all the rest of the usages of the DBAL. Does it makes sense?

You could do that by inspecting the stack trace but ewww…

Yes, that's a possibility, although since the driver connection is a critical code path I think it's better to not do these kind of operations there that are costly and may affect performances more than what we already do for logging the data for the distributed tracing feature.

even if you had inTransaction at your disposal, it would change that particular issue, would it

What I was thinking is that in the doctrine/migrations package the condition could be changed to check whether the object instance implements the inTransaction method. That would basically be a poor-man version of the check based on the interface, until we have it at least, and would allow us here to add such method to the connection class which in turn will be used to determine whether the transaction is active. If in the future the DBAL will formalize the need for this method then such method would then be officially used not only in the context of the migrations but also from the DBAL itself and this fact would be completely independent from the usage of this bundle

[greg0ire]

What I was thinking is that in the doctrine/migrations package the condition could be changed to check whether the object instance implements the inTransaction method.

Sounds good. Can you send a PR?