-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Symfony dispatch #2843
Symfony dispatch #2843
Conversation
src/Boot/PreprocessArgs.php
Outdated
* Preprocess commandline arguments | ||
* | ||
* If we are still going to support --php and --php-options flags, then | ||
* we need to remove those here as well (or add them to the Symfony |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine leaving behind these options for local requests. For remote requests, we might be able to ditch them as well. We have #env-vars or something like that where PATH can be manipulated.
5321e13
to
f51d0fb
Compare
Making some progress here. I am refactoring the Drush bootstrap into classes, starting towards our aspirational goal of removing the For testing, I made an TranslationThe next thing I am wondering about is, what do we want to do about translation? Do we want to stick with [UPDATE: This is explained in #2863. Folks used to use this, and presumably still would, if Drush sources were pushed to drupal.org.] Bootstrapping and Module CommandsWe can probably override |
There are still large parts of the preflight that are not done, but quite a bit of it works now. Still no aliases. However, all of the built-in commands are registered, and the |
2bb622e
to
fa61eb5
Compare
See also related PR consolidation/config#3 |
Wow, ambitious plan :) I think we have to make a few changes to the launcher. Could we define a API for the launcher? It currently includes a few files and calls drush_main. Its tricky because the launcher can work with drush 8 and 9. |
Right now I consider the front controller (the In any event, if you'd like to propose an API for the launcher that works with both the front controller and Drush 8, I'd be happy to consider it. Alternatively, we could just put everything in the |
Next step is to make bootstrapping an |
Now we've got a little bootstrapping. |
|
src/Boot/BootstrapHook.php
Outdated
|
||
$phase = $annotationData->get('bootstrap'); | ||
if (!defined($phase)) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should log a warning or something here.
It would also probably be nice to allow folks to use shorter aliases, e.g. full
instead of DRUSH_BOOTSTRAP_FULL
, via a lookup table.
$this->config->addPlaceholder(self::DRUPAL_CONTEXT); | ||
$this->config->addPlaceholder(self::SITE_CONTEXT); // not implemented yet (multisite) | ||
$this->config->addPlaceholder(self::ALIAS_CONTEXT); | ||
$this->config->addPlaceholder(self::PREFLIGHT_CONTEXT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the alias context should be higher priority than the preflight context.
Great work. A few thoughts ...
|
|
1aad3c8
to
0d8108c
Compare
+1 for dropping dt(), i don't like the fact that all Drush strings/translations leak slowly into my Drupal site. |
@webflo Hm, yes, that does sound like a design flaw. Translation strings should stay separate from the site. Surprised no translators have growled at Drush over this yet. |
Alias manager partially implemented; you may select a site via a site specification, e.g.:
Aliases are not loaded from files yet, so |
@weitzman Could you +1 consolidation/config#3? Still using that as a dev version here. Could use a second set of eyes before I make a stable tag with this API. I also see that I don't have anything to apply global options from config. I'm going to do that from a hook in consolidation/config that examines the global options in $application and pulls matching values from config. |
98fa9d6
to
0edb522
Compare
cd0c6c2
to
087c91d
Compare
Add a little color as well. Can’t do color in the tables right now as the style names get brown by the wrap() feature
…behavior of code sniff. :(
…it previously was (just require the front contorller).
Current Status
drush site-install
works for Drupal 8.3.0 and later. Fails on Drupal 8.2.2. Haven't tried on other 8.2.x releases; probably works from about 8.2.6 and later.Drush's vendor directory contains Symfony 3.x, but Drush gives preference to Drupal's autoloader, and will therefore use Drupal's Symfony 2.x components, or whatever else Drupal is providing.
Known Bugs
drush status --foo
) does not show a "no such option" error message (Symfony 2 only: this bug only happens when using a Drupal site older than Drupal 8.4.x.)To Do
@bootstrap
annotations via an "init" hook@bootstrap full
)drush @a,@b,@c cex -y
,drush @all
, etc.)parent
common
section to alias files that is merged into all of the variants of one site (limitedparent
replacement).target-command-specific
andsource-command-specific
for rsync / sql-syncpath-aliases
(e.g.drush rsync @foo:%mypath bar
) - maybe remove support.DRUSH_PIPE
context--include
Things that may be out of scope / partially out of scope / removed features for this PR:
help
andlist
--help foo
tohelp foo
in preflightunish.alias.yml
file instead of an old-style alias.drush.php file.root
,alias-path
, etc.)$msg = iconv('UTF-8', $charset, $msg);
for all $msg, perhaps with a subclass of ConsoleOutput)See also #975 for discussion on planned simplifications.