Skip to content

Conversation

@terabytesoftw
Copy link
Member

@terabytesoftw terabytesoftw commented Feb 17, 2020

Q A
Is bugfix?
New feature? ✔️
Breaks BC?
Tests pass? ✔️
  • Create DatabaseFactory independent, will allow to create the master and slave connections through the factory with autowired.

Example:

$connection->masters[] = [
    'setDsn()'      => ['mysql:host=127.0.0.1;dbname=yiitest'],
    'setUsername()' => ['root],
    'setPassword()' => ['root'],
];

$connection->open();
  • Remove dsn from the constructor: The dsn of the constructor was removed and two setters were created for its configuration.

Example Array:

$db = new Connection($cache, $logger, $profiler);

$dsn => [
     'driver' => 'mysql',
     'host'   => '127.0.0.1',
     'dbname' => 'yiitest',
],

$db->setBuildDsn($dsn);

Example string:

$db = new Connection($cache, $logger, $profiler);

$db->setDsn('mysql:host=127.0.0.1;dbname=yiitest');
  • Change name badged README.md phpunit --> build.

@terabytesoftw terabytesoftw requested review from a team and samdark February 18, 2020 14:33
@terabytesoftw terabytesoftw added the status:code review The pull request needs review. label Feb 18, 2020
@xepozz
Copy link
Member

xepozz commented Feb 18, 2020

I don't like it. Let's avoid lazy initializing via $config = [...].
I think it can be developed with ReplicationConnectionInterface (or smth else), that will have methods addMaster(ConnectionInterface $connection), addSlave(ConnectionInterface $connection) and other.

@terabytesoftw
Copy link
Member Author

terabytesoftw commented Feb 18, 2020

I don't like it. Let's avoid lazy initializing via $config = [...].
I think it can be developed with ReplicationConnectionInterface (or smth else), that will have methods addMaster(ConnectionInterface $connection), addSlave(ConnectionInterface $connection) and other.

In this particular case the factory avoids two things, one injecting the dependencies, and two possibility of creating dynamic connections, if I define everything in the container di that cannot be done.

@samdark
Copy link
Member

samdark commented Feb 19, 2020

Is connection possible without DSN?

@terabytesoftw
Copy link
Member Author

terabytesoftw commented Feb 19, 2020

Is connection possible without DSN?

You can create the Connection::class without configuring Dsn, Username and Password, but when you are going to open connection $connection->open() , if it is not configured, throw an exception, example:

if (empty($this->dsn)) {

The idea for now is to clean the constructor as much as possible, in order to inject the interfaces of SchemaInterface and CommandInterface and eliminate the factory within Connection::class.

Creating the connection class does not mean that you create the instance of PDO::class, it only creates at the time of opening the connection.

Having a DSN setter also allows you to have a connection created and to be able to access any connection dynamically, that is also an advantage.

I would have no problem adding more tests where we checked the exception without configuration of Dsn, Username and Password.

@samdark
Copy link
Member

samdark commented Feb 19, 2020

If we can't use Connection without DSN, it's better to have it as a mandatory requirement i.e. keep it in constructor.

@terabytesoftw
Copy link
Member Author

If we can't use Connection without DSN, it's better to have it as a mandatory requirement i.e. keep it in constructor.

Its mandatory for PDO::class not for Connection::class, which does too many things, and these changes are to decouple that class.

@samdark samdark merged commit 2fabe12 into yiisoft:master Feb 24, 2020
@samdark
Copy link
Member

samdark commented Feb 24, 2020

Thanks!

@terabytesoftw terabytesoftw deleted the path-factory-db branch February 26, 2020 13:20
@terabytesoftw terabytesoftw removed the status:code review The pull request needs review. label Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants