Deprecate inconsistently and ambiguously named driver-level classes by morozov · Pull Request #4100 · doctrine/dbal

morozov · 2020-06-23T05:56:16Z

Q	A
Type	improvement
BC Break	no

Driver and driver-level Connection and Statement classes have been renamed for consistency.
The PDO-related classes have been additionally moved under the PDO namespace.
The Driver\DriverException class has been renamed to Driver\Exception. On the one hand, the class name duplicated the name of its namespace; on the other, there's another DriverException class in the codebase.
The Driver\AbstractDriverException class has been renamed to Driver\AbstractException and marked internal.
Abstract driver-level exception classes have been deprecated in favor of the Driver\Exception interface and named exception classes.

greg0ire · 2020-06-23T20:02:41Z

🤔 how are any of these items not BC-breaks? Do we consider all these classes and interfaces internal?

morozov · 2020-06-23T20:07:49Z

how are any of these items not BC-breaks? Do we consider all these classes and interfaces internal?

No, the original classes and interfaces remain in place but they are deprecated and extend their new versions. It's a good time to do the rename before most of these classes (or all by design) become final.

Maybe the upgrade notes and the changes don't convey this clearly enough.

greg0ire · 2020-06-23T20:19:19Z

lib/Doctrine/DBAL/Driver/AbstractMySQLDriver.php

     * @link https://dev.mysql.com/doc/refman/8.0/en/server-error-reference.html
     */
-    public function convertException($message, DriverException $exception)
+    public function convertException($message, Exception $exception)


Small BC-break here: classes extending this one will narrow the type, which breaks the LSP. I'm not saying it has to be a blocker, but maybe in these 103 files there are similar BC-breaks that are more serious, so I raise this in case you did not have that in mind.

~~If you think it is serious, maybe we could invert the extension, but then we would have the opposite issue with return type hints (we might have fewer of those though, I don't know)~~ NVM that would make the signature change a BC-break without even thinking about extending classes.

Slightly off-topic, have we ever considered using roave's BC-checker on this package?

Small BC-break here: classes extending this one will narrow the type, which breaks the LSP. I'm not saying it has to be a blocker, but maybe in these 103 files there are similar BC-breaks that are more serious, so I raise this in case you did not have that in mind.

This is a valid point, I didn't have that in mind. To be absolutely on the safe side, we may only introduce the new classes but not change any method signatures that accept them (we could change the return types but we probably don't have many of them in 2.11.x).

Slightly off-topic, have we ever considered using roave's BC-checker on this package?

It might be a good time to give it a try.

Is there still a point in introducing the new classes? Can they be used anywhere?

I have a solution to that kind of issue, and we went with it when migrating the Doctrine\Common\Persistence namespace to just Doctrine\Persistence, and it was painful (Andreas wrote several blog posts about it). We vowed never to do that again, so I'm not sure how this is going to pan out. Maybe you can find a subset of this PR that consists in classes never used in method signatures that can be overriden outside of the DBAL?

This is why I tend to final everything…

Most of these classes can be finalized (#3590, #3818) but not all of them. We indeed can/should mark all the new ones that can be marked final right now instead of waiting.

morozov · 2020-06-24T01:28:46Z

This is what the output of the BC tool looks like:

$ roave-backward-compatibility-check --from=upstream/2.11.x
[BC] SKIPPED: Roave\BetterReflection\Reflection\ReflectionClass "Symfony\Component\Console\Helper\Helper" could not be found in the located source
...
[BC] REMOVED: These ancestors of Doctrine\DBAL\Driver\IBMDB2\Exception\ConnectionFailed have been removed: ["Doctrine\\DBAL\\Driver\\IBMDB2\\DB2Exception","Doctrine\\DBAL\\Driver\\AbstractDriverException"]
[BC] CHANGED: Doctrine\DBAL\Driver\IBMDB2\Exception\ConnectionFailed was marked "@internal"
[BC] REMOVED: These ancestors of Doctrine\DBAL\Driver\IBMDB2\Exception\ConnectionError have been removed: ["Doctrine\\DBAL\\Driver\\IBMDB2\\DB2Exception","Doctrine\\DBAL\\Driver\\AbstractDriverException"]
[BC] CHANGED: Doctrine\DBAL\Driver\IBMDB2\Exception\ConnectionError was marked "@internal"
[BC] REMOVED: These ancestors of Doctrine\DBAL\Driver\IBMDB2\Exception\StatementError have been removed: ["Doctrine\\DBAL\\Driver\\IBMDB2\\DB2Exception","Doctrine\\DBAL\\Driver\\AbstractDriverException"]
[BC] CHANGED: Doctrine\DBAL\Driver\IBMDB2\Exception\StatementError was marked "@internal"
[BC] REMOVED: These ancestors of Doctrine\DBAL\Driver\IBMDB2\Exception\PrepareFailed have been removed: ["Doctrine\\DBAL\\Driver\\IBMDB2\\DB2Exception","Doctrine\\DBAL\\Driver\\AbstractDriverException"]
[BC] CHANGED: Doctrine\DBAL\Driver\IBMDB2\Exception\PrepareFailed was marked "@internal"

All the IBMDB2-related errors can be ignored since these classes were introduced in 2.11.x (#4085).

…bbuh) This PR was merged into the 4.4 branch. Discussion ---------- [Messenger] fix compatibility with Doctrine DBAL 3.0 | Q | A | ------------- | --- | Branch? | 5.0 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | see doctrine/dbal#4100 and doctrine/dbal#4125 Commits ------- b17df8c fix compatibility with Doctrine DBAL 3.0

stof · 2020-06-29T16:21:33Z

lib/Doctrine/DBAL/Driver/AbstractSQLServerDriver/Exception/PortWithoutHost.php

 * @psalm-immutable
 */
-final class PortWithoutHost extends AbstractDriverException
+final class PortWithoutHost extends AbstractException


this is a BC break, because catching AbstractDriverException will not catch this exception anymore (remember that using the exception classes is not only done in instantiations but also in catch clauses)

The class is marked as internal though.

@greg0ire is right. Consumers should be only catching Driver\DriverException.

Ah but AbstractDriverException itself is not marked as internal, so they might be, might they not?

I see. Let's revert this line (#4131). It looks like the only change that needs to be reverted. The rest of the exceptions changed in a similar way were introduced in 2.11.x.

So this might be something that the BackwardCompatibilityCheck doesn't yet detect: even if the class is internal if the new version is not a subtype of the old version, it's a BC break. Even if it's not an exception.

well, for internal classes which are not an exception, that's fine (as it is not covered by the BC promise). For exception, that's a bit more complex: due to the catching semantic, even an internal exception class must still stay a subtype of all its non-internal types of the old version.

How is catching an exception different from accepting an argument of a given type by a function?

hmm, indeed. So caring about non-internal parents of internal classes indeed applies to other cases too.

Reported as Roave/BackwardCompatibilityCheck#256.

stof · 2020-06-29T17:10:04Z

lib/Doctrine/DBAL/Driver/PDOException.php


 /**
- * Tiny wrapper for PDOException instances to implement the {@link DriverException} interface.
+ * @deprecated Use {@link Exception} instead


this is missing a use statement to make the Exception reference valid.

stof · 2020-06-29T17:11:25Z

lib/Doctrine/DBAL/Driver/PDOSqlsrv/Connection.php

 * Sqlsrv Connection implementation.
 */
-class Connection extends PDOConnection
+class Connection extends BaseStatement


This is a BaseConnection, not a BaseStatement

@gjdanis

Release [2.11.0](https://github.com/doctrine/dbal/milestone/77) 2.11.0 ====== - Total issues resolved: **7** - Total pull requests resolved: **55** - Total contributors: **8** Improvement,Prepared Statements,SQL Server,Types,pdo_sqlsrv,sqlsrv ------------------------------------------------------------------ - [4274: Support ASCII parameter binding](doctrine#4274) thanks to @gjdanis Documentation ------------- - [4271: Add explanation about implicit indexes](doctrine#4271) thanks to @greg0ire Deprecation,Error Handling -------------------------- - [4253: Deprecate DBAL\DBALException in favor of DBAL\Exception](doctrine#4253) thanks to @morozov CI -- - [4251: Setup automatic release workflow](doctrine#4251) thanks to @greg0ire Deprecation,Schema Managers --------------------------- - [4230: Deprecate the functionality of dropping client connections when dropping a database](doctrine#4230) thanks to @morozov Deprecation,Platforms --------------------- - [4229: Deprecate more AbstractPlatform methods](doctrine#4229) thanks to @morozov - [4132: Deprecate AbstractPlatform::fixSchemaElementName()](doctrine#4132) thanks to @morozov Improvement,Test Suite ---------------------- - [4215: Remove test group configuration leftovers](doctrine#4215) thanks to @morozov - [4080: Update PHPUnit to 9.2](doctrine#4080) thanks to @morozov - [4079: Forward compatibility with PHPUnit 9.3](doctrine#4079) thanks to @morozov - [3923: Removed performance tests](doctrine#3923) thanks to @morozov Deprecation,Schema ------------------ - [4213: Deprecate the Synchronizer package](doctrine#4213) thanks to @morozov Blocker,Improvement,PHP,Test Suite ---------------------------------- - [4201: Update PHPUnit to 9.3](doctrine#4201) thanks to @morozov Blocker,PHP,Test Suite ---------------------- - [4196: The test suite uses the deprecated at() matcher](doctrine#4196) thanks to @morozov Connections,Deprecation,Documentation ------------------------------------- - [4175: Additional deprecation note for PrimaryReplicaConnection::query()](doctrine#4175) thanks to @morozov Connections,Deprecation,Prepared Statements ------------------------------------------- - [4165: Deprecated usage of wrapper components as implementations of driver-level interfaces](doctrine#4165) thanks to @morozov - [4020: Deprecated Connection::project(), Statement::errorCode() and errorInfo()](doctrine#4020) thanks to @morozov Connections,Deprecation ----------------------- - [4163: Deprecate duplicate and ambiguous wrapper connection methods](doctrine#4163) thanks to @morozov Error Handling,Improvement,Types -------------------------------- - [4145: Add TypeRegistry constructor](doctrine#4145) thanks to @morozov Deprecation,Drivers,Improvement,pdo_mysql,pdo_oci,pdo_pgsql,pdo_sqlite,pdo_sqlsrv --------------------------------------------------------------------------------- - [4144: Deprecate classes in Driver\PDO* namespaces](doctrine#4144) thanks to @morozov Connections,Documentation,Improvement ------------------------------------- - [4139: Mark connection constructors internal](doctrine#4139) thanks to @morozov Deprecation,Drivers,Error Handling ---------------------------------- - [4137: Deprecate driver exception conversion APIs](doctrine#4137) thanks to @morozov - [4112: Deprecate DriverException::getErrorCode()](doctrine#4112) thanks to @morozov Configuration,Connections,Deprecation,Error Handling ---------------------------------------------------- - [4134: Deprecate some DBALException factory methods](doctrine#4134) thanks to @morozov Code Style,Documentation ------------------------ - [4133: Fix more issues introduced by the deprecation of driver classes](doctrine#4133) thanks to @morozov BC Break,Drivers,Error Handling,pdo_sqlsrv,sqlsrv ------------------------------------------------- - [4131: Restore the PortWithoutHost exception parent class](doctrine#4131) thanks to @morozov Code Style,Improvement,Static Analysis -------------------------------------- - [4123: Remove the no longer needed error suppressions](doctrine#4123) thanks to @morozov Deprecation,Drivers,Error Handling,Improvement ---------------------------------------------- - [4118: Deprecate ExceptionConverterDriver](doctrine#4118) thanks to @morozov Bug,Connections,Improvement,Prepared Statements ----------------------------------------------- - [4117: Fixes for the recently introduced driver-level deprecations](doctrine#4117) thanks to @morozov Connections,Deprecation,Platform Detection ------------------------------------------ - [4114: Deprecate ServerInfoAwareConnection#requiresQueryForServerVersion() as an implementation detail](doctrine#4114) thanks to @morozov Deprecation,Prepared Statements,SQL Parser,oci8 ----------------------------------------------- - [4110: Mark non-interface OCI8 driver methods internal](doctrine#4110) thanks to @morozov Connections,Deprecation,Drivers,Improvement,Prepared Statements --------------------------------------------------------------- - [4100: Deprecate inconsistently and ambiguously named driver-level classes](doctrine#4100) thanks to @morozov Connections,Improvement ----------------------- - [4092: Remove Connection::$isConnected](doctrine#4092) thanks to @morozov Configuration,Connections ------------------------- - [4086: Mark Connection::getParams() internal](doctrine#4086) thanks to @morozov Bug,Drivers,ibm_db2 ------------------- - [4085: The IBM DB2 driver Exception class must implement the DriverException interface](doctrine#4085) thanks to @morozov PHP --- - [4078: Bump PHP requirement to 7.3 as of DBAL 2.11.0](doctrine#4078) thanks to @morozov Connections,Databases,Deprecation,Drivers ----------------------------------------- - [4068: Deprecate Driver::getDatabase()](doctrine#4068) thanks to @morozov Deprecation,Improvement,Portability ----------------------------------- - [4061: Deprecated platform-specific portability mode constants](doctrine#4061) thanks to @morozov - [4054: &doctrine#91;doctrineGH-4052&doctrine#93; Deprecate MasterSlaveConnection and rename to PrimaryReplicaConnection](doctrine#4054) thanks to @beberlei and @albe - [4017: Improve help of dbal:run-sql command](doctrine#4017) thanks to @ostrolucky Code Style,Improvement ---------------------- - [4050: Update doctrine/coding-standard to 8.0](doctrine#4050) thanks to @morozov Cache,Deprecation,Improvement,Prepared Statements ------------------------------------------------- - [4049: Replace forward-compatible ResultStatement interfaces with Result](doctrine#4049) thanks to @morozov Cache,Improvement,Prepared Statements ------------------------------------- - [4048: Make caching layer not rely on closeCursor()](doctrine#4048) thanks to @morozov Deprecation,Improvement,Prepared Statements ------------------------------------------- - [4037: Introduce Statement::fetchFirstColumn()](doctrine#4037) thanks to @morozov - [4019: Deprecated the concept of the fetch mode](doctrine#4019) thanks to @morozov Bug,Documentation,Improvement,Prepared Statements ------------------------------------------------- - [4034: Additional changes based on the discussion in doctrine#4007](doctrine#4034) thanks to @morozov Connections,Console,Improvement ------------------------------- - [3956: allow using multiple connections for CLI commands](doctrine#3956) thanks to @dmaicher Deprecation,Logging ------------------- - [3935: Deprecate EchoSQLLogger](doctrine#3935) thanks to @morozov Improvement,Packaging --------------------- - [3924: Actualize the content of the .gitattributes file](doctrine#3924) thanks to @morozov Azure,Deprecation,Drivers,Drizzle,MariaDB,Platforms,PostgreSQL,SQL Anywhere,SQL Server,Sharding,pdo_ibm ------------------------------------------------------------------------------------------------------- - [3905: Deprecate the usage of the legacy platforms and drivers](doctrine#3905) thanks to @morozov Deprecation,Query ----------------- - [3864: CompositeExpression and()/or() factory methods](doctrine#3864) thanks to @BenMorel - [3853: Deprecate calling QueryBuilder methods with an array argument](doctrine#3853) thanks to @BenMorel and @morozov Deprecation,Tools ----------------- - [3861: Deprecated the usage of the Version class](doctrine#3861) thanks to @morozov Improvement,Query ----------------- - [3852: First parameter of ExpressionBuilder::and/or() mandatory](doctrine#3852) thanks to @BenMorel Deprecation,Improvement,Query ----------------------------- - [3851: Rename andX() / orX() methods](doctrine#3851) thanks to @BenMorel - [3850: Prepare CompositeExpression for immutability](doctrine#3850) thanks to @BenMorel # gpg: Signature made Mon Sep 21 01:47:31 2020 # gpg: using DSA key 1BEDEE0A820BC30D858F9F0C2C3A645671828132 # gpg: Can't check signature: No public key # Conflicts: # README.md # lib/Doctrine/DBAL/Driver/AbstractOracleDriver.php # lib/Doctrine/DBAL/Driver/OCI8/Driver.php # lib/Doctrine/DBAL/Version.php

morozov added Improvement Drivers Connections Prepared Statements labels Jun 23, 2020

morozov force-pushed the rename-driver-classes branch from 686124d to d41b097 Compare June 23, 2020 06:02

morozov marked this pull request as ready for review June 23, 2020 06:22

morozov requested a review from greg0ire June 23, 2020 06:24

greg0ire reviewed Jun 23, 2020

View reviewed changes

morozov force-pushed the rename-driver-classes branch from d41b097 to 76c38ad Compare June 23, 2020 20:23

morozov changed the title ~~Rename driver classes~~ Deprecate inconsistently and ambiguously named driver-level classes Jun 23, 2020

morozov force-pushed the rename-driver-classes branch 2 times, most recently from 383d304 to eeeff24 Compare June 24, 2020 01:24

morozov force-pushed the rename-driver-classes branch from eeeff24 to 9115580 Compare June 24, 2020 01:33

morozov added 4 commits June 23, 2020 18:49

Deprecate inconsistently and ambiguously named driver-level classes

a7374a2

Rework deprecated MySQLi exceptions

b7ee1fb

Rework deprecated OCI8 exceptions

2eab085

Rework deprecated IBMDB2 exceptions

569ca39

morozov force-pushed the rename-driver-classes branch from 9115580 to 569ca39 Compare June 24, 2020 01:49

morozov requested a review from greg0ire June 24, 2020 02:52

greg0ire approved these changes Jun 24, 2020

View reviewed changes

morozov added this to the 2.11.0 milestone Jun 24, 2020

morozov added the Deprecation label Jun 24, 2020

morozov merged commit a7c431c into doctrine:2.11.x Jun 24, 2020

morozov self-assigned this Jun 24, 2020

morozov deleted the rename-driver-classes branch June 24, 2020 14:33

This was referenced Jun 25, 2020

Preparation to marking OCI8 driver classes final #4109

Merged

Deprecate DriverException::getErrorCode() #4112

Merged

xabbuh mentioned this pull request Jun 29, 2020

[Messenger] fix compatibility with Doctrine DBAL 3.0 symfony/symfony#37448

Merged

stof reviewed Jun 29, 2020

View reviewed changes

morozov mentioned this pull request Jun 29, 2020

Restore the PortWithoutHost exception parent class #4131

Merged

stof reviewed Jun 29, 2020

View reviewed changes

This was referenced Jun 29, 2020

Fix more issues introduced by the deprecation of driver classes #4133

Merged

The changes in @internal classes shouldn't be ignored completely Roave/BackwardCompatibilityCheck#256

Open

morozov mentioned this pull request Sep 4, 2020

Deprecate DBAL\DBALException in favor of DBAL\Exception #4253

Merged

github-actions bot mentioned this pull request Sep 20, 2020

Merge release 2.11.0 into 3.0.x #4277

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2022

Uh oh!

Conversation

morozov commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greg0ire commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morozov commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greg0ire Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov commented Jun 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

morozov commented Jun 23, 2020 •

edited

Loading

greg0ire commented Jun 23, 2020 •

edited

Loading

morozov commented Jun 23, 2020 •

edited

Loading

greg0ire Jun 23, 2020 •

edited

Loading

greg0ire Jun 29, 2020 •

edited

Loading

morozov Jun 29, 2020 •

edited

Loading