Check if site can bootstrap before running updb in drupal:update

[richardbporter]

Fixes #3277

Changes proposed

This change checks to see if the site can bootstrap via drush status (similarly to https://github.com/acquia/blt/blob/10.x/src/Robo/Commands/Doctor/DrupalCheck.php#L51) before running drush updb in the drupal:update BLT command. If the site cannot bootstrap, it logs a warning but continues to attempt database updates on other multisites.

Steps to replicate the issue

The steps originally outlined in #3277 suffice but locally you can replicate this by:

• Creating two multisites locally
  • a.com
  • b.com
• Installing Drupal in b.com
• Run blt auda

Previous (bad) behavior, before applying PR

The previous behavior would cause the blt auda command to fail on a.com. This actually has potentially serious consequences like leaving b.com or any number of other multisites in a wonky state during AC deployments.

Expected behavior, after applying PR and re-running test steps

The blt auda command logs a warning, skips running database updates on a.com and proceeds to b.com.

Additional details

Maybe this should be an opt-in feature via configuration?

Any feedback would be greatly appreciated. Thanks.

[shelane]

We would like to see this as well, but... can there be an option to run the install on sites that don’t have a successful bootstrap? (New multisite and database added to code base)

[danepowell]

Thanks a lot for the PR. I like the overall approach here (checking if a site is installed before running updates on it), but in terms of the implementation, I'd prefer if drupal:update continued to fail or throw an exception if Drupal is not installed. It should be the responsibility of calling functions to ensure that Drupal is installed, and try/catch the exception and fail gracefully (if failing gracefully is appropriate in the calling context).

I think the artifact:update:drupal:all-sites command (in src/Robo/Commands/Deploy/DeployCommand.php) would be a good place to implement this check. If there are other places that need it, it could be moved into an Inspector check.

What do you think?

[richardbporter]

That makes sense but there are two other commands invoked in drupal:update which might also throw exceptions. Should those be handled as well? Maybe those invocations could be moved to the updateSite method of the DeployCommand class. So if drupal:update is successful, it proceeds with drupal:config:import and drupal:toggle:modules.

The auda command could filter out sites that can't bootstrap but that would require another iteration.

[richardbporter]

Ah, just noticed the isDrupalInstalled method on the Inspector class. This Drush task approach is probably unnecessary... 🤦‍♂

Sounds like from #3827 discussion it only works for the default site though. Is that true even if the site context is changed? Like right after: https://github.com/acquia/blt/blob/10.x/src/Robo/Commands/Deploy/DeployCommand.php#L743

[danepowell]

That's a good question, I don't know off the top of my head whether it will change site contexts automatically. But I agree that using the inspector method (although it might require some modifications to work with non-default sites) would be the best approach.

[danepowell]

I confirmed that Inspector::isDrupalInstalled() is not site aware, because it uses a raw executor to run Drush rather than the site-aware DrushTask. So I think the path forward is as follows:

1. Modify Inspector::isDrupalInstalled() to either use the configured Drush URI similar to how Inspector::isActiveConfigIdentical() does, or use $this->taskDrush instead of the executor to run Drush. The latter requires adding the LoadTasks trait to the Inspector: use LoadTasks.
2. Modify DeployCommand::updateSite() to run $this->getInspector()->isDrupalInstalled() prior to running updates, and just skip the site / print a warning if the site is not installed.

It seems like you've got the chops to do this, but if you want help or don't have the bandwidth let me know.

[richardbporter]

I'll give it a shot. Do you prefer I reset this branch, revert the previous commit or just close in favor of another PR?

[richardbporter]

Closing in favor of #3831