Add test case for successful transactional DDL

[ostrolucky]

Q	A
Type	failing test case
BC Break	no
Fixed issues	-

Summary

Based on conversation at #4481 it's undesired that consumer should have capabilities to know if they can or cannot run transaction on given DDL. This commit demonstrates that this problem is present in DBAL too and it doesn't handle the case seamlessly. If consumer is not allowed to know if database allows transactional DDL, DBAL should handle attempts to execute DDL within transaction for databases not supporting transactional DDL seamlessly.

[morozov]

This test is incomplete. It performs a single atomic statement that doesn't need to be wrapped in a transaction. Please modify by adding another always failing statement (e.g. throw new Exception() after the foreach). Then define the expected behavior.

[ostrolucky]

I am not testing failure here, but success. MySQL commits operation successfully and despite that, DBAL throws error.

[morozov]

The successfull scenario doesn't need a transaction.

[ostrolucky]

Successful test scenario needs transaction if it exists to assert no exception has been thrown during successful transaction.

[morozov]

DDL statements are not transactional in MySQL and Oracle. No matter how much one wants them to be. If there are two DDLs in a transaction, and the second one fails, the first one will get committed no matter what API is used. I don't really understand this test.

[ostrolucky]

It's understandable that if there are two DDLs in a transaction, and the second one fails, the first one will get commited in MySQL. And in that case nobody would blame DBAL that it cannot do a rollback. But again, this test isn't about failure but success. If all operations in transaction() block succeed, there is no reason for DBAL to throw exception.

You are saying this doesn't need to be running in transaction for MySQL... But library using DBAL doesn't know what kind of statements user passes in, it will just always use transactions, because there is no point not to. I also need to remind this is not a MySQL test. This test is being executed for all RDBMses. So it does make sense for it to be always running in transaction, because transactional DDL is supported in some of them. Similar rational applies for using this in libraries.

Now, I was going for minimal test scenario, but if you seriously need hardcoded test case where transaction looks more useful even on MySQL, I could just change the queries in transaction to do some insert and then (successful) DDL. In such case when first (and only) DDL fails, changes would be rollbacked even in MySQL, so you would have to stop suggesting using transaction there doesn't make sense.

[morozov]

But again, this test isn't about failure but success.

This is fundamentally wrong. There's no point in running a transaction if you don't care about error handling. The whole point of a transaction is error handling (well, and isolation).

If all operations in transaction() block succeed, there is no reason for DBAL to throw exception.

The method assumes that it controls the transaction flow but it fails to. Hence the exception.

[morozov]

I believe the approach taken in doctrine/migrations#1131 is right if you don't care about the changes being really transactional. I didn't know about PDO::inTransaction() and came to learn about it while doing research. This might be the right API to implement in the DBAL. Although it may be not supported by all underlying drivers (e.g. mysqli).

[morozov]

The test failure on MySQL is expected because DBAL cannot guarantee the atomicity of the statements within the callback, and the exception clearly indicates that.

DBAL should handle attempts to execute DDL within transaction for databases not supporting transactional DDL seamlessly.

If it's not important whether the changes within the block are atomic or not (which is nonsence), transactional() can be reimplemented in the project (luckily, it's a few lines of code) and suppress the exception.