Symfony dispatch

[greg-1-anderson]

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

• Running a command with an unknown option (e.g. 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

• Preflight
• Load configuration from .yml files
• Find Drush commandfiles
• Identify Drupal root and load Drupal's autoloader
• Dependency injection
• Bootstrap Drupal per @bootstrap annotations via an "init" hook
  • Allow shorthand bootstrap names (e.g. @bootstrap full)
• Aliases
  • Add parser for @alias names and site specifications (e.g. user@server/path#default)
  • Allow alias manager to alter the selected Drupal site during bootstrap
  • Create a 'self' alias
  • Add 'self' alias to config overlay
  • Apply values from alias + other config to global commandline options
  • Find and load named aliases from alias yml files
  • Remove support for alias lists (drush @a,@b,@c cex -y, drush @all, etc.)
  • Remove support for parent
  • Add a common section to alias files that is merged into all of the variants of one site (limited parent replacement).
  • Remove support for target-command-specific and source-command-specific for rsync / sql-sync
  • Determine what to do with path-aliases (e.g. drush rsync @foo:%mypath bar) - maybe remove support.
• Commands
  • core-status
    • Make --include-field-labels default to true in table format
  • site-install
  • site-alias
    • Add names to alias records so that lists can be keyed by name
    • Add a mode to show all sites in a group, or all environments associated with a site
  • ssh
  • rsync @src @DesT (sans path aliases, e.g. @src:%files)
• Find commandfiles in modules (bootstrap further if command not found)
• Select site conf path (settings dir) based on --uri
• Decide on an interim strategy for early logging + shutdown & error handlers
  • Throw on fatal errors in preflight
  • Don't log at all in preflight; only log after DI container is set up
  • Omit error and shutdown hander until after DI container is set up
• Test the new code (Symfony dispatcher) instead of the old code (legacy Drush dispatcher) in the functional tests
• Make the tests pass, just to avoid regressions as we continue (skip tests not yet working)
  • annotatedCommandTest
  • backendTest (skipped a few; nonexistent commands + POST (stdin) + concurrency + an existing failure)
  • batchTest
  • contextTest (removed, obsolete)
  • commandUnitCase (removed, obsolete)
  • configPullTest
  • configTest
  • coreTest
  • expandWildcardTablesUnitTest
  • filesystemTest (removed, obsolete)
  • imageTest
  • initCommand
  • pmEnDisUnListInfoTest
  • queueTest
  • roleTest
  • rsyncTest
  • siteAliasTest (removed, obsolete)
  • siteAliasUnitTest (removed, obsolete)
  • siteSetTest
  • siteSetUnitTest (removed, obsolete)
  • siteSshTest
  • sqlConnectCreateTest
  • sqlDumpTest
  • sqlSyncTest
  • tablesUnitTest (removed, obsolete)
  • userTest
  • watchdogTest
  • xhUnitTest
• Backend invoke
  • Call remote commands via backend invoke when remote alias used via "init" hook
  • Handle log messages - send logs to remote Drush, print log messages recieved
  • Remove DRUSH_PIPE context
  • Cache log entries locally, e.g. for "log has errors" support
  • Print backend output at the end of commands
  • Pass through preflight args such as --include
  • Remove concurrency support
• Global option handling
  • Implement the important ones we still need
    • --yes
    • --debug
    • --simulate
    • --pipe (now a local option for commands that use formatters)
    • --strict
  • Convert to config
    • --halt-on-error
    • --ssh-options
    • --php
    • --php-options
    • --php-notices
    • --variables (DrupalBoot.php)
    • --remote-os
• sql-sync should recognize options for sql-dump and rsync
• Path aliases for rsync
• Fix skipped tests
  • config pull should work once --strict and rsync work
  • rsync test needs %file lookup. Also, coreTest has an rsync test that could be moved
• Add Doxygen to key methods
• Update /docs and /examples directories (test the commands in examples/Commands using --include)

Things that may be out of scope / partially out of scope / removed features for this PR:

• Consider enhancements to help and list
• Site-set command
• Remove support for standalone scripts
• Backend invoke for commands that do not exist locally
• Remap --help foo to help foo in preflight
• Show alias files in Drush status
• Show configuration files in Drush status
• Read commandline arguments from stdin when called in backend mode
• Support shell aliases Shell aliases (or yml commands)  #2943
• Site alias enhancements
  • Make the tests write a unish.alias.yml file instead of an old-style alias.drush.php file.
  • Convert remaining code using legacy site records to use AliasRecord class
  • Consider config alter hook / alias alter hook
  • Config option to enable/disable auto alias conversion, command to do one-shot conversion
• Allow configuration files to specify preflight options (root, alias-path, etc.)
• Translation
• Remove legacy code
  • Clean up / replace as many drush_context variables as possible.
  • Remove calls to drush_get_option
  • In DrupalBoot8, call a class implementation of module commandfile loader rather than adapter
  • Determine if cache.inc is still in use; replace it if so
• Consider global drush vs. site-local drush support features
  • Consider what to do vis-a-vis launcher (API vs. drush_main() etc.)
  • Consider support for aliases in launcher
  • Consider removing support for aliases in Drush
  • Consider removing support for redispatch to site-local Drush (or implement feature)
• Consider backwards compatibility features
  • Load old-style group.aliases.drushrc.php files
  • Exec drush8 for pm-* commands
  • Exec drush8 for old-style *.drush.inc Drush commands in modules
• Drush-version-specific configuration and search paths
• Fix up the rest of the global options
  • Survey and convert any we still need
  • Remove the ones we do not need
    • --ignore-modules
  • Convert the others to config
    • --editor
    • --bg
    • remove support for --drush
    • --drush-script
    • --backup-location
    • --drush-backups
    • --output_charset (this implies we call $msg = iconv('UTF-8', $charset, $msg); for all $msg, perhaps with a subclass of ConsoleOutput)

See also #975 for discussion on planned simplifications.

[weitzman]

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.

[greg-1-anderson]

Making some progress here. I am refactoring the Drush bootstrap into classes, starting towards our aspirational goal of removing the includes directory. Maybe I will be able to get rid of preflight.inc, environment.inc, context.inc and command.inc by the time I am done here -- or at least reduce them to a handful of vestigial functions.

For testing, I made an isolation directory for some phpunit tests that does true unit testing on the new bootstrap classes in isolation. Originally I put this directory inside of tests, but doing that caused the current Drush phpunit tests to pick up and run my test classes, which did not work. In time, I think I will move the existing tests to tests/functional, to allow me to move the isolation tests back to tests/isolation. I'm likely to also add tests/integration for tests that run through a full Symfony Console dispatch without a real Drupal site.

Translation

The next thing I am wondering about is, what do we want to do about translation?

Do we want to stick with dt('message')? While this is procedural, it is nice and short. Something like \Drush::t('message'), or even \D::t('message') is a bit longer. One thing I am wondering about, though, is why dt() passes through Drupal's t() method. Is this just to take advantage of strings that might already be translated? It seems like the overlap here would be negligible at best. Is there any other benefit? Could we just remove this feature?

[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 Commands

We can probably override Application::find() to increase the bootstrap level and pull in module commands if the user tries to run a command that does not exist. Same for help. Do we already have a solution for list though? More research needed.

[greg-1-anderson]

There are still large parts of the preflight that are not done, but quite a bit of it works now.

Still no aliases.
Still no bootstrapping.

However, all of the built-in commands are registered, and the list and help commands work. I'm using the built-in Symfony versions of those at the moment, as the Drush-specific versions make assumptions about our legacy dispatcher.

[greg-1-anderson]

See also related PR consolidation/config#3

[webflo]

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.

[greg-1-anderson]

Right now I consider the front controller (the dr script) to be only a stub. I haven't really made a plan for the launcher; at this point I'm just working on getting bootstrapping to work after refactoring the critical include files into classes. Note that at this point it is still up in the air whether this PR will get merged into Drush 9; it is likely that we will make this PR the first commit for Drush 10 once we tag 9.0.0 stable. It's possible, though, that the front controller and some of the class refactoring might make it back into Drush 9.

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 dr script into a drush_main function to satisfy the current expectations of the launcher.

[greg-1-anderson]

Next step is to make bootstrapping an init hook.

[greg-1-anderson]

Now we've got a little bootstrapping.

[greg-1-anderson]

drush site-install is now working on Drupal 8.3.0 and later, so I updated the summary of this issue to better track progress.

[greg-1-anderson]

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.

[greg-1-anderson]

Maybe the alias context should be higher priority than the preflight context.

[weitzman]

Great work. A few thoughts ...

1. I'm curious about your ideas around "Update tests to use Symfony dispatch instead of old Drush dispatcher"
2. I added checkboxes in OP for docs and doxygen
3. It looks like we are not adding any drush services. should we?
4. Its fine with me to drop the integration between our dt() and Drupal's t(). My slight inclination is to stick with dt() despite it being procedural.

[greg-1-anderson]

1. Right now, the drush script runs the existing Drush code (the "old Drush dispatcher"). I added some "isolation" unit tests to test the new code; however, all of the existing functional tests are running the existing code. To use the new code, use the new front controller in the dr script. Eventually, dr just becomes drush.
2. 👍
3. For what?
4. We should keep this if we do Syncronice back to Drupal .org to localize drush #2863. Otherwise, might as well drop it.

[webflo]

+1 for dropping dt(), i don't like the fact that all Drush strings/translations leak slowly into my Drupal site.

[greg-1-anderson]

@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.

[greg-1-anderson]

Alias manager partially implemented; you may select a site via a site specification, e.g.:

$ dr /path/to/drupal#default status --include-field-labels

Aliases are not loaded from files yet, so @alias does not work. Also, the @self alias is not added to the config overlay yet.

[greg-1-anderson]

@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. I'll ping you on that when I have it ready. (It was straightforward so I just merged it.)

[coveralls]

Coverage increased (+5.3%) to 53.87% when pulling de55232 on symfony-dispatch into c106607 on master.