Proxy with dynamic connection

[boehsermoe]

What are you changing/introducing

Replaced all Yii::$app->db usage with $this->db.

@nadar i merged unintentionally my master with #270, after that merged it should be fine.

What is the reason for changing/introducing

Can use a other database connection for the proxy sync. So you can sync any database also from a other system.

QA

Q	A
Is bugfix?	no
New feature?	yes
Breaks BC?	no
Tests pass?	yes/no

[nadar]

Instead of adding this to the module property, i think this should be an option for the cli command?

--db=xyz

[coveralls]

Coverage increased (+0.002%) to 16.439% when pulling 10855ac on boehsermoe:proxy-connection into 54c6527 on luyadev:master.

[boehsermoe]

I will add the command parameter later. At first I would to implement only the connection to make it configurable via config file. The Module::$proxyConnectionName will then used as default.

[nadar]

To be honest i really believe this should not belong to the Module, this is not something which should be configured in the Module as we are also not going to do this in other situations (for other commands). It should not be a big deal to change. Also the Yii Framework commands work like this, so we should follow those "best practice" behaviors:

https://github.com/yiisoft/yii2/blob/master/framework/console/controllers/MigrateController.php#L134

[nadar]

What i saw just now is that you are also use this component definition for the API - so you think even the API side needs to have configurable database component? Could you make an use case where this is needed? Maybe we should define the use case very quickly and check the best solution.

[boehsermoe]

My case is to sync two magento databases. For that I want to run the client on a staging/test environment and the api-proxy on production env. The client need the db connection to the staging db and the api to production db. Both of them should not have an other option to system with other db.

[nadar]

I think the admin side (api) is a good idea to configure via module, please use instance::ensure instead - everything else looks good now.

For the client (the one which actually runs the sync command) we should have the --db=xyz option instead of a config.

[nadar]

1. The ensure should work like $db = 'db' see https://www.yiiframework.com/doc/api/2.0/yii-di-instance.
2. Decide whether the db connection object for the console command should be done in ClientBuild or the Command itself and pass to the ClientBuild object.
3. The object would be passed to ClientTable as well, in order to ensure its a database object we could also use a getter/setter method in ClientTable and ClientBuild setDb(Connection $db) which ensures the correct instance is provided.

Thanks for your work, its harder topic then i thought. So to summarize it again:

• The server can specific the db connection trough admin module: proxyConnectionName (maybe using proxyDbConnection?)
• The client can change the database (where the data should be stored) only with --db=xyz

[nadar]

We are very close to 2.0 release, is there something i can provide on this PR in order to finish it soon? Let me know if we should resolve all conversations and take a "new" look.

[nadar]

This should be the finally review i assume :-)

Regarding class names: Its not a change from this PR, but it would be nice to always add classes into use statments (example: \yii\db\mysql\Schema)

[nadar]

Please change the getter setter behavior and add the @Property phpdoc annotation. Everything else looks good and we are ready to merge afterwards. Would be nice to do this asap, otherwise we have to move this feature to version 2.1.0

[nadar]

Thanks for taking time @boehsermoe!