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

[3.0] Rework driver exceptions #3367

Closed
wants to merge 60 commits into from

Conversation

BenMorel
Copy link
Contributor

@BenMorel BenMorel commented Nov 29, 2018

Q A
Type improvement
BC Break yes
Fixed issues N/A

Summary

Consumers of the Driver\Connection API are not interested in catching individual exceptions implementing Driver\DriverException, and will always catch (DriverException).

As such, the multiple implementations of DriverException provide little to no value, clutter the codebase, and force introducing a new exception class or using new class whenever we need to throw a DriverException from a new place.

This design was probably chosen for 2 reasons:

  • a historical reason: Driver\PDOConnection originally extended \PDO, and as such Driver\PDOException extended \PDOException. To implement an interface common to all drivers, it was chosen to create an interface DriverException, that Driver\PDOException would implement, while still extending \PDOException. This requirement is gone, as the DBAL is evolving away from extending PDO, towards encapsulation.
  • a practical reason: custom exceptions allow for custom factory methods; these can easily be replaced with static methods on the Connection classes.

Proposed changes

  • AbstractDriverException is now a concrete DriverException, replacing the interface
  • All sub-exceptions are now gone
  • All exotic new class ... extends AbstractDriverException are now replaced with new DriverException
  • Factory methods from sub-exceptions are moved to a public static method in the Connection, that can be used from within the Connection itself and from the Statement
  • All relevant Connection methods are now explicitly documented as throwing DriverException

This last point is very important: so far, most of the methods had no documented exceptions, while query() and exec() were documented as throwing DBALException, a contract that was never respected:

  • PDOConnection::query() and exec() throw Driver\PDOException, which is a DriverException, not a DBALException
  • DB2Exception::query() and exec() throw DB2Exception, which is a DriverException
  • SQLSrvException::query() and exec() throw SQLSrvException (via SQLSrvStatement::execute()), which is a DriverException
  • ... and so on.

guilhermeblanco and others added 30 commits November 24, 2018 18:04
…having to replicate the \PDOStatement interface in ResultStatement
@morozov
Copy link
Member

morozov commented Dec 3, 2018

There is absolutely no conflict in #3335 with exceptions: it's just using the new class where a concrete DriverException class is not available.

@Majkl578 drives the rework of exceptions. As far as I understand, design-wise it's not what he had in mind.

@BenMorel
Copy link
Contributor Author

BenMorel commented Dec 4, 2018

@morozov #3335 does not make any changes to exceptions, on the contrary it tries to accommodate them as best as it can.

@Majkl578 Could you please review #3335, to see if it poses any problem to you?

@morozov morozov force-pushed the develop branch 2 times, most recently from 3850154 to 4fbe91a Compare February 22, 2019 00:01
@morozov morozov force-pushed the develop branch 3 times, most recently from a640b82 to e7b6c16 Compare March 18, 2019 06:13
@morozov morozov force-pushed the develop branch 3 times, most recently from 10890d6 to 3836750 Compare June 27, 2019 06:29
@morozov morozov closed this Nov 3, 2019
@morozov morozov reopened this Nov 4, 2019
@morozov morozov changed the base branch from develop to master November 4, 2019 14:39
@morozov
Copy link
Member

morozov commented Jul 7, 2020

@BenMorel I made some progress recently cleaning up driver exceptions in #4072, #4085, #4125, #4130, and other PRs (see the history of the 3.0.x branch).

Currently, some of the problems you were trying to solve are solved but not everything is changed the way you planned to. Do you want to try and rebase your PR onto 3.0.x and see if any changes are still relevant?

@BenMorel
Copy link
Contributor Author

@morozov I’m on vacation right now, I’ll try to look into this as soon as I get back!

@BenMorel
Copy link
Contributor Author

BenMorel commented Oct 4, 2020

Looking at 3.0.x and master, exceptions look in good shape and the project now looks much closer to what I had in mind.

I'm therefore closing this PR, and would rather open a new one if I were to suggest other changes in the future.

Thank you.

@BenMorel BenMorel closed this Oct 4, 2020
@morozov
Copy link
Member

morozov commented Oct 4, 2020

Sounds good. Thanks for following up, @BenMorel.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants