[FEATURE] Master/slave connection support. #214

Zylius · 2017-03-06T13:04:59Z

Changes proposed in this pull request:

Add support for master slave configuration.

This PR doesn't have breaking changes so long as users don't have random read or write keys in their connection configurations.

Example master/slave connection configuration:

[
            'driver'    => 'mysql',
            'host'      => 'localhost',
            'port'      => '3306',
            'database'  => 'test',
            'username'  => 'homestead',
            'password'  => 'secret',
            'write'     => [
                'port'      => 3307,
                'user'      => 'homestead1',
                'password'  => 'secret1',
            ],
            'read' => [
                [
                    'port'     => 3308,
                    'database' => 'test2',
                ],
                [
                    'host' => 'localhost2',
                    'port' => 3309
                ],
            ],
        ];

Settings can be specified different for each read/write configuration or you can just specify your base settings and override them in a specific read/write configuration (as in example).

patrickbrouwers · 2017-03-06T17:56:42Z

Can you add some more information to the PR?
E.g. does it have breaking changes for using the current state of the config file?

guiwoda · 2017-03-06T18:02:29Z

src/EntityManagerFactory.php

+            throw new \InvalidArgumentException("Parameter 'read' must be an array containing multiple arrays.");
+        }
+
+        if (($key = array_search(0, array_map('count', $slaves))) && $key !== false) {


isn't $key !== false redundant here? if its false, first condition fails.
Search could return position 0, which would be false-ish here as well, right?

yeah this seem like it's trying to be too clever by half and will fail if the first config (position 0) is empty, poc here https://3v4l.org/N8gnk There doesn't seem to be any tests for this?

A lot of this could be replaced with

array_map(function($config){ if (!is_array($config)) { //... } if (!count($config)) { //... } }, $slaves);

only one loop, probably less buggy.

That's correct, it's faulty. Will remove the second check and add a test case for it.

guiwoda · 2017-03-06T18:07:18Z

tests/Configuration/Connections/MasterSlaveConnectionTest.php

+     */
+    public function testMasterSlaveConnection(array $resolvedBaseSettings, array $settings, array $expectedOutput)
+    {
+        $this->assertEquals(


I like how you used dataProvider to reuse this case multiple times, but I find it a bit hard to maintain in this current form.
I wouldn't know where to modify each expectation for each setting.

Maybe refactor each case into its own method, then do something like:

public function getMasterSlaveConnectionData() { return [ $this->dummyBaseSettings(), $this->dummyConfig(), $this->dummyExpectations(), $this->oracleBaseSettings(), $this->oracleConfig(), $this->oracleExpectations(), }

Or something along those lines?

guiwoda · 2017-03-06T18:07:44Z

tests/EntityManagerFactoryTest.php

 use LaravelDoctrine\ORM\EntityManagerFactory;
 use LaravelDoctrine\ORM\Loggers\Logger;
 use LaravelDoctrine\ORM\Resolvers\EntityListenerResolver as LaravelDoctrineEntityListenerResolver;
+


Remove empty line between use statements.

guiwoda · 2017-03-06T18:10:16Z

tests/EntityManagerFactoryTest.php

+     *
+     * @return array
+     */
+    public function getTestMasterSlaveConnectionData()


Same as comment ☝️

maxbrokman

Is a duplicate of #157 ?

maxbrokman · 2017-03-07T10:13:26Z

src/EntityManagerFactory.php

+            throw new \InvalidArgumentException("Parameter 'read' must be an array containing multiple arrays.");
+        }
+
+        if (($key = array_search(0, array_map('count', $slaves))) && $key !== false) {


yeah this seem like it's trying to be too clever by half and will fail if the first config (position 0) is empty, poc here https://3v4l.org/N8gnk There doesn't seem to be any tests for this?

A lot of this could be replaced with

array_map(function($config){ if (!is_array($config)) { //... } if (!count($config)) { //... } }, $slaves);

only one loop, probably less buggy.

maxbrokman · 2017-03-07T10:14:17Z

tests/EntityManagerFactoryTest.php

+        $out[] = [
+            $inputConfig,
+            \InvalidArgumentException::class,
+            "Parameter 'read' config no. 2 is empty."


consider adding a test for the bug when read config 0 is empty

Zylius · 2017-03-07T13:41:00Z

Updated it according to comments. I'm fully aware this is a duplicate of two other PRs but hopefully since there are so many PRs for this issue we'll get one in laravel doctrine and this PR will be the one getting merged.

patrickbrouwers · 2017-03-28T23:07:27Z

Can you add an example config to /config. This doesn't have to get published, but would be a good reference point for anybody wanting to use master-slave. Also a PR to our Docs repo would be very helpful.

Zylius · 2017-03-29T06:52:26Z

I don't know why you guys want to add sample documentation for laravels configuration file in your ORM repo but I've done it. Also, you can find actual documentation for this on the laravel web site.

Zylius · 2017-03-29T07:09:15Z

And here is the docs PR laravel-doctrine/docs#67.

kpicaza · 2017-03-29T16:14:19Z

Sorry, this pull request will be merged in future versions of package?

It can be very powerful feature for most of big projects.

Thanks

patrickbrouwers · 2017-03-29T19:52:57Z

@kpicaza yes, if it passes my functional tests I'll merge it.

kpicaza · 2017-04-11T16:29:40Z

Can i help you on this issue improving tests or doing something that is needed?

Zylius · 2017-04-12T10:42:41Z

So are you guys actually planning to merge this or just ignore it for the next six months and close it like the other PRs?

I don't really care, but it seems like the community really wants it, and it doesn't look like anything is failing because of this new feature.

guiwoda · 2017-04-12T11:36:05Z

This PR was opened a month ago, not six. And we've shown interest in it.
Mind your attitude, please. We appreciate your help, but just as you, we have other responsibilities outside open source. This will be reviewed and merged when we can spare the time to do so, in the meantime, people can force their dependencies to the fork if they really need it, or implement master / slave outside app, which can also be done.

Zylius · 2017-04-12T12:24:50Z

There must've been some misunderstanding, I was talking about the older PRs being open for six months and being closed.
Yeah, I've done the master/slave inside app thing. Just trying to help out the community.
Anyways, I appreciate your efforts in trying to make laravel a better framework!

guiwoda · 2017-04-12T12:59:53Z

I think you may be confusing projects: we don't have any history of closing six-month-old PRs.

I've fixed Travis configuration, now 1.3 is running (and passing) all tests. Please rebase your fork and check that tests also pass in it.

Zylius · 2017-04-12T13:31:07Z

Done ;)

guiwoda

Some more suggestions, I think it looks pretty good overall. Would merge as soon as this is ready!

guiwoda · 2017-04-12T14:46:43Z

config/read_write_database_sample.php

+    |
+    */
+
+    'default' => env('DB_CONNECTION', 'auth'),


Having an invalid default here may be confusing for users.

guiwoda · 2017-04-12T14:48:08Z

config/read_write_database_sample.php

@@ -0,0 +1,105 @@
+<?php


Do we want a sample in code? I believe having updated docs with sample in docs will have better visibility.
Also, if syntax matches what Laravel documents, then we don't really need this at all.

This question is meant for @patrickbrouwers actually, as he suggested adding it 😄

Yes can be removed, I didn't understand at first it was the same syntax as the laravel config.

guiwoda · 2017-04-12T15:01:16Z

src/EntityManagerFactory.php

            $driver
        );

+        if ($this->isMasterSlaveConfigured($driver) && $this->hasValidMasterSlaveConfig($driver)) {


Refactor into:

if ($this->isMasterSlaveConfigured()) { $this->validateMasterSlaveConfig($driver); $connection = (new ...); }

guiwoda · 2017-04-12T15:01:44Z

src/EntityManagerFactory.php

+            throw new \InvalidArgumentException("Parameter 'read' config no. {$key} is empty.");
+        }
+
+        return true;


By suggested refactor, this method does not need a return value

Zylius · 2017-04-13T06:22:20Z

Done.

guiwoda · 2017-04-13T13:03:29Z

I'm 👍 with this!
Shoud we merge, @patrickbrouwers ?

Zylius force-pushed the 1.3_master_slave_connection branch 2 times, most recently from d159be3 to 8d636e2 Compare March 6, 2017 13:19

guiwoda suggested changes Mar 6, 2017

View reviewed changes

maxbrokman reviewed Mar 7, 2017

View reviewed changes

Zylius force-pushed the 1.3_master_slave_connection branch from a7b463c to ee5a1ee Compare March 7, 2017 13:30

guiwoda approved these changes Mar 7, 2017

View reviewed changes

This was referenced Mar 28, 2017

[feature] Master slave connection #163

Closed

[FEATURE] Add read write connection support #157

Closed

Zylius force-pushed the 1.3_master_slave_connection branch from 8a8fcd9 to 503d835 Compare March 29, 2017 06:49

Zigmas Satkevičius added 2 commits April 12, 2017 16:25

Added master/slave connection support.

0ce34ee

Fixed master/slave validation not checking first slave configuration.

d824138

Zylius force-pushed the 1.3_master_slave_connection branch from 503d835 to d678898 Compare April 12, 2017 13:25

guiwoda suggested changes Apr 12, 2017

View reviewed changes

Zylius force-pushed the 1.3_master_slave_connection branch from d678898 to 3f8f34f Compare April 13, 2017 06:15

Refactored master/slave connection tests.

cddf5b3

Zylius force-pushed the 1.3_master_slave_connection branch from 3f8f34f to cddf5b3 Compare April 13, 2017 06:16

guiwoda approved these changes Apr 13, 2017

View reviewed changes

patrickbrouwers approved these changes Apr 24, 2017

View reviewed changes

patrickbrouwers merged commit 5f7523c into laravel-doctrine:1.3 Apr 24, 2017

[FEATURE] Master/slave connection support. #214

[FEATURE] Master/slave connection support. #214

Uh oh!

Conversation

Zylius commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this pull request:

Uh oh!

patrickbrouwers commented Mar 6, 2017

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!

maxbrokman left a comment

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!

Zylius commented Mar 7, 2017

Uh oh!

patrickbrouwers commented Mar 28, 2017

Uh oh!

Zylius commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zylius commented Mar 29, 2017

Uh oh!

kpicaza commented Mar 29, 2017

Uh oh!

patrickbrouwers commented Mar 29, 2017

Uh oh!

kpicaza commented Apr 11, 2017

Uh oh!

Zylius commented Apr 12, 2017

Uh oh!

guiwoda commented Apr 12, 2017

Uh oh!

Zylius commented Apr 12, 2017

Uh oh!

guiwoda commented Apr 12, 2017

Uh oh!

Zylius commented Apr 12, 2017

Uh oh!

guiwoda left a comment

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!

patrickbrouwers Apr 12, 2017 • 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!

Zylius commented Apr 13, 2017

Uh oh!

guiwoda commented Apr 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Zylius commented Mar 6, 2017 •

edited

Loading

Zylius commented Mar 29, 2017 •

edited

Loading

patrickbrouwers Apr 12, 2017 •

edited

Loading