Always use the inTransaction() method when possible in TransactionHelper

[ste93cry]

Q	A
Type	improvement (or bug, it depends on the POV)
BC Break	no
Fixed issues	-

Summary

As reported in getsentry/sentry-symfony#488, the check that has been introduced in #1104 and #1143 is assuming that if the driver connection is not an instance of PDO then the transaction should always be committed/rolled back. However, if the connection is decorated, the check will always fail and consequently the error that the check was meant to avoid will happen again. There has been a little discussion if the Doctrine\DBAL\Driver\Connection::inTransaction() method should be implemented so that it is the driver itself to define if a transaction is active and not the consumer and doctrine/dbal#4616 got opened. However, taking for granted that it will take some time to see it implemented, if it ever happens, this little workaround will solve the issue in the meanwhile.

[greg0ire]

Only now do I notice that while PDOConnection extends PDO, it is deprecated in favor of PDO\Connection which no longer does in DBAL 3. That means your fix is needed regardless, and that implementing inTransaction in PDO\Connection will be necessary.

[ste93cry]

True, but this lib does not support yet the newer version of the DBAL so for now this is a non-issue, and this code is future-proof if the method name is the one used in the implementation of doctrine/dbal#4616. A future iteration on this piece of code may of course change the check to use the more reliable instanceof TransactionAwareDriverConnection (or whatever name it will have)

[greg0ire]

Please have a look at the unit tests, it looks like there is insufficient mocking in some preexisting tests

[ostrolucky]

Sorry, personally I'm not approving this until some fix lands in DBAL. We don't even know what's the method going to be called there, if any. Feel free to overrule, I don't maintain doctrine/migrations.

[ste93cry]

We don't even know what's the method going to be called there, if any

I agree that the method_exists() check is quite weak and I would be more than happy to fix the DBAL, but I have the impression that it will take some time to see a release that fixes the issue and in the meanwhile downstream packages are having troubles due to the wrong assumption that the inTransaction() method exists only for drivers extending the PDO class

[ostrolucky]

Well instead of creating some imaginative method, having to contribute fix in doctrine/migrations and rely on duck typing, I would recommend to override commit function in your custom connection instead. There is also second workaround I posted last time, which is to switch from pdo_mysql to mysqli driver.

And I know downstream is struggling, that's why pressure needs to be put on DBAL to fix this correctly. We went with workaround last time I didn't like and see where it got us. None of these fixes are good without help from DBAL. You need this badly? Ok, then show that to DBAL team.

[ste93cry]

Well instead of creating some imaginative method

I'm not sure I get how this method may be imaginative, given that it's provided by the PDO class of PHP and that the class is basically a proxy to that one.

I would recommend to override commit function in your custom connection instead

That's something we though to do in getsentry/sentry-symfony#488 (comment), and the reason I'm afraid it won't work is explained in getsentry/sentry-symfony#488 (comment). TL;DR: I believe that the DBAL throws an exception if a DDL SQL statement is executed when a transaction is active, while this package silently avoid the error by checking the transaction status and avoiding to call commit() if it isn't active. Putting this check in the wrapped connection would mean that I cannot discern if I'm in the context of a migration or not without resorting to nasty hacks which are far worse than fixing the issue here for the migrations with the help of the DBAL

We went with workaround last time I didn't like and see where it got us

I don't know/remember what you're talking about, so I have no context to tell if that was a bad decision or not 😅

You need this badly? Ok, then show that to DBAL team.

I find useful to know if a transaction is currently active for the connection, and this has nothing to do with the DBAL. To be honest I don't see why you think it would not...

[ostrolucky]

I don't want this fix because every library which ships with own Connection class would have to add this method. Also, more libraries such as doctrine/migrations will pop up and you can't assume all of them will implement same duck typing check. Instead, DBAL should either handle this error silently/avoid it happenning or ship this method there. Then we could use it in doctrine/migrations and it would fix the problem for all of decorators.

I find useful to know if a transaction is currently active for the connection.

Cool, then ask DBAL to add it or ask pdo_mysql maintainers at bugs.php.net to stop throwing the error. I don't really understand why you think it has nothing to do with DBAL, but does with doctrine/migrations.

Putting this check in the wrapped connection would mean that I cannot discern if I'm in the context of a migration or not without resorting to nasty hacks which are far worse than fixing the issue here for the migrations with the help of the DBAL

You can inspect call stack via debug_backtrace, or catch. They are similarly simple solutions and I consider ugliness of them on similar level as patch in this PR.

[ste93cry]

Instead, DBAL should either handle this error silently/avoid it happenning or ship this method there. Then we could use it in doctrine/migrations and it would fix the problem for all of decorators.

I don't really understand why you think it has nothing to do with DBAL, but does with doctrine/migrations

I think there is a misunderstanding: I absolutely agree that the DBAL (see doctrine/dbal#4616) can help fixing the issue and I don't care if the DBAL silently swallows the error or do not call commit() on the driver connection if no transaction is active as long as it works as expected. If the DBAL provides the inTransaction() method in the DBAL I would happily use it here, as I said multiple times... The impression I had was that you didn't agree in fixing the issue in the DBAL in any way though, but maybe I'm wrong... On top of this, my thought was that duck typing was enough until the DBAL fixed the issue, one way or another: I never meant it to be a permanent solution

You can inspect call stack via debug_backtrace, or catch

Sure, but since the driver connection is a critical path and the issue originated while trying to make a performance tracing feature work, I want to avoid as much as possible to affect even more the performances in a bad way 😉

[ostrolucky]

Not sure what gave you impression I don't want fix in DBAL. It should absolutely be fixed in DBAL or php extension, that's why I'm sabotaging this PR. It wasn't fixed there so far because there neither was a contribution there, nor a pressure. If we do these half fixes elsewhere, there will not be pressure on fixing this in DBAL. Stop applying pressure in doctrine/migrations, this is wrong place. You have tools to solve the problem without this PR and if you think they are not perfect -> dbal.

[greg0ire]

I'm not 100% sure this should be fixed in the DBAL. Maybe tools can be added to help us, but maybe they cannot because they are impossible to implement with some drivers. I made a proposal for a long-term fix that would allow people to easily configure how they want their migrations to behave: #1104 (comment) , maybe I should create an RFC from that?

[ostrolucky]

There is isTransactionActive function in DBAL. IMHO it can (and should) be treated as a bug fix and this function should return false after implicit commit in pdo_mysql, because that's where we know for sure how to prove transaction is not actually active.

maybe they cannot because they are impossible to implement with some drivers

I don't see a blocker there. Leaky abstraction is nothing new in DBAL, see lot of notSupported exceptions all over the drivers. But I also don't see why not doing $pdo->inTransaction in pdo drivers and in rest just return $this->isTransactionActive(); (that's assuming responsibility of this being in isTransactionActive will be rejected)

Anyways this discussion from this point should be in DBAL. I'll leave somebody else to initiate discussion there though, my attempt failed in past.

[ste93cry]

Any news here? Issue is quite blocking for everyone that wraps the database platform, in my case getsentry/sentry-symfony

[ostrolucky]

You should ask here #1152

[ste93cry]

Since #1157 has been finally merged, I'm closing this PR because my solution is no longer needed 😃