Autoload bootstrap #1165

greg-1-anderson · 2015-02-11T01:10:42Z

This is a continuation of #1156. I started a new branch in case I needed to back up, but I'm pretty happy about how this is straightening out.

This patch no longer attempts to rely on Composer for initialization. Doing Symfony Console-like stuff looks like a great idea, but we'll have to tackle this later, in Drush 8. For now, I am maintaining the behavior of the current Drush bootstrap. Commandfiles must exist in the usual magic locations (~/.drush, etc.)

The bootstrap is a lot cleaner now. In drush.php, the main preflight phases are now:

drush_preflight();
$bootstrap = _drush_preflight_root();
drush_preflight_site();

These preflight phases also appear in preflight.inc in this order, and their responsibilities are much more clearly defined. valid_root() is moved to the Boot class.

drush_preflight: Load global config and global command files. Now also handles aliases and 'site-get'
_drush_preflight_root: Find the Drupal root, and decide which bootstrap class is needed.
drush_preflight_site: Find the site inside the Drupal root, and load config and command files associated with the root and site.

So, if you have a commandfile in ~/.drush, it can define a Boot class that is consulted when searching for the Drupal root from the cwd. Once a Boot class is found, Drush remembers it for successive operations, so existing sitealias code & c. still works the same way it always has.

If this looks okay, I think that some of the code from the bootstrap.inc next to DrupalBoot could be moved into includes/bootstrap.inc. Taking the "DRUPAL" out of some of these constants will have to be a Drupal-8 activity.

Oh, and Drush\Drush::autoload() still exists, but now the recommended time to call it is in your drush_hook_init(). I just realized that autoload() is still adding the commandfile; this is innocuous, but unnecessary. I'll remove it. The only purpose of this method will be to include the Drush extension's autoload.php file, if it was not already included as part of the Drupal site's autoload.php.

…ll it from drush_autoload.

…ser libraries can interact with Drush via Drush\Drush::autoload().

… now Drush\Drush::autoload().

…te selection and all site-specific config file loading until after the bootstrap class has been found. Give commandfiles a chance to provide bootstrap candidates, but for now always use the built-in instance.

…ar order. Move valid_root() function into DrupalBoot class.

greg-1-anderson · 2015-02-11T01:18:01Z

The tests passed when I ran on my branch: https://travis-ci.org/greg-1-anderson/drush/builds/50285664

If this is okay to commit, I will rebase and fix the merge conflicts.

weitzman · 2015-02-13T19:13:47Z

includes/environment.inc

      return $candidate;
    }
  }
-  return FALSE;
+  return null;


capitalize here, and above (in the diff)

weitzman · 2015-02-13T19:28:59Z

Left a few comments. I think this looks good. Might have to wait for Drush 8 though. Will chat with Greg.

greg-1-anderson · 2015-02-14T00:09:54Z

drush.php

-  $bootstrap->preflight();
+  // Find the selected site based on --root, --uri or cwd, and
+  // return the bootstrap object that goes with it.
+  $bootstrap = _drush_preflight_root();


This is the part of the PR that is most important to me. This and the corresponding code re-org that goes with it makes the Drush bootstrap much cleaner, and I'd rather not go keep the existing code around for all of the Drush 7 stable branch's life.

greg-1-anderson · 2015-02-14T00:18:41Z

As I said offline, I'd like to get some of this stuff in for Drupal 7 branch for better maintainability. Other parts, like making a Drush\Drush class, is not as important to me, and can wait until later.

The one thing we'd need to figure out, if we punt Drush\Drush and keep the rest, is how the existing procedural code should get a reference to the commandfile class. This sort of thing is usually done with a singleton pattern, but that's not really a best practice in code that uses DI, and I don't know that we want to put in a dependency injection container in Drush 7. That's why I settled for a static class (Drush\Drush) that held our other classes that use modern techniques. Maybe we could just fake this with drush_set_context for now, if you don't want Drush\Drush.

…an use until our DI strategy has been decided.

greg-1-anderson · 2015-02-18T18:19:12Z

In the commits above, I removed the Drush\Drush class, and went with simple procedural access functions. Maybe we can put this in for Drush 7 just as a bootstrap cleanup, and continue with DI stuff after we kick off Drush 8.

pirog · 2015-02-22T01:32:06Z

Giving this a try with Backdrop so we can try to package in "Backdrush" with Backdrop apps for Kalabox2. The custom Boot class seems to load good and proper.

Not as familiar with Drush or Backdrop as i would like to be... but it seems like after getting the custom class to load and writing a good valid_path method for backdrop the task is to write custom bootstrap.inc and command.inc files to actually handle the bootstrapping of backdrop.

Does that approach sound on target to you?

pirog · 2015-02-22T21:55:14Z

So i gave this a shot and it works decently (read: some basic stuff works like drush status or drush uli) well but we are still left with a couple problems.

What i'm using right now is this custom Boot class plus associated files
https://github.com/kalabox/kalabox-app-examples/tree/master/backdrop/config/drush
(you can ignore the aliases)

Plus this hacky hack to get drush to recognize that Backdrop is Drupal 7 compatible.
pirog@5c3099a

Some observations and thoughts here:

Right now this kind of spoofs drush into thinking that Backdrop is Drupal 7 which while somewhat accurate given Backdrops compatibility layer is not wholly accurate nor seems like a good idea.
This seems to produce some weird behavior like the correct "system.module" not getting loaded before drupal/environment.inc (and _7.inc) resulting in failure for a lot of commands like drush pm-list.
```
PHP Fatal error:  
Call to undefined function system_rebuild_module_data() in    
/usr/local/src/backdrush/commands/core/drupal/environment.inc on line 24
```
It is likely this happens with other modules and commands as well.
At the same time drush_drupal_major_version is used like 30 times throughout many command files which is also a problem unless we want to write a bunch of backdrop specific commands of those things which given how similar those commands are likely to be to D7 seems like not the right thing either.
Because of 1. we are able to use drush_include_engine('drupal', 'environment') in our custom Backdrop bootstrap.inc with some degree of success but as i read it maybe we actually want a drush_include_engine('backdrop', 'environment') and/or other Backdrop specific engines.

I'm more than happy to keep working on this but it would be good to get some feedback on these things and ideally a consensus-ish direction on how to proceed.

pirog · 2015-02-22T22:00:57Z

Also let me know if you want me to create a new issue for this!

pirog · 2015-02-22T22:21:39Z

Update over here:

So actually have got this working on Backdrop ... seemed like some commands just didn't have a bootstrap level. Not sure why that is but if you give them FULL BOOTSTRAP everything seems to work.

Of course i dont think we want to blanket full bootstrap all commands that have no bootstrap level specified so going to investigate this a little more and see if there is a better way to add this data in.

greg-1-anderson · 2015-02-22T23:30:34Z

Waiting for Moshe to get back from vacation, to review this and see if it we can get it in for Drush 7.

The Backdrop continuation work is looking good, but I think that this discussion should be continued at #342.

pirog · 2015-02-22T23:55:30Z

Cool cool!

I'll kick these comments over there then!

On Sunday, February 22, 2015, Greg Anderson [email protected]
wrote:

Waiting for Moshe to get back from vacation, to review this and see if it
we can get it in for Drush 7.

The Backdrop continuation work is looking good, but I think that this
discussion should be continued at #342
#342.

—
Reply to this email directly or view it on GitHub
#1165 (comment).

Cheers,

Mike Pirog
Kalamuna
www.kalamuna.com

weitzman · 2015-02-24T20:26:57Z

This looks good to me. Please take one last look at my code comments and see if any are unaddressed. Then, feel free to merge.

Also, I think the title of this PR should be updated as it morphed over time.

greg-1-anderson · 2015-02-25T20:42:19Z

Merged into master. Thanks.

pirog · 2015-02-26T23:11:02Z

Pumped to see this merged!

On Wed, Feb 25, 2015 at 12:42 PM, Greg Anderson [email protected]
wrote:

Closed #1165 #1165.

—
Reply to this email directly or view it on GitHub
#1165 (comment).

Cheers,

Mike Pirog
Kalamuna
www.kalamuna.com

greg-1-anderson added 10 commits February 4, 2015 07:55

Preliminary implementation of drush_autoload() function.

d779126

Factor out drush_add_commandfile from _drush_add_commandfiles, and ca…

785aaa9

…ll it from drush_autoload.

Test drush_autoload

490c17b

drush-ops#572: Move commandfile cache to its own class, so that compo…

d4e8000

…ser libraries can interact with Drush via Drush\Drush::autoload().

Remove drush-autoload branch from travis.yml.

2b831a9

Remove drush_autoload function from environment.inc; this function is…

6c983c5

… now Drush\Drush::autoload().

Move bootstrap object creation into a separate function.

f56c55a

Separate out drush_preflight_site() from drush_preflight(), moving si…

cc9fedd

…te selection and all site-specific config file loading until after the bootstrap class has been found. Give commandfiles a chance to provide bootstrap candidates, but for now always use the built-in instance.

Test the autoload-bootstrap branch

d1ca77a

Straighten out bootstrap, so dependencies are executed in a more line…

6c79252

…ar order. Move valid_root() function into DrupalBoot class.

This was referenced Feb 11, 2015

Allow commandfile handling to happen during composer Autoload time #1156

Closed

Allow users to force Drush to recognize validity of Drupal root and site #342

Closed

weitzman reviewed Feb 13, 2015
View reviewed changes

includes/environment.inc

return $candidate;

}

}

return FALSE;

return null;

Copy link

Member

weitzman Feb 13, 2015

Choose a reason for hiding this comment

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

capitalize here, and above (in the diff)

greg-1-anderson reviewed Feb 14, 2015
View reviewed changes

greg-1-anderson added 3 commits February 13, 2015 16:26

Code style: use NULL instead of null.

bb7abcc

Remove Drush\Drush class; replace with procedural functions that we c…

477ae81

…an use until our DI strategy has been decided.

Remove spurrious constant.

3aef5a2

Remove testing of autload-bootstrap branch in .travis.yml.

f99aadf

greg-1-anderson closed this Feb 25, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Autoload bootstrap #1165

Autoload bootstrap #1165

greg-1-anderson commented Feb 11, 2015

greg-1-anderson commented Feb 11, 2015

weitzman Feb 13, 2015

weitzman commented Feb 13, 2015

greg-1-anderson Feb 14, 2015

greg-1-anderson commented Feb 14, 2015

greg-1-anderson commented Feb 18, 2015

pirog commented Feb 22, 2015

pirog commented Feb 22, 2015

pirog commented Feb 22, 2015

pirog commented Feb 22, 2015

greg-1-anderson commented Feb 22, 2015

pirog commented Feb 22, 2015

weitzman commented Feb 24, 2015

greg-1-anderson commented Feb 25, 2015

pirog commented Feb 26, 2015

Autoload bootstrap #1165

Autoload bootstrap #1165

Conversation

greg-1-anderson commented Feb 11, 2015

greg-1-anderson commented Feb 11, 2015

weitzman Feb 13, 2015

Choose a reason for hiding this comment

weitzman commented Feb 13, 2015

greg-1-anderson Feb 14, 2015

Choose a reason for hiding this comment

greg-1-anderson commented Feb 14, 2015

greg-1-anderson commented Feb 18, 2015

pirog commented Feb 22, 2015

pirog commented Feb 22, 2015

pirog commented Feb 22, 2015

pirog commented Feb 22, 2015

greg-1-anderson commented Feb 22, 2015

pirog commented Feb 22, 2015

weitzman commented Feb 24, 2015

greg-1-anderson commented Feb 25, 2015

pirog commented Feb 26, 2015