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

Add dependency injection container to Drush. #2018

Merged
merged 19 commits into from
Mar 9, 2016
Merged

Conversation

greg-1-anderson
Copy link
Member

Continuation of drush-di, #1884.

This was referenced Feb 22, 2016
@greg-1-anderson greg-1-anderson changed the title Drush di4 Add dependency injection container to Drush. Feb 22, 2016
@@ -499,7 +499,7 @@ function _core_site_credentials($right_margin = 0) {
function _core_path_aliases($project = '') {
$paths = array();
$site_wide = drush_drupal_sitewide_directory();
$boot = drush_get_bootstrap_object();
$boot = \Drush::getBootstrap();
Copy link
Member

Choose a reason for hiding this comment

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

In D8, this method would usually be called Bootstrap, and not getBootstrap. See the names in Drupal.php ... I'm OK with or without the 'get'.

@greg-1-anderson
Copy link
Member Author

Rebased with master and renamed getBootstrap() to bootstrap().

@greg-1-anderson
Copy link
Member Author

Dependency Hell is getting bad here. There has to be a better way.

@greg-1-anderson
Copy link
Member Author

Drupal 8.0.x uses "symfony/dependency-injection": "2.7.*".
Drupal 8.1.x uses "symfony/dependency-injection": "~2.8".

And what are we going to do about that? Avoiding Symfony DI is not going to be sufficient; if we start using Symfony Console, then we are going to have overlap with other Symfony components. We knew this day was coming, but it appears it is now here. We need to get more serious about site-local Drush, or find another solution.

@greg-1-anderson
Copy link
Member Author

The big change was the release of Drupal 8.1.0-beta1, marked as a "supported" release. This means that Drush dl now prefers the following versions:

$ drush dl drupal --select
Choose one of the available releases for drupal:
 [0]  :  Cancel                                      
 [1]  :  8.0.x-dev    -  2016-Mar-07  -  Development 
 [2]  :  8.1.0-beta1  -  2016-Mar-03  -  Supported   
 [3]  :  8.0.5        -  2016-Mar-02  -  Recommended 
 [4]  :  8.0.4        -  2016-Feb-24  -  Security

Seems we should be taking the recommended release. Perhaps we are not because it is not supported? Compare to Drupal 7:

$ drush dl drupal --select --default-major=7
Choose one of the available releases for drupal:
 [0]  :  Cancel                                                       
 [1]  :  7.x-dev  -  2016-Feb-24  -  Development                      
 [2]  :  7.43     -  2016-Feb-24  -  Security, Supported, Recommended

This means we now test with Drupal 8.1.x instead of Drupal 8.0.x, which is probably okay. This could have unintended consequences for other folks, though. Should track this in a separate issue if anyone thinks it is an issue.

@weitzman
Copy link
Member

weitzman commented Mar 8, 2016

Drush 9 only supports site-local? No global commandfiles, config, etc. No .phar, everyone must learn Composer, ...

Lots to consider.

@greg-1-anderson
Copy link
Member Author

I don't really like completely removing the global Drush either. One thought I had was to make the global Drush handle things like site aliases, and commands such as sql-*, core-rsync and so on that do not require a bootstrap of Drush, and do something else when bootstrapping Drupal is necessary.

The big question is, what is "something else"?

(1) wp-cli has made good use of the WordPress web services APIs to gain access to WP functionality. This is a potential solution, but I do a surprising amount of work on local Drupal sites with no running webserver (relying just on drush rs), so I'm not sure I'd like to start requiring a server for Drush operations.

(2) For commands that are part of a Drupal contrib module, my thought was that we should make those standalone executables, each of which uses the Drupal autoloader. Each cli tool needs its own composer.json file, though, so the tool can declare its own dependencies, and we can give them libraries to use. As you mention, though, not every contrib module needs a composer.json today; this solution forces everyone to use either Composer or Composer Manager to run a cli tool -- but maybe Drush could do some of the work automatically?

(3) I also considered remapping any Drush dependency that overlaps with Drupal, so we'd have our own namespaced copy of everything. This could in theory be done with a Composer custom installer that used info in the extra section to decide which namespaces to rewrite. We'd also have to rewrite use statements, though, and the occasional longhand reference. I don't know that I'd want to maintain this.

(4) Perhaps we should maintain our own autoloader on a per-Drupal-site basis, so we'd have something like DRUPAL_ROOT/drush/vendor, or perhaps DRUPAL_ROOT/../drush/vendor. We could use the mediawiki merge installer to combine the Drush composer.json with Drupal's core/composer.json, or the site's composer.json if it is Composer managed (may require another cli option in addition to --root and --uri to identify a site). This means that each Drupal site would have two copies of everything in its vendor directory, but then we could just load the Drupal-specific autoloader, and have just one global Drush. (Challenge: using aliases to find the Drupal site before we load the autoloader. Might need to exec again whenever an alias is in use.)

(5) Variant of (4), just put our customizations into DRUPAL_ROOT/composer.json, so there's just one vendor, but it includes our stuff in it. We'd need a way to fix up composer.json again if it is overwritten by a Drupal site upgrade.

Not sure what the best way to go is. Leaning towards (2). Not convinced that any are 100% workable without overcoming additional challenges.

@greg-1-anderson
Copy link
Member Author

We can be thankful, at least, that Drupal only seems to update the version of its dependencies occasionally. Drupal 8.0.x is installed with Symfony 2.7.6 components, whereas Drupal 8.1.x is installed with Symfony 2.8.2 components. The conflicting components include:

symfony/class-loader                 v2.8.2  Symfony ClassLoader Component
symfony/console                      v2.8.2  Symfony Console Component
symfony/dependency-injection         v2.8.2  Symfony DependencyInjection Component
symfony/event-dispatcher             v2.8.2  Symfony EventDispatcher Component
symfony/http-foundation              v2.8.2  Symfony HttpFoundation Component
symfony/http-kernel                  v2.8.2  Symfony HttpKernel Component
symfony/process                      v2.8.2  Symfony Process Component
symfony/routing                      v2.8.2  Symfony Routing Component
symfony/serializer                   v2.8.2  Symfony Serializer Component
symfony/translation                  v2.8.2  Symfony Translation Component
symfony/validator                    v2.8.2  Symfony Validator Component
symfony/yaml                         v2.8.2  Symfony Yaml Component

This is the "black list" of things we'd want to avoid (or rename/remap) if trying to maintain compatibility between a global Drush and both Drupal 8.0.x and Drupal 8.1.x. Avoiding symfony/dependency-injection would be entirely doable, but avoiding symfony/console is less palatable. It is possible that symfony/yaml might be resilient enough to survive dependency hell, but I haven't tested this yet.

…here is for the moment (with hopes to fix this later).
…ny 2.7.6, but pin to that more aggressively now.
@greg-1-anderson
Copy link
Member Author

With the latest PR, everything works on 8.0.x, and everything fails on 8.1.x, so only the pm-updatecode test is failing.

@greg-1-anderson
Copy link
Member Author

So, I decided I would go ahead and try a different Dependency Injection container here. As I mention above, this is not a general solution for Dependency Hell, as we remain unavoidably tangled with Symfony / Console (which I presently do not see us avoiding). All the same, a "time bomb waiting to happen" is far superior to "my house is on fire right now". If this works, and the tests all pass, then perhaps this will buy us some time, so that we can move forward with this PR. Reducing the coupling between the code should make it easier to get to a place where we can actually solve Dependency Hell.

There are a lot of PHP Dependency Injection containers to choose from. My initial thought was that we should use something well-known, like Pimple; however, after looking around a bit, it seemed like a good idea to pick one that implemented the container-interop standard. The delegate lookup feature also seemed potentially useful to Drush, although only hypothetically so -- Symfony DI does not seem to implement container-interop, so it is questionable whether we could delegate to Drupal's container -- if we would even want to.

Beyond that, my selection of league/container was fairly arbitrary. Inflectors are a winning feature; wiring up the container using this feature is much nicer than what is necessary with the Symfony DI yaml loader. Note that I deliberately used extend to show how we could allow plugin framework bootstrap classes to wire themselves into the bootstrap manager. Of course, we could always just instantiate the bootstrap manager and pass it to the plugins, and have them call add directly, so I don't know that this little bit of niftyness is strictly necessary -- but there it is.

Hey, the tests just passed. Progress!

@weitzman
Copy link
Member

weitzman commented Mar 9, 2016

Code looks good. We could use a topic about preflight that describes what Drush is doing. All I can see about that right now is docs/bootstrap.md

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.

None yet

2 participants