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 a legacy 'drush dl' command #2790

Closed
wants to merge 7 commits into from
Closed

Add a legacy 'drush dl' command #2790

wants to merge 7 commits into from

Conversation

greg-1-anderson
Copy link
Member

Wraps 'composer require' / 'composer create-project'.

The create-project version can behave strangely; for example, drush dl drupal downloads Drupal 8.1.3 instead of 8.3.2. Not sure why that is. Also, it seems that --dev does not work; perhaps this is an expected characteristic of the drupal.org packagist.

@greg-1-anderson
Copy link
Member Author

Not sure if this code should stay in LegacyCommands or move somewhere else.

@greg-1-anderson
Copy link
Member Author

This is odd:

composer create-project --repository 'https://packages.drupal.org/8' 'drupal/drupal:^8.3' --ansi

  [InvalidArgumentException]                               
  Could not find package drupal/drupal with version ^8.3.  

Maybe I should remap drupal to drupal-composer/drupal-project. That would probably serve people better in the long run anyway.

@greg-1-anderson
Copy link
Member Author

Remapping to drupal-composer/drupal-project seems to work pretty well.

I tried a preliminary implementation of using git clone in conjunction with --dev mode. This idea might work pretty well if there was a good way to determine which branch to use that did not require a huge amount of code to parse release xml.

@greg-1-anderson
Copy link
Member Author

I threw in some quick prototype code to parse the release xml. Maybe this should ultimately use an xml parser rather than preg_match, maybe default_major isn't the right attribute, maybe there should be some way for the user to specify which version they want when using --dev.

Looks like there might be some promise here, though, in terms of volume of code to utility.

@weitzman
Copy link
Member

weitzman commented Jun 2, 2017

I see this as an education tool. I'd like for this to, by default, show you a composer command thats close to what you meant when you run drush dl ctools. If we want to go further and do same for drush dl ctools-8.2 --devthats fine. At no point should the command suggestgit clone`. We should not wrap git, just composer. I'm not sure about actually running composer for folks. We then have to deal folks who don't have composer installed or have bad perms and all that.

I'd prefer to stay away from XML fetch/parse entirely. This should be the job of composer and drupal.org's Packagist.

Am willing to discuss all this.

@greg-1-anderson
Copy link
Member Author

I'm totally on the fence about git clone and happy to drop that. I agree that the story gets inconsistent when we start parsing release xml.

I do think that we should run composer for the user. The command that is run is in fact echo'ed in the progress logs. drush dl foo outside a Drupal site is actually something I do fairly often, just to look at the module source code; this is inconvenient to do via composer. Also, I aim to bring back drush quick-drupal, which I feel is a very important command.

I know composer, but I find that the lack of drush dl and drush qd to be the main reasons I switch back to Drush 8. I feel like Drush is not installed when these commands are not available. More beer, less effort.

@greg-1-anderson
Copy link
Member Author

I reviewed the commands in LegacyCommand.php again, and don't feel that wrappers for the others are really necessary. I also pointed folks trying pm-releases at composer search. There is no way in Composer to list available versions of a package not yet installed.

If you're satisfied with the story here, I think this is done-ish (pending release of Robo 1.0.8 and maybe some docs / test updates).

@weitzman
Copy link
Member

weitzman commented Jun 2, 2017

The case when composerRoot is not found may need more thought. It still works today to have a global Drush9 and point --root=[any-drupal-webroot]. In this case, it looks like composerRoot is the drupal docroot (bug?). If we do nothing, i expect the new project to download to the docroot. Which reminds me, this implementation of dl deliberately doesn't handle destination smartly?

@greg-1-anderson
Copy link
Member Author

I forgot about --destination; that should probably be supported, although probably only for the composer-rootless case. I'd guess it should throw an error to use that with the composer require branch, as we only want Composer to place the files.

It technically works to use Drupal's docroot as the composer root, but this should not be done if the target site has a nested docroot. I think that right now the DrupalFinder is only instantiated if we search for the docroot. I'll ensure that it is set up correctly when root is identified. This will be necessary for areas other than drush dl, I'm sure.

Not sure if we should warn or advise if the user is deliberately using drush dl to install modules into the composer.json at the Drupal root. Maybe we should be pointing folks to drupal-composer/drupal-project in this case.

@weitzman weitzman changed the title Add a lgacy 'drush dl' command Add a legacy 'drush dl' command Jun 2, 2017
@weitzman
Copy link
Member

weitzman commented Jun 4, 2017

I think that right now the DrupalFinder is only instantiated if we search for the docroot. I'll ensure that it is set up correctly when root is identified. This will be necessary for areas other than drush dl, I'm sure.

It would be helpful if we find composerRoot very early on in preflight (phase 0). Thats because I want to get the Drush version via composer show and stop relying on the drupal.info text file. Right now we instantiate DrupalFinder during PHASE 1, but PHASE 0 would be ideal IMO.

@weitzman
Copy link
Member

weitzman commented Jun 9, 2017

Borrowing from our new generate command, maybe destination can be as simple as

$modules_dir = is_dir(DRUPAL_ROOT . '/modules/contrib') ? 'modules/contrib' : 'modules';

@greg-1-anderson
Copy link
Member Author

If there is a DRUPAL_ROOT, then we have a Drupal site. If we have a Drupal site, then Composer should set the destination for the modules. The --destination flag should only be valid when there is no Drupal site -- when you are just downloading a module to examine it. It should be an error if --destination points somewhere inside a Drupal site.

@weitzman
Copy link
Member

I dont see much demand for this. If you run dl today, it tells you the composer command you should be running. That help can be expanded.

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