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

Allow commandfile handling to happen during composer Autoload time #1156

Closed

Conversation

greg-1-anderson
Copy link
Member

This patch moves a minimal amount of the handling of Drush commandfiles out of includes/command.inc, and into classes managed by Composer. This allows Composer projects to add Drush commandfiles while they are being loaded, by adding Drush\Drush::autoload(__FILE__) to the end of their *.drush.inc files (outside of any function).

Why would we want to do that?

In issue #342, I started to clean up and refactor Drush's bootstrap process, so that it would be possible for some other CMS, e.g. Backdrop, to have a backdrop.drush.inc file that added a new Drush bootstrap class in time for it to be used in a (newly added) hook to help identify the Drupal / Backdrop site root. This organization turned out to be really hard, because the dependencies between the different parts of the bootstrap are fairly complicated.

In contrast, Composer is an excellent dependency manager. If a composer-managed extension (e.g. backdrop.drush.inc) is in a library that is managed by Composer, than you can be sure that the Composer autoloader will include the Drush files before the files of the libraries that depend on it. With this patch, we allow the Commandfile handler to accept additions prior to any other initialization that might need to happen in Drush, so that dependent Composer libraries can affect the Drush bootstrap (as permitted by the hooks that Drush provides -- work still needed in this area).

This makes things much cleaner and easier to manage, and also moves us closer to having more code managed in classes, rather than in procedural code.

My vision here is to define a basic Drush API, currently in the class Drush\Drush, and make this part of Drush 7. This API class will bridge the gap between the old procedural code, and the new OO / DI code. Drush\Drush is a big static class; other classes are initialized correctly with DI. We should do most of the OO work in the Drush master branch, after Drush 7 stable is released, but getting this basic starting point into 7 stable will help with the transition from the old to new interfaces, by providing some overlap.

Our new API class should be carefully considered, so that we can be sure that the names we pick are something we can live with for a while.

I have a comment in preflight.inc:

drush_drupal_load_autoloader() uses require_once. Is this going to be a problem if $vendor_path == $drupal_root .'/core/vendor/autoload.php'?

I haven't investigated this yet. I could do this prior to commit if it seems important, but it seems like things are basically working right now, so perhaps nothing actually needs to change here.

Reviewing the diffs in my own PR, I see that the whitespace configuration settings for my new editor are not configured correctly. I'll fix this prior to merging.

@weitzman
Copy link
Member

weitzman commented Feb 8, 2015

How would a Composer managed commandfile get loaded? I get that these files should have Drush\Drush::autoload(__FILE__) in global scope but what calls that code in the first place?

FWIW, here is how Console handles command discovery - http://symfony.com/doc/current/cookbook/console/console_command.html

@greg-1-anderson
Copy link
Member Author

Code in the global scope runs as soon as the file is included or required. In the case of Composer-managed code, this is at the point when the autoload.php file is included. For Drush, this happens very early in the bootstrap, so we know that our commandfiles will be available if they are managed by Composer.

The Symfony Console class requires a lot of infrastructure from Symfony in order to work. For example, you need an app/config/confi.yml with a "console.command" tag to declare that your command is a command. It's unclear to me when this code runs, and if we would be able to get a list of commandfiles early in the bootstrap. I could look into it; I am unsure at this point how much work it would be to adopt that much of Symfony. #88 has been open for a while.

@weitzman
Copy link
Member

weitzman commented Feb 8, 2015

My understanding is that requiring auto load.php just tells Drush where the
classes can be found. It does not load those classes. That would be a
tremendous slowdown to load thousands of classes that may not get used in a
request.

@greg-1-anderson
Copy link
Member Author

You are absolutely correct. I made a mistake -- the inline command is not happening as soon as I originally thought that it was. Just confirmed this with another test.

Need to do more testing, and update this PR. Back later.

@greg-1-anderson
Copy link
Member Author

The continuation in #1165 fixes the problems here. If that PR is okay, this one can be deleted.

@weitzman weitzman closed this Feb 24, 2015
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.

2 participants