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

Look harder for a native connection #1210

Merged
merged 2 commits into from
Nov 12, 2021

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Nov 11, 2021

doctrine/dbal 3 switches from inheritance to composition but still
provides an accessor for PDO, and it is named like the method in
Connection.
Unwrapping until that method no longer exists should give the innermost
connection regardless of the DBAL version

I hade to adapt some tests to ensure getWrappedConnection returns an object.

Fixes #1209

How can I test this?

composer config repositories.greg0ire vcs https://github.com/greg0ire/migrations
composer require doctrine/migrations "dev-fix-compat-with-dbal-3 as 3.3.0"

@greg0ire greg0ire added the Bug label Nov 11, 2021
@greg0ire greg0ire force-pushed the fix-compat-with-dbal-3 branch from 4c4a3bc to 5c0e06c Compare November 11, 2021 15:51
It is not like this could work from another directory than the root of
the project.
@greg0ire greg0ire force-pushed the fix-compat-with-dbal-3 branch from 5c0e06c to 1e98776 Compare November 11, 2021 15:56
@derrabus
Copy link
Member

derrabus commented Nov 11, 2021

Tagging @morozov because we currently have a discussion about deprecating the methods you use to access the PDO connection, see doctrine/dbal#4966

@greg0ire
Copy link
Member Author

I think in previous discussions, we established that this TransactionHelper was fragile, hence why I deprecated it, so IMO this is in favor of deprecating getting the wrapped connection 😛

@greg0ire greg0ire force-pushed the fix-compat-with-dbal-3 branch 3 times, most recently from a9bba0b to f9d5556 Compare November 11, 2021 16:17
doctrine/dbal 3 switches from inheritance to composition but still
provides an accessor for PDO, and it is named like the method in
Connection.
Unwrapping until that method no longer exists should give the innermost
connection regardless of the DBAL version

Fixes doctrine#1209
@greg0ire greg0ire force-pushed the fix-compat-with-dbal-3 branch from f9d5556 to c9e0386 Compare November 11, 2021 16:18
@derrabus
Copy link
Member

Okay, in other words, this class will be broken when DBAL 4 is used eventually and that won't be a problem? In that case, the change looks good. 🙂

@greg0ire
Copy link
Member Author

Okay, in other words, this class will be broken when DBAL 4 is used eventually and that won't be a problem?

Yes. Maybe we should create doctrine/migrations 4.0.x and drop TransactionHelper to clarify that, WDYT @goetas ?


/* Attempt to commit or rollback while no transaction is running
results in an exception since PHP 8 + pdo_mysql combination */
return ! $wrappedConnection instanceof PDO || $wrappedConnection->inTransaction();
return ! $innermostConnection instanceof PDO || $innermostConnection->inTransaction();
Copy link
Member

Choose a reason for hiding this comment

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

If the "in transaction" method for the drivers of the platform that don't support transactional DDL is still of interest, it looks like the right course of action is to revive the discussion in PHP internals.

Copy link
Member Author

Choose a reason for hiding this comment

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

We are planning to drop the TransactionHelper class in the next major release (ref), so I don't think it will be of interest.

@UlrichThomasGabor
Copy link

fixes #1209

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exception on wrong transactional logic hides underlying exception
4 participants