civicrm / civicrm-setup

MIT License
7 stars 5 forks source link

Allow CiviCRM database to be different than the CMS one during civi.setup.ini #14

Closed davialexandre closed 6 years ago

davialexandre commented 6 years ago

Problem

Any DB configuration passed to the model via the \Civi\Setup::init() method was ignored and CiviCRM ended up installed in the CMS database.

Solution

The problem was caused because the plugins listening to the civi.setup.init event, which is triggered when you call \Civi\Setup::init(), would override any value passed to the db param with the cms db configuration.

To avoid that, the db configuration is only set to the same one as the cmsDb if no db configuration is specified (i.e. $model->db is empty).

monishdeb commented 6 years ago

I agree with this patch which provides flexibility in setting separate DB for Civi if the configuration is passed to the model or else will use the CMS DB as default.

totten commented 6 years ago

@davialexandre belated thanks for submitting this.

I struggled for a bit -- because there is a really good point here, but I've felt ambivalent about the change, and couldn't put my finger on why. But some time/distance has helped a bit.

Let's put this in the context of a more complete example:

// Snippet 1: Initialize and customize model at the same time
\Civi\Setup::init([
  'cms' => 'Drupal',
  'db' => ['server'=>'localhost:3306', 'username'=>'admin', 'password'=>'s3cr3t', 'database'=>'mydb'],
  'cmsDb' => ['server'=>'localhost:3306', 'username'=>'admin', 'password'=>'s3cr3t', 'database'=>'mydb'],
  'cmsBaseUrl'=> 'http://example.com',
]);
$setup = \Civi\Setup::instance();

$setup->installFiles();
$setup->installDatabase();

That's actually a pretty straight-forward example you could write after skimming Civi/Setup.php and Civi/Setup/Model.php. But as you point out, it won't work as expected -- because the autodetected value of db overrides the inputted value of db.

Another pattern is to let init() figure out defaults -- and then set the values afterward, e.g.:

// Snippet 2: Initialize model and then customize it.
\Civi\Setup::init(['cms' => 'Drupal']);
$setup = \Civi\Setup::instance();

$setup->getModel()->setValues([
  'db' => ['server'=>'localhost:3306', 'username'=>'admin', 'password'=>'s3cr3t', 'database'=>'mydb'],
  'cmsDb' => ['server'=>'localhost:3306', 'username'=>'admin', 'password'=>'s3cr3t', 'database'=>'mydb'],
  'cmsBaseUrl'=> 'http://example.com',
]);

$setup->installFiles();
$setup->installDatabase();

This is basically what cv core:install does. It only passes cms, setupPath, and srcPath to init(). Everything else is customized afterward.

The simple solution is this: use Snippet 2 instead of Snippet 1.

Granted, Snippet 1 is pretty. Maybe supporting Snippet 1 would make the library clearer and less confusing. My problem is that #14 confronts this confusion but ultimately leaves it just as confusing. Other settings (eg cmsDb, cmsBaseUrl, templateCompilePath, components, paths, mandatorySettings) have the same issue. All the default values in {Drupal,Backdrop,WordPress,Components}.civi-setup.php were written with the expectation of using Snippet 2. Supporting Snippet 1 would require adding more conditionals to more plugins/fields.

Honestly, I think it's easier to clarify the relevant docs. That way (a) we don't have deal with a series of follow-up patches to handle other settings and (b) the autodetection code in Drupal.civi-setup.php (et al) stays simple and (c) there are fewer issues about interoperability (e.g. "the Foo installer requires a newer version of the installer library").