MasterSlaveConnection:: 8000 query() throws driver-specific exceptions (e.g. PDOException etc) #4248
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0d9b977 to
bf5e969
Compare
bf5e969 to
84d9ffd
Compare
df7b408 to
fe7ef75
Compare
|
@beberlei, as the one who insisted on keeping this functionality in the library, please provide your review. |
|
Superseded by #4456 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes how driver exceptions are handled in Doctrine/DBAL/Connections/MasterSlaveConnection::query() and prevents driver-specific exceptions from bubbling. Driver exception conversion was added in 4cc70c1, but somehow
MasterSlaveConnectionwas forgotten.This PR reduces code duplication by calling the parent method, so no more chance of forgetting to apply some change to
MasterSlaveConnectionifDoctrine\DBAL\Connection::query()is modified. At the end of the day except for the proper exception handling parent's method behavior is the same.Alternative PRs
Unlike #3313, this PR addresses only the core issue - exception not being converted. That PR suggests catching
PDOExceptionin addition toDBALException, which is completely unnecessary if exception is properly converted. Also unlike #3313, this PR adds a localized unit test, while the other PR tests merely a side-effect on thepingmethod.BC Break
Any bug fix is a change of behavior, which can be a potential BC break for someone. Any of the 3rd party code that relies on current buggy behavior which in fact violates the contract would get broken by this change.
So in practice it's a BC break for some projects that probably tried to implement some workarounds for this bug. But ignoring exploitation of undocumented/buggy behavior when making stuff work according to the spec is a common thing when evaluating if a change breaks BC. The only exception that is supposed to be thrown out of the
querymethod isDBALExceptionas advertised in the docblock. Therefore I wouldn't consider it a BC break.Other
I can see 2.11.x branch has some significant changes around the code affected by this PR. I could create a separate PR for 2.11 if you consider this solution good enough.