This patch identifies the Drupal site that will be bootstrapped very early, in DRUSH_BOOTSTRAP_DRUSH. This re-factoring has numerous benefits.

Note that a reroll of #957182 will be necessary to actually use these together; even though these two patches are designed to be used together, I thought it would be better for review purposes to present them separately.

CommentFileSizeAuthor
#11 drupal-bootstrap-fix.patch2.27 KBgreg.1.anderson
early-bootstrap-id-site.patch14.42 KBgreg.1.anderson

Comments

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Needs work

Awesome! Wish I had thought of this :)

  1. Does this simplify alias loading trickery in sql-sync?
  2. Need to update the topic: docs-bootstrap
  3. Let's make sure the new functions have some Doxygen
  4. Assure that comments start with a Capital letter and end with a period.

After that, this is RTBC if it passes the unit tests.

greg.1.anderson’s picture

greg.1.anderson commented

sql-sync alias trickery is unaffected by this change. As a separate issue, I was considering that if we added metadata to the sql-sync and rsync command record, we might be able to move this same trickery into the command dispatching code, but the bootstrap changes don't help. The trickery is needed in case the site specified in one of the parameters defines aliases that are used as the other parameter. This happens after bootstrap.

I'll add some documentation, and will also review _drush_bootstrap_conf_path and drush_site_path can maybe be unified into a single routine, so that /sites/sites.php only needs to be called once by drush (directly; it will still be called a second time when we call conf_path to bootstrap Drupal).

greg.1.anderson’s picture

greg.1.anderson commented

The unit tests pass in #0 if you have a cached site installed from a previous run, but was broken for a clean test run. Maybe the tests should clear the cache before the first test?

Anyway, I have site-install fixed again, and will commit it as soon as I've rewritten the bootstrap topic. Unifying _drush_bootstrap_conf_path and drush_site_path turned out to be not worthwhile, so I skipped it.

moshe weitzman’s picture

moshe weitzman commented

That's odd that this patch would invalidate the cached sites. I can't think of why that would happen.

My current position is that it is up to the person running tests to clear cache (use `drush cc drush`) as needed. Auto-clearing this cache would slow down tests further.

greg.1.anderson’s picture

greg.1.anderson commented
Status: Needs work » Fixed

The bug occurred when site-install was run with --sites-subdir=dev; because sites/dev/settings.php did not exist yet, the early Drupal site selection code decided that the site to be selected was "default". When site-install later created sites/dev/settings.php and tried to bootstrap, drush remembered that it wanted to bootstrap to "default", which failed. The cache obscured this error because sites/dev/settings.php still existed from a previous run, causing the early bootstrap code to select the correct site. I fixed the bug by having site-install explicitly select the site specified by --sites-subdir in its validate hook.

Documented and committed to master.

owen barton’s picture

owen barton commented

Looks like this patch broke quick-drupal - I managed to work around with the following, but not sure if that is the correct fix - since _drush_bootstrap_select_drupal_site() is underscore prefixed. Greg - could you comment?

@@ -711,6 +711,8 @@ function drush_core_quick_drupal() {
       return drush_set_error('QUICK_DRUPAL_CORE_DOWNLOAD_FAIL', 'Drupal core download/extract failed.');
     }
     drush_set_option('root', $base . '/' . $core);
+    // Reset the bootstrap Drupal context, so it finds the new site.
+    _drush_bootstrap_select_drupal_site();
   }
   if (!drush_get_option('db-url', FALSE)) {
     drush_set_option('db-url', 'sqlite:' . $base . '/' . $name . '.sqlite');
owen barton’s picture

owen barton commented
Status: Fixed » Needs work
greg.1.anderson’s picture

greg.1.anderson commented

Yes, you are correct. The underscore should be removed from that function, since it will need to be called any time a command changes the "state" of the bootstrap -- different Drupal root, new site folder in 'sites', etc.

greg.1.anderson’s picture

greg.1.anderson commented

Actually, let me fix this. I think that if I adjust the implementation of _drush_bootstrap_select_drupal_site(), then I can call it automatically from drush_bootstrap_to_phase, and site-install + quick-drupal won't have to.

Patch to come shortly.

greg.1.anderson’s picture

greg.1.anderson commented
Status: Needs work » Needs review
StatusFileSize
new drupal-bootstrap-fix.patch2.27 KB

Give this a try. All tests pass, but I didn't test quick-drupal. Should work.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

Looks OK at first glance. Can't test now if it fixes the bug.

drush_get_context('DRUSH_BOOTSTRAP_PHASE', 0). That 0 looks odd. Please use DRUSH_BOOTSTRAP_DRUSH or FALSE or something more descriptive than 0.

owen barton’s picture

owen barton commented
Status: Reviewed & tested by the community » Needs work

I tested this and it didn't fix the bug for me - I committed the #7 workaround for now (with a TODO), so quick-drupal works in the meantime.

The patch looks like it _should_ work, so something else must be going on. I might have some time over the next few days to figure out what is going on, unless Greg gets to it first ;)

greg.1.anderson’s picture

greg.1.anderson commented
Title: Identify the Drupal site to be used earlier in drush's bootstrap » Identify the Drupal site to be used earlier in drush's bootstrap for drush quick-drupal
greg.1.anderson’s picture

greg.1.anderson commented
Version: » 8.x-6.x-dev
Status: Needs work » Closed (won't fix)
Issue tags: +Needs migration

This issue was marked closed (won't fix) because Drush has moved to Github.

If this feature is still desired, you may copy it to our Github project. For best results, create a Pull Request that has been updated for the master branch. Post a link here to the PR, and please also change the status of this issue to closed (duplicate).

Please ask support questions on Drupal Answers.

greg.1.anderson’s picture

greg.1.anderson commented
Issue summary: View changes

Put [] around issue reference