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

8 to master #2373

Closed
wants to merge 226 commits into from
Closed

8 to master #2373

wants to merge 226 commits into from

Conversation

greg-1-anderson
Copy link
Member

@greg-1-anderson greg-1-anderson commented Oct 4, 2016

  • Need to initialize Robo commandfiles correctly
  • Need to bootstrap annotated commands correctly
  • Need to support @allow-additional-options

So, I went ahead and added support for @allow-additional-options here. Truthfully, this might be difficult to support in Symfony. We can postpone removing it until later; however, we should consider whether there is a good use-case for allowing unvalidated options moving forward. Hooks can always declare their additional options explicitly with @option, and that is a validated pattern. php-eval / php-script seems to be the one place this might be helpful; however, it might be better to simply de-support drush_get_option() in these sorts of scripts, and force folks to define an actual command if arguments and options are desired.

Another area where I broke backwards compatibility is in php-eval output. The output-formatters library only outputs objects, arrays and strings. Booleans and integers are ignored. Again, I think that output of integers and booleans is not really important in most cases, with php-eval being a notable exception. I figure that we can force folks to use var_export($foo); instead of return $foo; if they wish to display booleans, though; perhaps this is good enough.

Enough tests have passed that I think this test run will finish up all green.

greg-1-anderson and others added 30 commits March 9, 2016 10:56
…its testing of tags based on the branches configuration.
commit 1c561dc
Author: John Bickar <[email protected]>
Date:   Fri Feb 12 15:01:56 2016 -0800

    Declare example-value for --target-dump and --source-dump

commit 54a17d6
Author: John Bickar <[email protected]>
Date:   Fri Feb 12 14:30:07 2016 -0800

    Update drush sql-sync --target-dump and --source-dump documentation. See #1435
It duplicates config-import --partial, and is buggy. A fix for --partial is coming in #2069
…st re-using some of its internals. Adds preview at end as well.
commit 0438baf
Author: Moshe Weitzman <[email protected]>
Date:   Tue Mar 29 10:11:33 2016 -0400

    A fix to the implode() of sanitize queries.

commit 8ed8452
Author: Moshe Weitzman <[email protected]>
Date:   Mon Mar 28 08:59:03 2016 -0400

    Fix #1869 and #1582. Better support for database prefixes during sql-sanitize ... Database prefixes are still discouraged by Drush maintainers.
… not a .module file.

Note that this would need rework for Drush8 since Drupal 6 did not use .info.yml files. A backport is welcome for 8.x.
Copy link
Member

@weitzman weitzman left a comment

Choose a reason for hiding this comment

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

Done with code review. Will poke around in the debugger next.

@@ -0,0 +1,173 @@
<?php
Copy link
Member

Choose a reason for hiding this comment

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

We already have https://github.com/drush-ops/drush/blob/master/lib/Drush/CommandFiles/InitCommandFile.php. We should not add this old style file back in, I think.

@@ -155,7 +155,12 @@ function make_drush_command() {
'makefile' => 'Filename of the makefile to convert.',
),
'options' => array(
'format' => 'The format to which the make file should be converted. Accepted values include make, composer, and yml.'
'format' => 'The format to which the make file should be converted. Accepted values include make, composer, and yml.',
/*
Copy link
Member

Choose a reason for hiding this comment

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

Debug code? Why commented out?

}

/**
* Initialize and cache the command factory. Drush 9 uses dependency injection.
Copy link
Member

Choose a reason for hiding this comment

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

Confusing reference to Drush 9, since this code is itself Drush 9.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll fix up the comment. This code will use more DI in the near future, but does not right now.


/**
* This function is set as the $command['callback'] for Symfony Console commands
* e.g. those provided by Drupal 8 modules. Note that Drush 9 calls these with
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this comment needs updating as this code is Drush 9.

@@ -0,0 +1,117 @@
<?php
/**
* This file was recommended at http://php.net/manual/en/function.array-column.php#refsect1-function.array-column-seealso
Copy link
Member

Choose a reason for hiding this comment

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

This file is no longer needed since master branch wont support PHP 5.4

@@ -317,14 +331,55 @@ function _drush_invoke_hooks($command, $args) {
}
}

// We will adapt and call as many of the annotated command hooks as we can.
// The following command hooks are not supported in Drush 8.x:
Copy link
Member

Choose a reason for hiding this comment

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

Wrong Drush version mentioned.

@@ -183,6 +183,19 @@ public function selectReleaseBasedOnStrategy($request, $restrict_to = '', $selec
}
$releases = $project_release_info->filterReleases($filter, $version);

// Special checking: Drupal 6 is EOL, so there are no stable
Copy link
Member

Choose a reason for hiding this comment

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

I think this whole block is unneeded, as we dont support Drupal 6 in master.

@@ -130,6 +130,11 @@ function testCoreRequirements() {
'ignore' => 'cron,http requests,update,update_core,trusted_host_patterns', // no network access when running in tests, so ignore these
'strict' => 0, // invoke from script: do not verify options
);
// Drupal 6 has reached EOL, so we will always get errors for 'update_contrib';
Copy link
Member

Choose a reason for hiding this comment

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

Not needed on master

@@ -68,6 +68,9 @@ public function setUp() {
}

function testUpdateCode() {
if (UNISH_DRUPAL_MAJOR_VERSION < 7) {
Copy link
Member

Choose a reason for hiding this comment

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

Not needed.

@greg-1-anderson
Copy link
Member Author

kk, did all of that; tests still green.

@weitzman
Copy link
Member

weitzman commented Oct 5, 2016

  • Whats BuilderAwareInterface? In general, it would be ideal to detail the command authors should use Robo features. I see that Init command does so, but not others.
  • druplicon appears as a command in drush help. Do we support a @hidden annotation yet?
  • -—druplicon option is showing under all commands. Would be good for this to be a hidden option only visible under global options.
  • The end of drush help shows a bunch of non end-user commands. See Other commands: (ExampleCommandFile,EvalCommandFile), All commands in InitCommandFile: (InitCommandFile)
  • Upon running drush browse -v admin, I see that \Drush\CommandFiles\core\BrowseCommands::browse is passed the following array as $options:
$options = {array} [9]
 no-browser = true
 verbose = true
 browser = false
 druplicon = false
 config-file = "/Users/moshe.weitzman/reps/d7/drush/drushrc.php"
 context-path = "/Users/moshe.weitzman/reps/d7/drush/drushrc.php"
 uri = "http://d7.dd:8083"
 php = "/Applications/DevDesktop/php5_6/bin/php"
 php-options = false

Kind of an odd set of keys in there. I wonder if this is the most clear API to be handing our command callback authors.

  • Inside that browse method, I see drush_invoke_process($site_record, 'browse', func_get_args(). The func_get_args() is no longer correct. Should be [$path], I think.
  • Also from browse(), I see
      if (!drush_bootstrap(DRUSH_BOOTSTRAP_DRUPAL_FULL)) {
        // Fail gracefully if unable to bootstrap Drupal. drush_bootstrap() has
        // already logged an error.
        return FALSE;
      }

The code comment and behavior look wrong. If I run drush browse outside a Drupal site, I see somewhat client error message: [error] We could not find an applicable site for that command..

  • FYI there is a pre-2.0 code comment at \Consolidation\AnnotatedCommand\Hooks\HookManager::callProcessor
  • This log message should be inside the conditional below it: drush_log(dt('Displaying Druplicon for "!command" command.', array('!command' => $commandName)));
  • I’m seeing notices when I run dr st -v from outside a site:
~/reps/drush (8-to-master<) $ dr st -v
 [info] Undefined index: name annotationcommand_adapter.inc:506
 [info] Loading outputformat engine.
 [info] Undefined index: name annotationcommand_adapter.inc:506
 [info] Undefined index: name annotationcommand_adapter.inc:506
 [info] Undefined variable: annotationData annotationcommand_adapter.inc:735
 [info] Undefined variable: annotationData annotationcommand_adapter.inc:735
 [info] Undefined variable: annotationData annotationcommand_adapter.inc:735
 [info] Undefined variable: annotationData annotationcommand_adapter.inc:735
  • \Drush\CommandFiles\InitCommandFile should now have @bootstrap NONE annotation and remove related code @todo.

@greg-1-anderson
Copy link
Member Author

@bootstrap none is currently the default. I guess that's backwards from current Drush; should probably make the default full and make folks label it none like today -- although if we were starting over, I'd say no annotation == no behavior == bootstrap none.

@greg-1-anderson
Copy link
Member Author

Could / should we just start throw-ing from commandfiles if we want to stop execution with an error?

@greg-1-anderson
Copy link
Member Author

I got a lot of the above comments cleared up, but there are a few open threads on this issue. Could we merge here, and discuss each of the open questions in separate issues / PRs?

@weitzman
Copy link
Member

weitzman commented Oct 6, 2016

  • I can get behind BOOTSTRAP_NONE being the default. Its strictly more of a DrupalBoot thing than a Drush thing.
  • Yes, throwing exceptions sounds good to me. Also, I think we can ditch rollback hooks when we ditch the legacy runner.
  • I notice that this PR merges 226 commits. I'm ambivalent about whether you squash merge it or not. Your call. Interestingly, this PR lists 30 participants. They must have been added because they submitted a commit.
  • Fine with me to merge and open followups.

Thanks @greg-1-anderson.

@greg-1-anderson
Copy link
Member Author

All of the commits here are an artifact of the fact that GitHub is confused about the merge that is happening. I think that all of those commits are everything on the 8.x branch that has been committed since it forked off of master, and all of the participants here are all the folks who made at least one of those commits.

I'll merge with caution through the cli to ensure that what we end up with is rational.

@weitzman
Copy link
Member

weitzman commented Oct 7, 2016

This epic PR has landed. Thanks @greg-1-anderson.

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.