BLT 12 checks isAcsfInited() instead of isAcsfEnv() in default.local.settings.php

[benjifisher]

Describe the bug

With Drupal 9 and BLT 12, I have recently run blt recipes:acsf:init:all and committed the results. Now, when I run blt setup -n, I see the following:

blt setup -n
...
> internal:drupal:install
[Acquia\Blt\Robo\Tasks\DrushTask] Running /app/vendor/bin/drush @self site-install myprofile install_configure_form.enable_update_status_module=NULL --sites-subdir=default --site-name='My ACSF Implementation' --site-mail=no-reply@example.com --account-name=3nwOL3QIoj --account-mail=no-reply@example.com --locale=en --no-interaction -v --ansi in /app/docroot
 [info] Executing: command -v mysql
 [info] Executing: mysql --defaults-file=/tmp/drush_INe98z --database=drupal9_ --host=database --port=3306 --silent -A < /tmp/drush_mglprV
...

The database name should be drupal9, not drupal9_. This leads to errors a few lines later, when BLT tries to invoke drush site:install.

Expected behavior

The blt setup command should execute without errors, setting up a local site with the default profle.

System information

• Operating system type: Linux
• Operating system version: Ubunto 20.04
• BLT version: 12.x-dev

Additional context

The unwanted _ character comes from these lines in default.local.settings.php, which were added in 5a4aa87 as part of the fix for Issue #3983:

$db_name = '${drupal.db.database}';
if (EnvironmentDetector::isAcsfInited()) {
  $db_name .= '_' . EnvironmentDetector::getAcsfDbName();
}

After recipes:acsf:init:all, the test isAcsfInited() returns TRUE, and the underscore is added to the database name.

A minimal fix is to replace isAcsfInited() with isAcsfEnv(). I will submit a PR that does this.

If I understand correctly, the BLT strategy for settings files is that the local version should only be used in local environments, not CI and not on Acquia Cloud. If so, then a better fix would be to remove that entire if clause.

[benjifisher]

Come to think of it, both of these methods (isAcsfInited() and isAcsfEnv()) belong in acquia/blt-acsf. That will require more restructuring than I can do as part of this bug fix.

[danepowell]

I think the problem here is that EnvironmentDetector::getAcsfDbName() doesn't work locally. In BLT 11 the db name was determined based on the hostname: 5a4aa87#diff-3f01be0f15533ad5bd0f496e6bec3e74L94

I think we need to bring back that functionality in some form. Not sure if it can live in the environment detector since it's dependent on BLT config.

We'd also need to ensure it's optional, so the db name isn't modified if no site name can be determined.

[benjifisher]

From the closed PR:

This is a local settings file, so it should never be loaded in a hosted environment

That is consistent with my suggestion at the end of the issue description here: instead of the minimal fix in #4230, completely remove the if block.

I will be happy to submit a PR that checks EnvironmentDetector::getAcsfDbName(). That should fix the bug. Would that be good enough for now?

In the long run, if acquia/blt and going to be decoupled from acquia/blt-acsf, then getAcsfDbName() should probably be renamed. Maybe getMultisiteDbName()?

FWIW, checking isAcsfEnv() is not that different from checking getAcsfDbName():

  public static function getAcsfDbName() {
    return isset($GLOBALS['gardens_site_settings']) && self::isAcsfEnv() ? $GLOBALS['gardens_site_settings']['conf']['acsf_db_name'] : NULL;
  }

[benjifisher]

Thanks. I have not tested yet, but I am pretty sure that #4238 will fix this bug.