[GH-4052] Deprecate MasterSlaveConnection and rename to PrimaryReplicaConnection #4054

beberlei · 2020-06-06T22:55:25Z

Q	A
Type	bug
BC Break	no
Fixed issues	#4052

Summary

Renames MasterSlaveConncetion to PrimaryReplicaConnection and all params or connection keys from master to primary and from slaves/slave to replica.

A completly BC implementation calling the new class is provided as well and throws deprecation messages.

Usage:

$conn = DriverManager::getConnection(array(
   'wrapperClass' => 'Doctrine\DBAL\Connections\PrimaryReplicaConnection',
   'driver' => 'pdo_mysql',
   'primary' => array('user' => '', 'password' => '', 'host' => '', 'dbname' => ''),
   'replica' => array(
       array('user' => 'replica1', 'password', 'host' => '', 'dbname' => ''),
       array('user' => 'replica2', 'password', 'host' => '', 'dbname' => ''),
   )
));

Open question: Should the master => primary and slave/salves => replica renames for constructor params and connect method trigger deprecations on MasterSlaveConnection only, or also when wrongly used on PrimaryReplicaConnection?

codecov-commenter · 2020-06-06T23:07:10Z

Codecov Report

Merging #4054 into 2.11.x will increase coverage by 0.00%.
The diff coverage is 74.19%.

@@            Coverage Diff            @@
##             2.11.x    #4054   +/-   ##
=========================================
  Coverage     71.02%   71.02%           
- Complexity     5261     5280   +19     
=========================================
  Files           217      218    +1     
  Lines         13355    13416   +61     
=========================================
+ Hits           9485     9529   +44     
- Misses         3870     3887   +17

Impacted Files	Coverage Δ	Complexity Δ
.../DBAL/Connections/PrimaryReadReplicaConnection.php	`68.37% <68.37%> (ø)`	`47.00 <47.00> (?)`
...octrine/DBAL/Connections/MasterSlaveConnection.php	`66.66% <90.90%> (-0.31%)`	`11.00 <5.00> (-31.00)`
lib/Doctrine/DBAL/DriverManager.php	`94.28% <100.00%> (+0.28%)`	`49.00 <0.00> (+3.00)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3cfb57d...071824e. Read the comment docs.

morozov

While I'm fully onboard with the rename, the things I don't like about the current and the new implementation are:

It inherits the wrapper connection instead of using composition (not sure if it's possible with the current design).
It changes the semantic of the connect() method by adding an optional argument.

While the above is not currently a blocker, I'm curious if anyone had any thoughts about redesigning it rather than just renaming.

morozov · 2020-06-06T23:18:44Z

@beberlei are you sure we want to introduce a deprecation in 2.10.x? It probably should be 2.11.x.

greg0ire · 2020-06-07T06:55:39Z

While the above is not currently a blocker, I'm curious if anyone had any thoughts about redesigning it rather than just renaming.

I had the same thought in #4052 :

An alternative would be to just not use aliases and copy/paste/improve the class (making it final, using private properties, using type declarations). We would then deprecate MasterSlaveConnection (and maybe stop maintenance on it?).

greg0ire · 2020-06-07T07:29:03Z

I'm working on a alternate implementation so that you can see what I mean.

greg0ire · 2020-06-07T08:10:25Z

See #4055

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php

lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php

greg0ire · 2020-06-08T17:26:35Z

tests/Doctrine/Tests/DBAL/Functional/PrimaryReadReplicaConnectionTest.php

+        $platformName = $this->connection->getDatabasePlatform()->getName();
+
+        // This is a MySQL specific test, skip other vendors.
+        if ($platformName !== 'mysql') {


@stof said this was supposed to work with PG too, should we maybe change this line?

@greg0ire yes thats a future todo imho, the test actually only works with MySQL because of the charset test.

stof · 2020-06-08T18:39:41Z

lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php

        }

-        return $statement;
+        return parent::connect($connectionName);


This is broken. When passing slave or master as connection name, you must call the alternative method instead of calling parent::connect($connectionName); with a renamed connection name, as you changed the parent class.

parent::connect() should be called only for remaining cases

stof · 2020-06-08T18:40:31Z

lib/Doctrine/DBAL/Connections/PrimaryReadReplicaConnection.php

+    public function connect($connectionName = null)
+    {
+        if ($connectionName !== null) {
+            throw new InvalidArgumentException('Passing a connection name as first argument is not supported anymore. Use ensureConnectedToPrimary()/ensureConnectedToReplica() instead.');


Do we even need that argument and that exception ? This is a new class.

@stof yes i believe, the problem is that passing to many arguments does not throw an error in strict_types=0, as such if someone would wrongly convert their code and keep one connect("master") or slave somewhere they would never notice.

stof · 2020-06-08T18:42:02Z

tests/Doctrine/Tests/DBAL/Functional/MasterSlaveConnectionTest.php

-/**
- * @group DBAL-20
- */
-class MasterSlaveConnectionTest extends DbalFunctionalTestCase


these tests should be kept (as legacy tests), to ensure that the deprecated class still works fine.

stof · 2020-06-09T08:38:44Z

lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php

-        return parent::prepare($statement);
+        @trigger_error(
+            sprintf(
+                '%s is deprecated since doctrine/dbal 2.10 and will be removed in 3.0, use %s instead.',


@beberlei this should be updated to 2.11 instead.

…ryReplicaConnection

morozov · 2020-06-11T03:06:19Z

@beberlei or @greg0ire could you merge this up, please? I'm not sure I will resolve the conflicts correctly.

greg0ire · 2020-06-11T05:51:46Z

I'm on it!

greg0ire · 2020-06-11T06:23:32Z

#4063

morozov · 2020-06-17T04:46:45Z

@greg0ire could you please also file a PR that removes the deprecated class from 3.0.x?

doctrine/dbal v2.11.0 (doctrine/dbal#4054) has deprecated MasterSlaveConnection class in favour of PrimaryReadReplicaConnection This change adds support for this new PrimaryReadReplicaConnection class (while being BC-compatible with MasterSlaveConnection on older dbal versions) and would allow users to use wrapperClass that extends PrimaryPrimaryReadReplicaConnection (which otherwise fails as this package would try to use master/slave config instead of primary/replica one)

…ons API.

@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

…ons API.

beberlei requested review from morozov and greg0ire June 6, 2020 22:55

beberlei force-pushed the GH-4052-PrimaryReplicaConnection branch from 3f549d0 to ac48a5c Compare June 6, 2020 23:09

morozov previously approved these changes Jun 6, 2020

View reviewed changes

beberlei changed the title ~~[GH-4052] Deprecate MasterSlaveConnection and rename to PrimaryReplicationConnection~~ [GH-4052] Deprecate MasterSlaveConnection and rename to PrimaryReplicaConnection Jun 7, 2020

greg0ire mentioned this pull request Jun 7, 2020

Deprecate master/slave terminology and underlying feature #4055

Closed

beberlei dismissed morozov’s stale review via 1b23e30 June 7, 2020 21:37

beberlei force-pushed the GH-4052-PrimaryReplicaConnection branch from ac48a5c to 1b23e30 Compare June 7, 2020 21:37

beberlei changed the base branch from 2.10.x to 2.11.x June 7, 2020 21:37

beberlei force-pushed the GH-4052-PrimaryReplicaConnection branch from 1b23e30 to 3e93264 Compare June 7, 2020 21:46

greg0ire reviewed Jun 8, 2020

View reviewed changes

lib/Doctrine/DBAL/Connections/PrimaryReplicaConnection.php Outdated Show resolved Hide resolved

stof reviewed Jun 8, 2020

View reviewed changes

lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php Outdated Show resolved Hide resolved

stof reviewed Jun 8, 2020

View reviewed changes

greg0ire reviewed Jun 8, 2020

View reviewed changes

greg0ire previously approved these changes Jun 8, 2020

View reviewed changes

stof reviewed Jun 8, 2020

View reviewed changes

beberlei dismissed greg0ire’s stale review via 7788fff June 9, 2020 08:31

stof previously approved these changes Jun 9, 2020

View reviewed changes

stof reviewed Jun 9, 2020

View reviewed changes

[doctrineGH-4052] Deprecate MasterSlaveConnection and rename to Prima…

071824e

…ryReplicaConnection

beberlei dismissed stof’s stale review via 071824e June 9, 2020 10:34

beberlei force-pushed the GH-4052-PrimaryReplicaConnection branch from 7788fff to 071824e Compare June 9, 2020 10:34

stof approved these changes Jun 9, 2020

View reviewed changes

beberlei added this to the 2.11.0 milestone Jun 9, 2020

beberlei merged commit 7047798 into doctrine:2.11.x Jun 9, 2020

beberlei deleted the GH-4052-PrimaryReplicaConnection branch June 9, 2020 12:48

morozov assigned beberlei Aug 14, 2020

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

Merge release 2.11.0 into 3.0.x #4277

Merged

Dodenis mentioned this pull request Sep 25, 2020

Fix master-slaves deprecation from dbal 2.11 doctrine/DoctrineBundle#1215

Merged

sserbin mentioned this pull request Dec 28, 2020

[FEATURE] support PrimaryReadReplicaConnection (former MasterSlaveConnection) laravel-doctrine/orm#472

Merged

beberlei added a commit to beberlei/dbal that referenced this pull request Mar 6, 2021

[doctrineGH-4054] Convert MasterSlaveConnection to Doctrine deprecati…

a6cb2cc

…ons API.

beberlei added a commit to beberlei/dbal that referenced this pull request Mar 7, 2021

[doctrineGH-4054] Convert MasterSlaveConnection to Doctrine deprecati…

4d4af98

…ons API.

beberlei added a commit to beberlei/dbal that referenced this pull request Mar 22, 2021

[doctrineGH-4054] Convert MasterSlaveConnection to Doctrine deprecati…

59148f4

…ons API.

morozov added the Replication label Jan 27, 2022

github-actions bot locked as resolved and limited conversation to collaborators Jan 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GH-4052] Deprecate MasterSlaveConnection and rename to PrimaryReplicaConnection #4054

[GH-4052] Deprecate MasterSlaveConnection and rename to PrimaryReplicaConnection #4054

beberlei commented Jun 6, 2020 •

edited

Loading

codecov-commenter commented Jun 6, 2020 •

edited

Loading

morozov left a comment

morozov commented Jun 6, 2020

greg0ire commented Jun 7, 2020

greg0ire commented Jun 7, 2020

greg0ire commented Jun 7, 2020

greg0ire Jun 8, 2020

beberlei Jun 8, 2020

stof Jun 8, 2020

stof Jun 8, 2020

beberlei Jun 8, 2020

stof Jun 8, 2020

stof Jun 9, 2020

morozov commented Jun 11, 2020

greg0ire commented Jun 11, 2020

greg0ire commented Jun 11, 2020

morozov commented Jun 17, 2020

[GH-4052] Deprecate MasterSlaveConnection and rename to PrimaryReplicaConnection #4054

[GH-4052] Deprecate MasterSlaveConnection and rename to PrimaryReplicaConnection #4054

Conversation

beberlei commented Jun 6, 2020 • edited Loading

Summary

codecov-commenter commented Jun 6, 2020 • edited Loading

Codecov Report

morozov left a comment

Choose a reason for hiding this comment

morozov commented Jun 6, 2020

greg0ire commented Jun 7, 2020

greg0ire commented Jun 7, 2020

greg0ire commented Jun 7, 2020

greg0ire Jun 8, 2020

Choose a reason for hiding this comment

beberlei Jun 8, 2020

Choose a reason for hiding this comment

stof Jun 8, 2020

Choose a reason for hiding this comment

stof Jun 8, 2020

Choose a reason for hiding this comment

beberlei Jun 8, 2020

Choose a reason for hiding this comment

stof Jun 8, 2020

Choose a reason for hiding this comment

stof Jun 9, 2020

Choose a reason for hiding this comment

morozov commented Jun 11, 2020

greg0ire commented Jun 11, 2020

greg0ire commented Jun 11, 2020

morozov commented Jun 17, 2020

beberlei commented Jun 6, 2020 •

edited

Loading

codecov-commenter commented Jun 6, 2020 •

edited

Loading