-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Deprecated the usage of the Version class #3861
Conversation
morozov
commented
Jan 30, 2020
Q | A |
---|---|
Type | deprecation |
BC Break | no |
@@ -8,6 +8,9 @@ | |||
|
|||
/** | |||
* Class to store and retrieve the version of Doctrine. | |||
* | |||
* @internal | |||
* @deprecated Refrain from checking the DBAL version at runtime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will result in a deprecation for users of https://github.com/symfony/error-handler/blob/master/DebugClassLoader.php . Wouldn't marking it as @internal
be enough to warn users against using it, and maybe more semantically correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is removed in master. AFAIK master is going to be 4.0, so the deprecation message should contain the information when this class will be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will result in a deprecation for users of https://github.com/symfony/error-handler/blob/master/DebugClassLoader.php.
The problem is not the deprecation but how it's being handled. In our case, we're just adding an annotation purely for documentation purposes. If a 3rd party component turns this annotation into an undesired runtime side effect, the problem is in the component. Do users use it in production? The "debug" name suggests they shouldn’t.
Wouldn't marking it as
@internal
be enough to warn users against using it, and maybe more semantically correct?
I don't know. Both annotations are standard and correspond to the messages I want to communicate:
@intermal
— the class is an implementation detail and shouldn’t be relied upon outside of the package. It should have been marked like this since the introduction.@deprecated
— the class is going to be removed in a following major release.
It is removed in master. AFAIK master is going to be 4.0, so the deprecation message should contain the information when this class will be removed.
We don't usually do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's removed on master, then @deprecated
should stay 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do users use it in production? The "debug" name suggests they shouldn’t.
You're right, that probably makes this quite ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no problem with both @internal
and @deprecated
, since they are different. The explanation @morozov gave above is perfectly correct.
As for the version, these are my thoughts:
- If we deprecate something and don't know when it will be released, we should say "in a future major release" as that's what will happen
- Once we know what release removes the member, we should update the deprecation message to state the version that removes it. Even if we deprecate in 2.x for removal in 4.0, we should mention that the class will be removed in 4.0. Users that are interested in the deprecations will appreciate the information as it lets them decide when to take care of the deprecation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we deprecate something and don't know when it will be released, we should say "in a future major release" as that's what will happen
I'm fine with this definition of “when” — “in a future major release”, however, isn’t implied by semver? It definitely cannot happen earlier.
Once we know what release removes the member, we should update the deprecation message to state the version that removes it.
This is why I don't want to specify any exact versions. Specifically in DBAL, we don’t know that upfront and I don’t want to introduce additional work on updating deprecation comments.
Users that are interested in the deprecations will appreciate the information as it lets them decide when to take care of the deprecation.
I didn’t think people may be really interested in the exact version. What is the practical reason for interest in a specific removal version? In our case, even if we mention a specific version, we don’t have any release dates for those versions.
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