Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Drush's own bootstrap into drush_preflight() and move rest to new DrupalBoot class #624

Merged
merged 30 commits into from
Jul 16, 2014

Conversation

weitzman
Copy link
Member

@weitzman weitzman commented May 8, 2014

Drupal 8 is about to significantly change its bootstrap (removes early phases), so I decided to refactor Drush in preparation. I will say that we can likely handle D8 within the current Drush bootstrap model so we shouldn't feel obligated to commit this PR. Feedback welcome.

Conceptually, this PR improves the separation of Drush from Drupal. Specifically, a new Drupal "Boot" class now owns the bootstrap process, as I've moved Drush's own bootstrap to a new preflight stage. This separation lets us think more clearly about two potential directions for Drush (not mutually exclusive):

  • Break out Drupal support into a new repo and vendor that in via Composer.
  • Review all the Drush bits with an eye to replacing them with Symfony components like Console, Process, etc. See Leverage symfony/console package #88 and Convert most of exec.inc to use Symfony Process component. #673. Once thats done, Drush is a wrapper around Symfony components. One raison-d'etre for this wrapper is handling redispatch to remote sites via site aliases. Symfony Console doesn't handle that. There are probably other big benefits.

Code changes

  1. Splits the DRUSH_BOOTSTRAP_DRUSH code into a new set of functions called 'preflight' and adds an includes/preflight.inc. drush_main() runs this early on, as before. The DRUSH_BOOTSTRAP_DRUSH constant is deprecated in favor of DRUSH_BOOTSTRAP_NONE.
  2. After preflight, Drush hands control to a configurable "Boot" class. The only such class right now is a new lib/Boot/DrupalBoot class. We could conceivably have other Boot classes for Wordpress, Symfony apps, whatever. Those could be vendored in via Composer. Soon, I think we should specialize DrupalBoot into Drupal7Boot, Drupal8Boot, etc.
  3. The DrupalBoot class is responsible for all subsequent bootstrapping and dispatching. So, the old /includes/bootstrap.inc has been moved to /lib/Boot and "belongs" to Drupal.
  4. After dispatch, Drush resumes control as usual with its postflight and shutdown code. No changes here.

Known issues

  1. During preflight, we no longer find aliases/commandfiles/config that are located in [DRUPAL_ROOT]/drush, [DRUPAL_ROOT]/sites/all/drush, and [DRUPAL_ROOT]/sites/[URI]. That happens later during DRUSH_BOOTSTRAP_DRUPAL_ROOT and DRUSH_BOOTSTRAP_DRUPAL_SITE phases. A byproduct of this is that any remote aliases defined there are found too late for redispatch via Drush's backend. This is why testContextHierarchy is failing. We could mitigate this by:
    1. Do nothing. Anyone who wants redispatch from these dirs can specify them with --alias-path (@see Grayside trick)
    2. Let DrupalBoot suggest directories to search during Drush preflight
    3. Add redispatch ability during ROOT and SITE phases in Drupal bootstrap.
  2. testComplete (i.e. bash completion test) is failing for some reason. Likely fixable.
  3. There are still quite a few bootstrap concepts within the Drush code. We can move those into DrupalBoot over time
  4. Drupalboot class should be refactured in future so that it isn't a horrid mix of OO and procedural code.

register_shutdown_function('drush_shutdown');

// Not used?
// drush_set_context('DRUSH_BOOTSTRAP_PHASE', DRUSH_BOOTSTRAP_NONE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DRUSH_BOOTSTRAP_PHASE is used a couple of places to see if a Drupal site has been selected, etc.; might be better to provide an API for this, and discourage or prohibit folks from looking at the bootstrap phase directly.

@greg-1-anderson
Copy link
Member

Direction looks great. I haven't tried it, but I'm in favor of committing and pushing forward from here once tests are cleaned up.

@weitzman
Copy link
Member Author

weitzman commented Jun 6, 2014

@grugnog - when trying to fix --early handling, you can just add commits to the drupalboot branch. Thanks.

@grugnog
Copy link
Member

grugnog commented Jul 6, 2014

The issue was that separate calls to the alias and command-name caches were causing a cache miss (which triggered a full rebuild) followed by a later cache hit. We validate cache misses though asserting that no hit's are present, and hence this was breaking the tests. To resolve this, I simply added a static cache to the function, so it no longer attempts to load a cache after it has already rebuilt the main completion array.

In other words, I think the tests should have been failing here, and this patch just uncovered the actual issue. The follow-up question then would be - why are the tests passing on master when they shouldn't be? Looking at the output it seems like something may have regressed with caching on master (but fixed here), but it needs further investigation.

@weitzman
Copy link
Member Author

weitzman commented Jul 7, 2014

Thanks @grugnog.

@greg-1-anderson - Any chance you can fix the merge conflict that this PR now has? The way to do that is to run git checkout drupalbook && git rebase master. Fix conflicts and commit+push the result. I think you are the ideal person to do this since the conflicts pertain to your recent improvements.

@greg-1-anderson
Copy link
Member

I think that should do it -- but I'm not staying up to see if the Travis build passes. :)

(For some reason, while I can run individual tests locally, I get false failures when I try to run all tests at once. Haven't spent the time to try to figure out why that is yet.)

@greg-1-anderson
Copy link
Member

Not quite right. drush @self anycmd is failing. Missed @self initialization somewhere in the bootstrap, I guess. More later.

…etting uri via cwd still does not happen until the 'bootstrap root' phase, after the kind of site (still only Drupal supported) is known.
@greg-1-anderson
Copy link
Member

I pushed another commit that insures that root and uri are set earlier in the bootstrap. This still isn't quite enough to fix everything, as commands such as 'status' and 'site-alias' (anything that only goes through the 'preflight' / formerly known as drush_bootstrap_drush) will dispatch before the code that sets uri from cwd is executed. This is pretty close to right, as drush_bootstrap_max will still do the right thing, but it means that drush @self status and drush sa @self do not work if uri is detected from cwd. This is a fairly fine distinction (c.f. 1. i. ii. and iii. in OP), so this might just be good enough for now.

Some more refactoring of bootstrap responsibilities could be done. Also, I moved _drush_bootstrap_select_drupal_site() to preflight, so this method could perhaps be renamed. I'm okay with either continuing to work with this on the branch, or just committing to master & continuing the cleanup there. We'd just need to touch up the tests to "fix" the known failures.

@weitzman
Copy link
Member Author

I configured .travis.yml so that it starts testing this branch. I'll help fix the fails. Thanks for the effort here, folks.

@weitzman
Copy link
Member Author

Since we are processing --uri and --root during preflight, I'd be OK with doing the cwd discovery of uri as well during preflight.

@greg-1-anderson
Copy link
Member

The thing about uri discovery is that it requires Drupal-specific code. If we want to leave ourselves open to (a) uri discovery that runs Drupal code, (b) uri discovery that is different in D7 & D8, or (c) support for other CMS systems such as Backdrop, Wordpress, etc., then we should:

i. Load all of the bootstrap classes in preflight
ii. Ask each bootstrap class if the selected root can be handled by it
iii. Give the bootstrap class that claims the root a chance to do preflight operations (like find uri)

Then bootstrap as we currently do. I think this would be worth doing. The main prereq would be to move all of the lib/Drupal/Boot/bootstrap.inc code into a class, and invent some easy heuristic for Drush to find bootstrap classes. Maybe we could do this after we load the initial Drush commandfiles late in the preflight, and call a commandfile hook to get bootstrap class names (or just have it instantiate and return a bootstrap class).

@greg-1-anderson
Copy link
Member

This PR implements #342

@weitzman
Copy link
Member Author

We are now green, with barely any changes needed. I'll merge this in the morning - too tired now.

In 5d63fcd I removed the call to _drush_bootstrap_select_drupal_site() during every bootstrap phase. That's now done only during the SITE bootstrap phase as I didn't see a use case for arbitrary changes to URI and we were actually resetting uri to NULL in one test fail that I debugged.

weitzman added a commit that referenced this pull request Jul 16, 2014
Refactor Drush's own bootstrap into drush_preflight() and move the rest of bootstrap into a new DrupalBoot class.
@weitzman weitzman merged commit 971f6c9 into master Jul 16, 2014
@weitzman
Copy link
Member Author

I've merged this PR. Let's keep cleaning preflight and DrupalBoot until they look pretty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants