Skip to content
This repository has been archived by the owner on Aug 1, 2021. It is now read-only.

Implement D8 initialization for civicrm-setup #11

Merged
merged 3 commits into from
Feb 20, 2018

Conversation

monishdeb
Copy link
Member

#10

@monishdeb
Copy link
Member Author

This is how I tested from php

try {
  \Civi\Cv\CmsBootstrap::singleton()->bootCms()->bootCivi();
}
catch (Exception $e) {
}

  \Civi\Setup::assertProtocolCompatibility(1.0);
  \Civi\Setup::init([
    "srcPath"=> "/Users/monish/src/civicrm",
    "setupPath"=> "/Users/monish/src/civicrm-setup",
    "cms"=> "Drupal8",
    "cmsBaseUrl"=> "http://localhost:8888/test-drupal8/",
    "db"=> [
      "server"=> "127.0.0.1:8889",
      "username"=> "root",
      "password"=> "root",
      "database"=> "test_drupal8"
  ]]);
  $setup = Civi\Setup::instance();
  if (!$setup->checkAuthorized()->isAuthorized()) {
    exit("Sorry, you are not authorized to perform installation.");
  }
  $setup->installFiles();
  $setup->installDatabase();

@@ -31,7 +31,7 @@
// Compute settingsPath.
$drupalSystem = new CRM_Utils_System_Drupal();
$cmsPath = $drupalSystem->cmsRootPath();
$siteDir = _drupal_civisetup_getSiteDir($cmsPath, $_SERVER['SCRIPT_FILENAME']);
$siteDir = \Civi\Setup\FileUtil::getDrupalSiteDir($cmsPath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving _drupal_civisetup_getSiteDir() to a shareable class seems like a good idea.

The functionality here is really specific to the Drupal file structure, so it'd be better to call the util \Civi\Setup\DrupalUtil::getSiteDir(...).

@@ -31,7 +31,7 @@
// Compute settingsPath.
$drupalSystem = new CRM_Utils_System_Drupal();
$cmsPath = $drupalSystem->cmsRootPath();
$siteDir = _drupal_civisetup_getSiteDir($cmsPath, $_SERVER['SCRIPT_FILENAME']);
$siteDir = \Civi\Setup\DrupalUtil::getDrupalSiteDir($cmsPath);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@totten Done :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

\Civi\Setup::dispatcher()
->addListener('civi.setup.init', function (\Civi\Setup\Event\InitEvent $e) {
$model = $e->getModel();
$cmsPath = \Drupal::root();
Copy link
Member

@totten totten Feb 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The \Drupal::root() call should be moved down a few lines. The civi.setup.init event will fire in all contexts -- if you tried to run \Drupal::root() on a D7/Backdrop/WordPress/Joomla site, it would cause a hard crash.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh right thanks for noticing that. Done


// Compute DSN.
global $databases;
$databases = \Drupal\Core\Database\Database::getConnectionInfo();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused -- in D8, is the $databases a global variable, or is it something we lookup via static function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops my bad, earlier I started to work on copied file of plugins/init/Drupal.civi-setup.php so missed to omit this line. Corrected now :)

@totten
Copy link
Member

totten commented Feb 20, 2018

I haven't tested on D8, but I'm inclined to merge anyway. Some considerations:

  • Tested on D7 and WP, with en_US and fr_FR -- and they still installed fine. Also, I ran cv's installation test-suite, and that still passes.
  • The patch generally reads OK.
  • The overall D8/composer effort requires multiple patches, and it's easier to test/reproduce if we don't have to manually apply all of them.
  • Changes in this repo won't go public until we tag a new release and update composer.{json,lock} for civicrm-core.

Thank you, @monishdeb!

@totten totten merged commit 92edff3 into civicrm:master Feb 20, 2018
@monishdeb monishdeb deleted the issue-10 branch February 21, 2018 02:24
@monishdeb
Copy link
Member Author

Thanks you @totten for merging this PR :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants