Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
We need a PSR-0 class auto-loader for Drush so that we can simplify the bootstrap process as all code would be autoloaded when needed.
Proposed resolution
- Bring in Composer to generate an autoload.php file for us
- Use Composer's dependency system to install any third-party libraries (HTTPServer, Console_Table, etc)
- Have Drush instantiate a Composer installation on first-run
Follow up tasks
Once the patch is committed, we can...
- Simplify the bootstrap by moving files over to PSR-0 autoloading classes
- Have any new Drush dependencies download automatically for us without us having to micro-manage them
- Instead of including Composer directly, have Drush download it for us?
- Possibly switch from using Composer's Phar file to the source directly?
- Introduce a "drush composer" command to allow further use of Composer via Drush?
User interface changes
- When running Drush for the first time, the user is told that the dependencies are being installed. This is a one-time process and the user won't be prompted about it again.
- There is a new command named
drush composer
, which allows direct use of Composer.
API changes
drush_bootstrap_prepare_dependencies()
is called during the bootstrap to make sure all of Drush's dependencies are met. It then loads the class loader to make sure all classes can be autoloaded.- Drush's PSR-0 classes can be placed in
drush/lib/Drush
.
Original report by msonnabaum
Here's an initial patch to add a PSR-0 autoloader to drush, which is based on symfony's.
To test it I've only moved over the cache classes, but that appears to be working for me. This also removes php 5.2 support.
Comment | File | Size | Author |
---|---|---|---|
#73 | 1316322.patch | 14.53 KB | RobLoach |
#71 | drush-composer-1316322-71.patch | 13.99 KB | msonnabaum |
#70 | drush-composer-1316322-70.patch | 13.46 KB | msonnabaum |
#69 | 1316322.patch | 170.33 KB | RobLoach |
#68 | composer.patch | 169.37 KB | RobLoach |
Comments
Comment #1
msonnabaum CreditAttribution: msonnabaum commentedIgnore the last patch. Missing some files.
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedLooks good. Just revert the drush_register_file_for_deletion() change in favor of unlink().
Comment #3
Steven Jones CreditAttribution: Steven Jones commentedAwesome stuff, I'm coming from the Aegir project, where we really need some kind of autoloader for our classes.
It would seem that because the way that this has been coded, the only classes that can be loaded by this autoloader would be in the Drush namespace, and if say, a contrib drush extension wanted to make use of classes, the way this code is impling it should be used would be to spin up another instance of the autoloader, registering its own namespace, rather than somehow adding its namespace to the drush autoloader? Is that the intention?
We're considering a major re-working of a lot of our code in Aegir's backend because we aren't extendible at all, contrib extensions must mix their code in with ours, ideally, we could just register with some central registry, and ideally this central registry would be provided by Drush.
Ideally, what would be awesome would be a hook that a commandfile could implement to register namespaces with drush, is there such a hook already? I guess a simpler solution would be to have a central way to register namespaces, and then commandfiles could just call it in their main body of code, which would get loaded as soon as Drush loads it.
Comment #4
Crell CreditAttribution: Crell commentedRenaming/hacking the autoload class is a bad idea. That's just going to confuse people. It also makes proper attribution for licensing purposes harder. Just do as core is doing and include the code verbatim, in a properly namespaced folder, and wire it up with the code from bootstrap.inc.
Comment #5
Steven Jones CreditAttribution: Steven Jones commentedI've been dying to get something implemented in Provision, so as you can have multiple autoloaders just fine, I've added the following:
Into our dev branch, note that we're using class prefixes and a slightly modified version of the autoloader so we can be PHP 5.2 compatible.
With the above code we can do things like this, in http.drush.inc:
Which is quite nice. Anyway, it would be cool if Drush provided some of this for free, but actually, it's not that much code, but it seems like a shame to include an autoloader but lock it away so no-one else can benefit from it.
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commentedComment #7
msonnabaum CreditAttribution: msonnabaum commentedI've been working on the core version of this, and how modules will register namespaces over here:
http://drupal.org/node/1290658#comment-5450054
Once that patch lands I'll re-roll this using the same technique.
Comment #8
greg.1.anderson CreditAttribution: greg.1.anderson commentedUsing the unmodified class loader would be a great idea per #4, but I am still concerned about conflicts with the Symfony universal class loader in Drupal core/includes. Drush may need to use the class loader before Drupal is bootstrapped, but if we require_once the same UniversalClassLoader.php from a different location, then php will die when the second class loader is included.
Post #1, the Drush bootstrap was rewritten to locate the location of the Drupal site that will be loaded very early in the bootstrap process.
_drush_bootstrap_select_drupal_site()
is called before every Drush bootstrap phase. So, the location of the Drupal root is set prior todrush_bootstrap_drush()
, but might change again just prior todrush_bootstrap_drupal_root()
if a config file changes --root, or if a site alias is selected bydrush_sitealias_check_arg()
.I propose that for Drush 6 we move the call to
_drush_bootstrap_select_drupal_site()
such that it is called once, somewhere indrush_bootstrap_drush()
afterdrush_sitealias_check_arg()
. At this point, we will consider that the selected Drupal site has been permanently fixed, We can then load the class loader directly fromDRUPAL_ROOT . '/core/includes/Symfony/Component/ClassLoader/UniversalClassLoader.php'
, or load our own copy if no Drupal site has been selected, or if there is no selected site, or if Drupal does not include Symfony. I do still worry that we might not be able to find the class loader if it has been installed by the symfony module, since it does this:If we can reconcile this, then we should not need to rename the autoloader. Perhaps we should hop over to the symfony module issue queue, and suggest that Symfony should go in sites/all/libraries?
Comment #9
moshe weitzman CreditAttribution: moshe weitzman commentedWe discussed mapping all our namespaces in a composer.json in root of drush. one of us would then use composer tool to fetch all our classes and put then in /vendor. composer would also write out an autoloader for each listed class.
For contribs we would discussed registering a mount point in a /lib subdir beneath each commandfile.
Mark could explain it better.
Comment #10
greg.1.anderson CreditAttribution: greg.1.anderson commentedMaybe I don't understand symfony well enough, but why do we need an autoloader for each class?
Is the problem with #8 that Drupal might use a different version of Symfony than Drush was using? That could be a problem. Is the composer tool renaming or renamespacing all of the vendor classes to make sure that doesn't happen? Not sure that I am clear on all of the implications yet.
Comment #11
moshe weitzman CreditAttribution: moshe weitzman commentedSorry, I meant one autoloader that knows how to load all the used by core drush.
Comment #12
greg.1.anderson CreditAttribution: greg.1.anderson commentedWill Drush ever use the Symfony that is included with Drupal (c.f. symfony module in #8), or will we use our own private copy?
Comment #13
Crell CreditAttribution: Crell commentedIn Drupal 8 I don't see why it couldn't use the core class loader. If you want to maintain Drupal 7 compatibility, though, you'd need to include your own since Drupal 7 has no PSR-0 capable class loader.
Comment #14
greg.1.anderson CreditAttribution: greg.1.anderson commentedIn Drupal 7, there is a symfony module that will load Symfony. If running Drupal 8 or Drupal 7 w/ symfony, Drush must either use Drupal's copy, or rename / renamespace all of the Symfony classes. It seems to me that the former is the right approach, but I don't understand all of #9 yet.
Comment #15
greg.1.anderson CreditAttribution: greg.1.anderson commentedc.f. http://drupal.org/project/composer
This should probably be in Drush core too.
Comment #16
msonnabaum CreditAttribution: msonnabaum commentedHere's a start on using composer for our psr-0 autoloader. It also adds all our dependencies for us which is nice.
I included composer as a library as well, so we can invoke it directly to download the dependencies on install. That code isn't written yet, but with this you can run composer install and see how the autoloader works at least.
Comment #17
greg.1.anderson CreditAttribution: greg.1.anderson commentedForgot the patch in #16?
Comment #18
RobLoachDefinitely is interesting... Composer and Drush Make do have similar paths, but their feature set is quite different. Using Composer's ClassLoader for this, as well as using it to bring in the additional dependencies makes almost perfect sense. Would we be packing the dependencies with Drush itself?
Comment #19
msonnabaum CreditAttribution: msonnabaum commentedI'm dumb. Here's the patch.
Comment #20
greg.1.anderson CreditAttribution: greg.1.anderson commentedVery slick. The only flaw is interoperability with symfony in Drupal. See #8.
I think we need to do something like this:
We'll also need some code to check to see if we are running under Drupal 8. Hm, of course there is an obvious flaw in the code above; we will need to require_once the autoloader before we bootstrap Drupal, but drupal_get_path is not going to work until Drupal is bootstrapped. Postponing initializing the autoloader until after bootstrap would mean that we could not use any symfony features during bootstrap. Since we were thinking about using Symfony for the new context class, we have another chicken-and-the-egg problem, because we then need to initialize Symfony before we load our Drush configuration files, but Drush cannot figure out where Drupal is until it has loaded said configuration files, since they may have settings which affect site selection.
Right now, the only general-purpose solution I can think of it to add code to Drupal 8 and the Drupal 7 symfony module to check and see if the autoloader has been included. A better solution, I think would be to require PHP 5.3.0 or later for Drush-6, and restrict Symfony use to Drush-6 and later. If the autoloader is provided by php, then Composer will make sure that we do not have code conflicts for any other modules. Then we should interoperate with Drupal just fine.
My only remaining question about Symfony and Composer is, who tells Composer to load Drush's composer.json?
Comment #21
RobLoachNot sure Drush should depend on the Symfony module since that's a Drupal module rather than a Drush component. It's possible for Drush to download and install its own dependencies via Composer during the Drush bootstrap. We could either ship composer.phar, or have it automatically lazy-load download it if composer.phar isn't found. If we do that, then we can install Drush's dependencies directly through the .phar file:
What I'm suggesting here is....
Comment #22
greg.1.anderson CreditAttribution: greg.1.anderson commented#21 would work just fine iff the symfony module and Drupal 8[*] were changed to also use
include('phar://' . $composer . '/vendor/.composer/autoload.php')
(or include_once). If Drush runs code per #21, and then Drupal comes along and trys to again load the autoloader per the code in the symfony module (see #8), then PHP will throw an error, because the same code is being loaded from two different paths. The different paths defeats_once
, so you get duplicate function errors.[*] Haven't looked at the D8 code. If it requires PHP 5.3, and does not load the autoloader, then perhaps there is no conflict there.
Comment #23
Crell CreditAttribution: Crell commentedD8 requres PHP 5.3, and at the moment uses the Symfony ClassLoader for any namespaced code. Switching to Composer is under consideration: #1424924: Use Composer for updating Symfony components (without removing Symfony code from repo) and #1398772: Replace .info.yml with composer.json for extensions.
I don't think any changes are necessary for phar:// support, is there? It should Just Work(tm) in PHP 5.3.
Comment #24
RobLoachThe attached patch...
drush composer
commandWould be nice if we...
Comment #25
RobLoachMoved to Composer's provided Console_Table and updated the folder docs.
Comment #26
Owen Barton CreditAttribution: Owen Barton commentedIf we are going to manage Console_Table through Composer, it seems like we should do the same with httpserver also (I would be happy to figure out how to package it and post it to one of the supported repositories, if snagging direct from github is not Composer supported).
Comment #27
RobLoachGreat idea! Got both the Console_Table and the HTTPServer autoloaded via the Composer autoloader. Needed to switch from the PEAR downloader to SVN though, which actually seems speedier and more responsive.
Comment #28
greg.1.anderson CreditAttribution: greg.1.anderson commentedI'm still concerned about this part:
I expect if two components (Drush and Drupal) try to include autoload.php via two paths, then the second one to run will fail. Haven't had a chance to try it yet, but
drush @site status
should do the trick if @site is D8 -or- D7 w/ the symfony module enabled. Anyone try that yet? If it works, we're good.n.b. Don't really know what's meant by 'the composer source'; perhaps that is the solution to this problem?
Comment #29
Crell CreditAttribution: Crell commentedComposer like any other PHP project is, um, a bunch of PHP files. :-) It's compiled into a phar file (kind of like a Java JAR file) for convenience, but works either way. I presume the todo there is to replace the phar file with the original raw PHP files.
That said, I do not understand the rest of #28. What's going to break and why, exactly?
Comment #30
greg.1.anderson CreditAttribution: greg.1.anderson commentedSuppose you have a file 'foo.php' that defines a single function a() that just prints "hello".
Then:
The second call to include_once behaves correctly; foo.php is already included, so it is not included a second time. The third call to include_once fails; if you have a copy of foo.php at a different path ("b/foo.php"), include_once presumes that this must be a different foo, so it goes ahead and includes it again. You get an error, because the function a() is already included.
So, if we
include_once(".../autoloader.php");
, then we need to make sure that any other code that also callsinclude_once(".../autoloader.php")
uses exactly the same path. Otherwise, php will throw 'cannot redeclare' errors.Comment #31
greg.1.anderson CreditAttribution: greg.1.anderson commentedOh, I just tried it, and it works just fine. There is no problem here... because Symfony rocks, of course. The file autoloader.php defines no functions itself; it is designed to be included again and again, and therefore already has a check in place to insure that it does not bomb out on its second invocation. This only makes sense, as it would be kind of hard to resolve issues such as #28 / #30 if you could only include the autoloader once, because it needs to be possible for components to be able to 'bootstrap' symfony independently without getting in each other's way, or require some global at the application layer, etc.
The key bit from the autoloader looks like this:
So, +1 for #27. It works great, and I see no reason why it would cause problems in Drush-5, either on its own, with Drupal 7 + symfony module, or Drupal 8 (although I did not try the last).
Comment #32
msonnabaum CreditAttribution: msonnabaum commentedMostly great. Just a couple of notes.
I was surprised to see that Console_Table doesn't get autoloaded in the version I did. I'd like to have it autoload, but I really don't want to switch to getting it from svn. The install is slow enough as it is. Can we not add the autoload info to the way I originally defined it?
I'm confused about what we're doing with the composer command. We aren't using it internally yet and I'm not seeing the advantage of using it instead of calling composer directly. I'd rather remove it for now until we have a good use case.
I'm not crazy about everything happening in drush_bootstrap_prepare_dependencies(). It feels like that could be abstracted better. Maybe calls to the composer command? I also really don't like the chrdir, and if that's necessary, let's see if we can fix it in Composer.
Comment #33
greg.1.anderson CreditAttribution: greg.1.anderson commentedAnother thing: this patch also commits Symfony code directly to the Drush project's repository; the license is GPL-compatible, although not GPL. Drupal policy says GPL-only here: http://drupal.org/node/66113
Perhaps this is changing, with Symfony going into Drupla 8 core?
Comment #34
msonnabaum CreditAttribution: msonnabaum commentedDocs may be out of date. We've already committed Symfony code to core: http://drupalcode.org/project/drupal.git/tree/HEAD:/core/vendor/Symfony/...
Comment #35
greg.1.anderson CreditAttribution: greg.1.anderson commentedI suggested in #15 that the composer drush command should probably be in core because the symfony module for D7 uses it to install symfony in Drupal. I imagined that other modules might also start doing the same thing. An alternative, though, is that we could submit modifications to the symfony module & elsewhere to automatically use composer (via direct php call, not a drush command) in their pm-enable pre-enable or post-enable hooks. With a little sample code, and a little more adoption, perhaps the composer drush command would not be needed very often. Then again, I don't really have a good answer for why folks thought that the composer drush command was needed at all, when composer already has a command line interface. Shrug.
Comment #36
Crell CreditAttribution: Crell commentedRob Loach thought a composer Drush command was needed. I disagree. :-)
Comment #37
msonnabaum CreditAttribution: msonnabaum commentedMy preference is for people to invoke composer directly when they have a composer.json rather than hide it with a drush command. I really want to promote the adoption of composer in the Drupal community and there's still a lot of confusion around what it even is, so I'm hesitant to wrap it in any way aside from our own internal use.
I do think there's opportunity for a deeper integration into drush, but it will require some more thought and it's beyond the scope of this issue.
Comment #38
RobLoachThings that this patch addresses...
drush composer
command to a post-issue. I didn't think a Drush composer command was needed, just thought it would be nice :-P.Things that this patch does not address...
drush_bootstrap_prepare_dependencies()
. We're using the Composer API directly here, so I'm not sure how to make it easier.chdir()
. Composer uses the current working path by default. It would be nice to be able to change that. Maybe https://github.com/composer/composer/issues/55 ? I was trying "config": { "vendor-dir": "vendor" } without success. I created https://github.com/composer/composer/issues/503 so that we arn't blocked by this.Comment #39
RobLoachComment #39.0
RobLoachUpdated issue summary.
Comment #40
moshe weitzman CreditAttribution: moshe weitzman commentedWhat happens when we decide we need a newer version of httpserver, for example. In current Drush, we will automatically download a new version. I don't see that this happens with this patch.
Do we want to say anything about composer in Drush's README.txt?
I'd like new issues opened for the Follow Up tasks in the Summary or we remove items that no longer need Follow-up.
I don't completely get why folks are so excited about Symfony. Not that it matters, but I think that code like below has very poor readability:
Comment #41
RobLoachGood call, Moshe. We should cache the installed .json file somewhere, and run a Composer update when the hash is changed.
I agree that we should move to use statements in there. Do you think we should have this stuff in a composer.inc file or something just to help decouple it from the bootstrap?
Comment #42
greg.1.anderson CreditAttribution: greg.1.anderson commentedDoes php / Symfony have anything like Java's import statement? Seems that a lot of these features parallel Java language features; not all of these are my favorites either. But anyway, with imports, the above could become:
I have to admit, even with this simplification I am not terribly excited by constructs like
new ArrayInput(array('install'))
just to pass an 'install'command to composer. :?Comment #43
RobLoachMaybe when we bring in the
drush composer
command. There isn't currently anything user-facing with this patch. Do you think we should make a note about using it to install the dependencies?Comment #44
greg.1.anderson CreditAttribution: greg.1.anderson commentedThat's a great improvement. I don't think that we should ever do this, though:
There are presumably a lot of classes that have Factories;
new Factory(...);
is just not explicit enough. I think that this should always be written asnew Composer\Factory(...);
. We could useuse
statements for everything else. (Coding standards for OO Drupal needed...)Comment #45
RobLoachTo avoid class name conflicts, you can do:
But we don't have any other Factory classes anywhere. There arn't any other Factorys that we're working with in
composer.inc
. The use statements are going into Drupal 8. Are there any other third-party libraries that Drush makes use of that we could push into Composer?Do you think that we should have Drush download Composer rather than shipping Composer.phar with it?
Comment #46
greg.1.anderson CreditAttribution: greg.1.anderson commentedI don't want to bikeshed this discussion too much. Either
use Composer\Factory as ComposerFactory
ornew Composer\Factory(...);
would be fine. Drush should follow the OO coding conventions established by Drupal, but those are still formative right now. I think it is safe to say that if we start adopting OO, then we will have multiple factories in some files, and there should be a convention for keeping it readable. But that's my opinion; others may feel that$composer = Factory::create(...)
is clear enough, as the variable name indicates what kind of Factory is intended.As for downloading Composer rather than including it, this should also be decided per Drupal policy. If the policy is unclear, downloading is safer, as it conforms with the historic policy. Per #34, it seems that non-GPL code is being committed to d.o already, so I think it's probably safe for us to follow suit.
For my part, I am fine with committing #43 as-is, although I would sort of like to see one of the Composer\Factory alternatives for clarity. Up to Moshe.
Comment #47
RobLoachSounds good with me! I did it for the Installer too, just for consistency.
Comment #48
greg.1.anderson CreditAttribution: greg.1.anderson commentedI feel a lot better about #47; thanks. The code works well, and correctly calls the installer when the composer.json file changes. The README could, perhaps, use some credits info for symfony / composer, but if we did that, it seems we should also put something in for the http server and console table.
Comment #49
greg.1.anderson CreditAttribution: greg.1.anderson commentedWait, I'm not sure that this is right. If I change composer.json, and then change it back (with Drush runs between each step), then I get this message:
Your lock file is out of sync with your composer.json, run "composer.phar update" to update dependencies
Is composer supposed to leave the composer.lock file sitting around? It seems like the lock file is supposed to allow composer to take care of updates automatically, but it takes a really long time to call the installer every single time Drush runs. Maybe we should run a composer update command instead of a composer install when the composer.json hash changes?
Comment #50
RobLoachAh, I see what the problem is. Instead of using the ArrayInput(), we should be interacting with the installer directly. This actually gives us more control over how the installation process works.
Thanks for finding that one, Greg!
Comment #51
RobLoach--binary
Comment #52
greg.1.anderson CreditAttribution: greg.1.anderson commentedThat works brilliantly. Nice to see the ArrayInput going away; the new code looks much much better.
Comment #53
RobLoachIf we used the Composer sources rather than composer.phar, do you think this could be committed?
Comment #54
greg.1.anderson CreditAttribution: greg.1.anderson commentedI'm presuming you were talking with Moseh in IRC. I suppose it is less than ideal to have composer checked in to our repository; I don't think that checking in the source instead of the phar makes this aspect any better.
What if we ran
curl -s http://getcomposer.org/installer | php
instead? That's from the composer README file; seems that's better than using git, which might not be available (e.g. on Windows under DOS or Powershell). This would be pretty easy to do using drush_shell_cd_and_exec; that would take care of having composer embedded inside of Drush.Also, the README also advises that
php composer.phar self-update
can be used to get a newer version of Composer. I wonder if there is a time when we should do this? Maybe we put it off until Drush needs a newer version of Composer to download something; then we could add a Composer version check + self-update at that time.Comment #55
RobLoachcurl -s http://getcomposer.org/installer | php
just retrieves composer.phar and saves it to the current working directory with permissions so you can run./composer.phar update
. If we were to have Drush download composer.phar rather than ship composer.phar with Drush, we'd do something similar to make sure the phar file was available.I'm just trying to understand if there were any blockers here. Was wondering if using the Phar file was the problem. Using composer.phar directly does have its benefits, but if it's a blocker for getting this in, then we could use the source directly.
Comment #56
greg.1.anderson CreditAttribution: greg.1.anderson commentedAs I said in #54, I have no preference vis-a-vis committing composer.phar vs. committing composer sources to the repository. Committing a binary is a little dirtier than committing sources, but either way, you're still committing an external component, which in my mind is the greater problem.
If we're going to download rather than check in composer, I similarly don't think it matters if we download the phar file or the sources; downloading the phar seems better to me, if we go this route.
Which of these issues, if any, are blockers is up to Moshe.
Comment #57
katbailey CreditAttribution: katbailey commentedActually, running
curl -s http://getcomposer.org/installer | php
also ensures that php is configured correctly for composer to be able to run.Adding the composer source to the drush repo definitely sounds like the wrong approach to me (would that even work? Doesn't composer itself use a composer.phar to get its own dependencies or is that my brutal misunderstanding of this repo https://github.com/composer/composer?).
Comment #58
msonnabaum CreditAttribution: msonnabaum commentedMy preference has always been to include the source. I'm not interested in downloading composer for a user to be able execute independently of drush, I just want to treat it as a dependency to be used internally.
Using the phar is probably simpler, but I've been pretty turned off from phars in general after running into this bug: https://github.com/composer/composer/pull/201.
We're committing symfony sources to drupal, I don't see why it's an issue to commit composer to drush. I'm also not opposed to downloading it as long as we aren't trying to run the installer.
Comment #59
RobLoachThree possible methods:
I'm happy with whichever solution we go with.
Comment #60
katbailey CreditAttribution: katbailey commentedThis isn't a foregone conclusion - see #1424924: Use Composer for updating Symfony components (without removing Symfony code from repo) and #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead.
Comment #61
msonnabaum CreditAttribution: msonnabaum commentedUsing composer to manage core and core shipping with symfony components are two different issues.
I think it's likely we'll get a composer.json in, but I doubt the symfony code will get removed.
Comment #62
katbailey CreditAttribution: katbailey commented@msonnabaum yes, that's why there are separate issues for them in the core issue queue. My point was that it doesn't make sense to justify adding composer/symfony source to drush on the basis that we're adding symfony source to core, because the latter is not necessarily true.
Comment #63
RobLoachHere it is with the Composer source, including dependencies, instead of the .phar file.
Comment #64
RobLoachForgot --binary again.
To commit it, you use:
Comment #65
chx CreditAttribution: chx commentedNo, don't skip .phar use #1185360: Refactor code loading and support loading from PHAR archives
Comment #66
RobLoachThis is for Drush, not Drupal. The patch using the .phar file is at #51.
Comment #67
RobLoachHere's patches with the phar file, or with the sources. Your choice!
Off topic: Forked Console_Table for Composer support for funsies.
Comment #68
RobLoachThought I'd do an update of the patch.
Comment #69
RobLoachTalked with the Console_Table maintainer, and he agreed that having both PEAR and Composer support for Console_Table was a good idea. End result is that Drush can download Console_Table much faster as it doesn't have to go through PEAR.
Comment #70
msonnabaum CreditAttribution: msonnabaum commentedHere's a slightly different approach.
This patch contains no vendor source, phar or otherwise. It adds composer back into the composer.json so we can treat it as a proper dependency.
To solve the problem of composer not being available to install itself, it downloads the composer.phar to a tmp dir to perform the initial install. After that the autoloader and all sources we need are in place.
Comment #71
msonnabaum CreditAttribution: msonnabaum commentedHere's a new version with the vendor dir in .gitignore, docblocks, and I removed a drush_set_error() since it isn't available at that point.
Comment #72
Crell CreditAttribution: Crell commentedEh? Not if it's hosted on d.o, it's not.
Comment #73
RobLoachComment #74
patcon CreditAttribution: patcon commentedtagging
Comment #75
katbailey CreditAttribution: katbailey commentedSo... I love this version of the patch - so clean and lovely.
When you run a drush command for the first time after updating, would it be possible to have some messaging to the effect that "Drush is performing a self-update, this may take a few seconds. This is necessary because your version of drush has been updated but its dependencies have not yet been updated."? Otherwise you're kinda left hanging there after just running drush --version or whatever, and then after a while it starts doing all this crazy shit, installing stuff, and well... it could cause some confusion I think.
Also, should we consider including the composer.lock file for the same reason we're considering including it for core, i.e. so that the composer.json could reference dev versions while ensuring that drush end users all have the exact same versions of the dependencies?
Anyhoo, would be awesome to get at least *some* of the Composer love consummated some time soon... :-)
Comment #76
patcon CreditAttribution: patcon commentedNice! Looks awesome. Not sure why vendor dir isn't in the root .gitignore anymore, but it's all good.
Not sure why I'd get errors when you didn't, but I think the
"classmap": [ "" ]
needs to be"classmap": [ "." ]
. At least it did when I was playing with Composer beforeComment #77
msonnabaum CreditAttribution: msonnabaum commentedI think it's probably ok to have the composer.lock in there. Especially if it would let us get rid of the md5 business that's there now. I wasn't crazy about that when I saw it but didnt have a better idea.
Comment #78
moshe weitzman CreditAttribution: moshe weitzman commentedI'm on the fence about Composer. IMO it is solving a problem that we don't have - downloading dependencies. Drush has very few dependencies, and has a solid system for fetching them. Composer does write an autoload.php which is nice but we could surely do that ourselves.
What's this about? Can't the autoloader handle all the require_once?
I'm not a fan of putting Drush in the Drupal namespace. Drush is its own project that happens to be hosted on drupal.org. Could we go with drush/drush?
Drush core doesn't currently use YAML AFAIK. Why is this here?
httpserver is an optional component. many hosting environments can't run it anyway. could we put this in an optional section. if we did that, what would ramifications be?
Why would this function get called twice in same request? If it won't, please remove the static. We don't add statics "just in case". They have to prove merit first.
Is this really supposed to be TRUE at the end? The code flow here is not clear, IMO
Grammer is borked here.
Why are we suppressing warnings here? Is this not useful information?
s/runs/run
Would be good to use drush_tempdir() or similar. Maybe we can move the composer run later in the bootstrap like _drush_bootstrap_drush_validate() where the console_table stuff is now.
No error handling?
Drush has wrappers for system(). We could possibly use them if we move the composer run later (see above).
We have lost the ability to reuse an installed PEAR Console_Table. This is a bit lame, considering that we currently ask folks to install via PEAR.
This writable requirement is going to be a problem for hosting firms who kindly provide drush to their clients.
Comment #79
jhedstromMarked #1792550: Facilitate installing via Composer. as a duplicate.
Comment #80
greg.1.anderson CreditAttribution: greg.1.anderson commentedThis 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.
Comment #80.0
greg.1.anderson CreditAttribution: greg.1.anderson commentedUpdated issue summary.
Comment #81
clemens.tolboomXREF (slightly related)
- provision #2143043: Add composer dev for UML Diagrams and PHPUnit tests
- https://github.com/drush-ops/drush/pull/297 Add UML Diagram generation to tests.
Patch from #73 needs a big re-roll I guess.