Do not use by default transactional DDL when the database engine does not support them

[goetas]

Q	A
Type	bug/feature/improvement
BC Break	yes/no
Fixed issues	#1104

revert #1131 and #1143

[greg0ire]

I think people reviewing this should read doctrine/dbal#4481 (comment) first.

I proposed this approach myself in #1104 (comment), so of course I approve 👍

Long term, I think the behavior of this method should be configurable so that users can choose to make it behave like before the change, after the change, or like return false.

@morozov @stof @ostrolucky please review

[greg0ire]

As an afterthought: should TransactionHelper and its usages be removed?

[goetas]

I have reverted #1143 as suggested by @greg0ire.

Should this change be done on the 2.3.x branch instead of 3.1.x?

[greg0ire]

I think you should revert #1131 too, and yes, I think we should target 2.3.x, because although it was reported on 3.1, It's probably not specific to it.

[goetas]

Base changed to 2.3.x

[greg0ire]

@ste93cry please also review :)

[morozov]

A few thoughts on the current state of the PR:

1. Can a migration contain a non-DDL statement? If yes, this is a breaking change: previously parts of the migration between DDL were transactional regardless of the platform, and now it depends on the platform.
2. The original issue states the problem as "DDL migrations produce errors". If except for the error the existing logic is OK, the fix should be about suppressing the error, not changing the logic.
3. The behavior of the program (whether it has to perform the changes in a transaction or not) should be defined in the program (by its author: via the generated code or the configuration of the code that generates it), not at runtime.
4. There's no integration test or user documentation that describes the new expected behavior. What should be the users' expectations be regarding this change?

[greg0ire]

@morozov I think you would prefer #1149 to this then? The original issue has been solved, but another one has cropped up where decoration makes the same issue come back because the instanceof PDO check does not work on a decorator.

[morozov]

That one doesn't seem to solve the problem with mysqli which doesn't implement inTransaction() and also won't work with DBAL 3 (as you mentioned). Catching the commit/rollback exception might be a simpler and better forward-compatible solution.

[greg0ire]

I don't think mysqli is actually affected: #1104 (comment) , as for DBAL 3, I said the current code will not work with it, the code proposed in #1149 could work if we end up implementing inTransaction in PDO\Connection

[morozov]

I don't think mysqli is actually affected

This is probably because up until doctrine/dbal#3480 (4.0.x) DBAL didn't consistently convert commit/rollback errors to exceptions.

[goetas]

I understand here the points by @morozov .

Can a migration contain a non-DDL statement? If yes, this is a breaking change: previously parts of the migration between DDL were transactional regardless of the platform, and now it depends on the platform.

The original issue states the problem as "DDL migrations produce errors". If except for the error the existing logic is OK, the fix should be about suppressing the error, not changing the logic.

Indeed, that would be a braking change. Another idea is then just to document it very well, with some big red alert. So we move the responsibility on the user.

I do not like the idea of suppressing errors...

The behavior of the program (whether it has to perform the changes in a transaction or not) should be defined in the program (by its author: via the generated code or the configuration of the code that generates it), not at runtime.

I did not understand this part....if the user overrides isTtransactional, isn't that defined in the program and not at runtime?

There's no integration test or user documentation that describes the new expected behavior. What should be the users' expectations be regarding this change?

when we agree on the solution, more documentation or tests can be added.

IMO #1131 and #1143 were merged too quickly.

[greg0ire]

Indeed, that would be a braking change. Another idea is then just to document it very well, with some big red alert. So we move the responsibility on the user.

Agree, that is part of the long term solution. The other part would be to give them an easy way to do so, because having to add

public function isTransactional(): bool
{
   return false;
}

to every migration is not very practical, unless maybe we automate it. I was proposing to complexify isTransactional() to rely on a configuration key, but that would have the same drawback as the code in this PR: it would be determined at runtime. So instead, we should rely on a configuration key to generate (or not generate) that override, as suggested by @morozov .

I did not understand this part....if the user overrides isTtransactional, isn't that defined in the program and not at runtime?

That's the thing: by default, generations are created without that override.

[greg0ire]

I probably should have done this sooner, but I created an RFC to centralize this discussion: #1153

[ste93cry]

@ste93cry please also review :)

For what's worth, I tested it and it looks like the issue is solved, but I still think that exposing the inTransaction() method from the DBAL is the best way and it would be helpful to users also outside the context of this specific issue

[ste93cry]

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

[NeilMasters]

I have to echo @ste93cry request. This is becoming a severe irritation.

[greg0ire]

@ste93cry I know I have positive reviews coming up for #1157 from @goetas when he finds some time

That PR contains an explanation that tells the user something you could repeat to yours if that PR does get merged indeed: using MySQL + transactions is risky, and was supported by accident up until now. End users should mark the migrations that use DDL as non-transactional by using a method override, which is not convenient right now but should become more easy in the future.

@NeilMasters you probably didn't mean harm with your comment, but be aware that it's quite close to spam, and sure isn't constructive.

[ostrolucky]

But what's the blocker to merge this PR? 🤔 I have an urge to just press the green button, any objections to that?

Dismissing my own review since I am no longer sure about this.

[greg0ire]

Another idea is then just to document it very well, with some big red alert. So we move the responsibility on the user.

I think we should go that way, that's why I created #1157 (in particular, the explanation part). I don't think we should add more band-aids over the band-aid, especially if we decide to remove them later. So the blocker would be a decision by @goetas on what the right course of action is.

[goetas]

#1157 looks better than this as it achieves something similar but being backward compatible.