US15531: Add explicit support for D8

[cweagans]

Right now, it's a per-site configuration option, since most of our stuff is still D7. Right now, the only thing the flag does is change how settings.j2 is written out.

[conortm]

@cweagans This is great! What about, instead of d8: true in the sites array, we have drupal_version: 8, and we just default the value to 7?

[scottrigby]

@cweagans This is great! I use pubstack for D8 sandboxes, but have just made my own settings files as needed. This is nicer. Also I like @conortm 's suggestion of an optional drupal_version. Naming variables, modules (GitHub repos - ha!) with 7 or 8 in them feels a little weird :wink:

[cweagans]

@conortm @scottrigby Fair enough. I'll do that tomorrow morning.

[ericduran]

Hmm, If everyone wants to do this that's fine.

Personally I would just separate a D7-settings.inc and D8-settings.inc and then I'll just include it conditionally from settings.inc based on the site.

We can then just have that settings.inc figure out D7 or D8.

For d7 we can use

if (defined('DRUPAL_CORE_COMPATIBILITY'))

and on D8:

if (class_exists('Drupal') && defined('Drupal::CORE_COMPATIBILITY'))

We can also check for 7.x and 8.x in those constants if we want.

[ericduran]

This way no extra config or anything needed on the user end.

[cweagans]

I think I'd prefer to just write out the right configuration for the version of Drupal we're using. In reality, only people that are using D8 will have to care about this configuration, so I think the impact will be pretty minimal.

[ericduran]

Yep, that's fine. I just prefer having as little config as possible, since I'll like to be able to provision an entire box without knowing before hand what version of Drupal you're running.

[conortm]

Much to my chagrin, I agree with @ericduran :stuck_out_tongue_closed_eyes:. Less config. I think we should have version-specific settings templates that get required conditionally from the current settings include.

[ericduran]

Another note is that on D8 we're going to want to configure different $conf settings aka $conf['file_directory_temp'] vs $conf['file_temporary_path']

[cweagans]

I thought about this more last night, and I see where you're coming from. One thing that I'd like to confirm before we go in this direction, though, is how Acquia handles D8 sites at this point. If they write out the right configuration, maybe we should attempt to do the same? Otherwise, conditional includes are probably fine.

[ericduran]

Acquia does the conditional includes:

Here's their code:

  function ah_drupal_core_version() {
    if (defined('DRUPAL_CORE_COMPATIBILITY')) {
      // Only 7.x+ ever actually exists at this point.
      switch (DRUPAL_CORE_COMPATIBILITY) {
        case '8.x':
          return '8';
        case '7.x':
          return '7';
        case '6.x':
          return '6';
        case '5.x':
          return '5';
        default:
          ah_unknown_drupal_core_version();
          break;
      }
    }
    // in D8, the old var got removed:
    // https://drupal.org/node/2067017
    else if (class_exists('Drupal') && defined('Drupal::CORE_COMPATIBILITY')) {
      switch (Drupal::CORE_COMPATIBILITY) {
        case '8.x':
          return '8';
        case '7.x':
          return '7';
        case '6.x':
          return '6';
        case '5.x':
          return '5';
        default:
          ah_unknown_drupal_core_version();
          break;
      }
    }
    else if (function_exists('drupal_init_language')) {
      return '6';
    }
    else if (function_exists('_drupal_cache_init')) {
      return '5';
    }
    ah_unknown_drupal_core_version();
  }

base on that they include D7-prod-nbcupublisher7-settings.inc or D8-prod-nbcupublisher7-settings.inc

[cweagans]

@ericduran @conortm: Okay, what do you think of that? ^

[ericduran]

:+1: :+1:

[conortm]

:+1: