Multisite support

[becw]

I'm having some issues with this -- I think partly because I'm being timid about making the structural changes. What I'm expecting to do here is to move the Drupal site configuration from one location in the properties array to another.

This writeup of my current thoughts will hopefully help me figure out a next step, and be helpful for reviewers too...

Old:

drupal:
  root: "${drupal.root}"
  hash_salt: "${drupal.hash_salt}"
  profile: "${drupal.profile}"

  database:
    database: "${drupal.database.database}"
    username: "${drupal.database.username}"
    password: "${drupal.database.password}"
    host: "${drupal.database.host}" 

  twig:
    debug: true

New:

drupal:
  root: "${drupal.root}"

  twig:
    debug: true

  sites:
    default:
      hash_salt: "${drupal.hash_salt}"
      profile: "${drupal.profile}"

      database:
        database: "${drupal.database.database}"
        username: "${drupal.database.username}"
        password: "${drupal.database.password}"
        host: "${drupal.database.host}"

Aside from renaming properties where they're currently referenced, this poses a couple of issues:

• We can no longer reference just drupal.sites.default.whatever, since if there are multisites, we would also need to look at drupal.sites.second_site.whatever
• Though these properties are declared in a yaml array, Phing doesn't internally treat them as arrays, nor does it provide any iterators that traverse properties
• We have to determine which tasks need to run for the site (as a whole) vs. for a specific multisite

I've been thinking about a couple of ways to address these issues:

• A phing task to copy properties from one part of the property array to another. So, we could copy drupal.sites.default.* to drupal.site.*, run our tasks, then copy drupal.sites.second_site.* to drupal.site.*, and run the same tasks again with the new values. Code for this (CopyPropertiesTask.php) is included in this PR.
• A phing task to iterate over properties by key. Emulate phing's core but take a property prefix and a target as input, and then run the target with each key as a parameter value. Code for this (ForeachKeyTask.php) is included in this PR.
• Alternatively, a phing filter that expands properties with a particular prefix. Like but takes a property prefix as input, and slices that off of any property names before expanding them. So, with the property drupal.sites.default.whatever, you could pass the prefix drupal.sites.default. and then values like ${whatever} in your document would be expanded. Code for this approach is included in this PR (ExpandPrefixedProperties.php).

I'm still concerned that any of these approaches introduces a lot of complexity in the default build.xml file -- specifically in the build, install, and behat targets -- that will make that file less usable to developers.

To address this last issue, I'm considering a couple of approaches:

• Moving a big chunk of logic into a new Task class, which risks obscuring what's going on. This would also make the activities much less customizable.
• Setting a site interactively for any task that requires it -- so that these tasks can always run against just one site. It could be set up so that if there was just one site in drupal.sites.*, it would always run against that one.

Other open questions...

• Should the settings.build.php and services.build.yml template files be a per-site setting?
• Should some properties always be generated internally? How do we handle default values for things like the files directory? Is there a middle ground between "always set by the-build" and "always required in a project's properties files"?

[becw]

Ahh I think I found a path forward, based on this:

Setting a site interactively for any task that requires it -- so that these tasks can always run against just one site. It could be set up so that if there was just one site in drupal.sites.*, it would always run against that one.

Further next steps are in @todo comments in the code, for the moment.

This code currently installs with one site, but with the properties in their newly nested location.

[becw]

Ok, it's working! I have a couple of open questions with this approach:

• If you have multiple sites, this will prompt you for a site when you run things like phing artifact. That isn't super meaningful, because the artifact target doesn't care what Drupal multisite you're talking about, it will just build all of them.
• Do we need to adapt the behat.args property to multisites? Like, should the behat command be one of those that can vary per site, so that sites can use different behat profiles or configuration files?
• Does the drupal-load-db stuff need to be adapted to multisites?

I don't super want to address all of these things in this PR, because we might be able to tag-team these updates, but also I want to make sure that this PR doesn't prevent us from doing the right thing in other places.

What do you think?

[becw]

Also, the way this works is only obliquely documented so far.

1. Drupal multisites should each be configured in a sub-property of drupal.sites, like drupal.sites.default, drupal.sites.intranet, and drupal.sites.conference2019
2. When the-build is invoked, it will check for sub-properties of drupal.sites.

• If it finds one key, it will silently use that
• If there is more than one key, it will prompt you to pick which to use
• The prompt can be bypassed by setting the key with -Dbuild.site=intranet as an argument to the phing command, or by setting an environment variable with export THE_BUILD_SITE=conference2019

3. The values from drupal.sites.intranet (or whichever key you selected) are copied to drupal.site.*, so that drupal.sites.intranet.uri is mapped to drupal.site.uri. This way, the-build can find all the info it needs for Drupal in the same place, no matter which multisite it's running against.
4. Default values are filled in from drupal.sites._defaults. This is so that you don't have to declare all of the site-specific properties explicitly for your drupal.sites.conference2019 multisite.
5. Finally, a few values are set dynamically in the-build.xml. This is done for properties that depend on the Drupal sites subdirectory, like the settings file destination and the Drupal files directory. We can't do this in the defaults.properties.yml anymore, because once you add more multisites to a project-specific config you will not get good defaults from the-build for those new multisites. You can override these if you want, but if you don't -- you'll have something usable.

I think that covers it, but it's a lot more complicated than it used to be. I wonder if there are ways to simplify it... but it might just come down to documentation. Maybe adding some of the new drupal.sites._defaults structure to the defaults/templates/the-build/build.default.properties.yml file, and some comments about why we're referencing drupal.site.foo instead of drupal.sites.default.foo in the defaults/templates/drupal.settings.build.php file?

[byrond]

I think this needs to be settings_template. I was building an artifact for Acquia, and it was using the local build settings template.

[becw]

nice catch!

[becw]

@byrond -- thanks for your review so far! I'm super appreciative that you're running the artifact builds.

I've made the following changes since you last looked:

• Updated the documentation
• Added a target to create new multisites (phing drupal-add-multisite)
• Added targets to run the build or install against all multisites (phing build-all, phing install-all)

I'm mostly satisfied with where this is right now. I'm going to create spin off new issues re: updating the db loading, though, since that's affected by this work, but I don't want to add even more code to this PR.

[byrond]

Something weird is happening when running the-build-installer now. This doesn't seem to fail on the host when there is no database server running.

example > drupal-has-database-connection:

    [drush] Executing '/Users/byronduvall/example/vendor/bin/drush --nocolor --root="/Users/byronduvall/example/web" --uri="https://example.local" --config="/Users/byronduvall/example/drush/drushrc.php" sql-query "SELECT 1"'...
Query failed.                                                            [error]
    [drush] Executing '/Users/byronduvall/example/vendor/bin/drush --nocolor --root="/Users/byronduvall/example/web" --uri="https://example.local" --config="/Users/byronduvall/example/drush/drushrc.php" --yes sql-create'...
    [drush] Creating database drupal. Any possible existing database will be dropped!
    [drush] Do you really want to continue? (y/n): y
     [echo] Drupal database connection available.
Install Drupal now (y,n)?

[byrond]

This seems to be a bug in Drush. The result is false, but drush_set_error() is never called. This also happens in drush sql-drop, and apparently used to happen on drush sql-query:

drush-ops/drush#34 (comment)

[byrond]

I created a PR against Drush 8.x here: drush-ops/drush#3732

[byrond]

OK, that one was too aggressive. See drush-ops/drush#3733

[byrond]

A fix for this has been rolled into the 8.x branch! drush-ops/drush@97f7d2a 🎉

[byrond]

@becw I did some more testing of this PR, and things are working great. I only found the one quirk with the-build-installer (mentioned above). I'm not sure if we can do anything about that with respect to this PR, so it may need to be fixed in Drush.