Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate silencing transaction-related errors #1170

Merged
merged 1 commit into from
Jun 19, 2021

Conversation

greg0ire
Copy link
Member

Q A
Type improvement
BC Break no
Fixed issues #1169

Summary

Some platforms do not support transactions for DDL statements, and
doctrine/migrations historically swipes the issue under the rug by not
letting uninformed users know about it. Since PHP 8, we even silence the
issue suddenly made visible by PDO. Let's stop doing that.

MySQL users should typically set transactional to false in their
configuration and manually edit the isTransactional method override
generated in their migrations to return true (or drop it entirely) if
they run DDL migrations and want those to be transactional.

@greg0ire greg0ire force-pushed the deprecate-silencing branch from 6d23827 to d53a6ef Compare June 19, 2021 13:35
Some platforms do not support transactions for DDL statements, and
doctrine/migrations historically swipes the issue under the rug by not
letting uninformed users know about it. Since PHP 8, we even silence the
issue suddenly made visible by PDO. Let's stop doing that.

MySQL users should typically set transactional to false in their
configuration and manually edit the isTransactional method override
generated in their migrations to return true (or drop it entirely) if
they run DDL migrations and want those to be transactional.
@greg0ire greg0ire force-pushed the deprecate-silencing branch from d53a6ef to 38c7f52 Compare June 19, 2021 13:38
@@ -15,6 +16,18 @@ final class TransactionHelper
public static function commitIfInTransaction(Connection $connection): void
{
if (! self::inTransaction($connection)) {
Deprecation::trigger(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be nice to know which migration triggered the error... but i guess that here there is not enough context to know that... :(

@greg0ire greg0ire added this to the 3.2.0 milestone Jun 19, 2021
@greg0ire greg0ire merged commit 8828026 into doctrine:3.2.x Jun 19, 2021
@greg0ire greg0ire deleted the deprecate-silencing branch June 19, 2021 14:33
@goetas goetas mentioned this pull request Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants