Add support for Drush 9

Thanks for the patch! A few notes:

a) Question, is this a required part of the composer.json file? If so, we should fill it out. If not, let's just leave it out.

+    "authors": [
+        {
+            "name": "Author name",
+            "email": "author@example.com"
+        }
+    ],

b) in composer.json:

\ No newline at end of file

c) in composer.json:

+    "name": "org/config_update_ui",
+    "description": "This extension provides new commands for Drush.",

That seems wrong. Should probably be drupal/config_update_ui? And I would expect the description should be the description of the Config Update UI module, not a generic one like that?

d)

+/**
+ * A Drush commandfile.
+ *
+ * In addition to this file, you need a drush.services.yml
+ * in root of your module, and a composer.json file that provides the name
+ * of the services file to use.
+ *
+ * See these files for an example of injecting Drupal services:
+ *   - http://cgit.drupalcode.org/devel/tree/src/Commands/DevelCommands.php
+ *   - http://cgit.drupalcode.org/devel/tree/drush.services.yml
+ */

This needs to be replaced by an actual doc block for the class, not this boiler-plate doc.

e) I don't like duplicating code -- we'd have to support it now in two places. So I think either the Drush 8 commands should use this new class, or the Drush 9 commands should call the existing functions.

f) The functions in that class... should they be declared static? They seem to be static. Either that or they should ideally use dependency injection to get the services rather than using the \Drupal class? Normally we don't call \Drupal inside any class in Drupal. Can Drush use dependency injection? Hopefully...

Oh, one more thing. The first line of any function/method documentation should be one line not exceeding 80 characters. These are way too long. Such as:

+  /**
+   * Revert a set of config items to the versions provided by installed modules, themes, or install profiles. A set is all differing items from one extension, or one type of configuration.
+   *

I actually think the UI module should be where the Drush commands live. The base module just provides base classes, which are being used by several contrib modules; the UI module provides the reports and operations (including both UI and Drush versions of both). The patch is also pretty hard to review if you combine this change into the patch, because you can't really see what changed in the Drush 8 file. So... can you move it back at least for now, and if you want to move the Drush commands into the base module, file a separate issue for that? Thanks!

The patch doesn't apply for me locally either... I think you need to do a git pull. The function drush_config_update_ui_config_revert() changed somewhat recently and it looks like you were patching from an older version. The issue that changed this function was
#2935395: Drush revert command does not work with non-entity config

In the existing Drush tests (for D8), I had to load a special include file that mocked the Drush functions that the tests called. See
https://cgit.drupalcode.org/config_update/tree/config_update_ui/tests/sr...

Thanks for the new patch! The tests pass, but there are 69 coding standards messages in your new code, so I think I'll wait to review the patch until those are fixed (probably much of my review would be things like "You need to add a documentation header here" etc.).

You can see the coding standards messages by clicking through to the test result
https://www.drupal.org/pift-ci-job/976118

Thanks! Now that the coding style errors are gone, I'll give the code a review. A few notes:

a) in the drush.inc file:

+/**
+ * Bootstrap the Drush Commands CLI service and set the logger to Drush 8.
+ */
+function drush_config_update_ui_cli_service() {
+  $config_update_ui_sevice = \Drupal::service('config_update_ui.cli');
+  $config_update_ui_sevice->logger = new ConfigUpdateUiDrush8Logger();
+
+  return $config_update_ui_sevice;
+}

Typo: The variable name here $config_update_ui_sevice should end in _service not _sevice.

b) Same file:

+  $config_update_ui_sevice->logger = new ConfigUpdateUiDrush8Logger();

Normally we wouldn't want to update a member variable directly. We'd make a method.

c) same file:

+ * This is a stand in for Psr\Log\LoggerInterface which Drush 9 uses.
+ */
+class ConfigUpdateUiDrush8Logger {

- All classes mentioned in code should be fully namespaced starting with \
- Comma needed before "which".
- Stand-in should be hyphenated.
- Our Drupal coding standards normally require all classes to be in their own files, with namespaces. So this class should be moved to the src directory.

d) Inside that class:

+  /**
+   * Output a success message for Drush 8.

All function docs first lines should end in a verb like Outputs, not Output. Please fix everywhere in your patch.

It seems like you aren't that familiar with Drupal coding standards. Some key pages to read:
https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...
https://www.drupal.org/docs/develop/coding-standards/object-oriented-code
https://www.drupal.org/docs/develop/coding-standards/namespaces

e)

 +++ b/config_update_ui/config_update_ui.services.yml
@@ -0,0 +1,4 @@
+services:
+  config_update_ui.cli:
+    class: Drupal\config_update_ui\ConfigUpdateUiCliService

I don't know of any other instance in Drupal Core or most contributed modules where service classes have the word "Service" in the class name. Probably not a great idea here either? Also, since it's really just for Drush, maybe it makes more sense to have Drush in the class name than CLI?

f) in the Commands.php file:

+/**
+ * A set of drush commands for Config Update Manager.
+ */

Drush should be capitalized. Please fix everywhere in your patch.

g) Same file:

+  /**
+   * The interoperability cli service for Configuration Update Manager.

CLI is an acronym and should be all caps. Please fix everywhere in your patch.

h) same file:

+  /**
+   * List config types.
+   *
+   * @command config:list-types
+   * @aliases clt,config-list-types
+   */
+  public function listTypes() {

Missing @return documentation. Several other functions in your patch with return values are missing @return docs too.

Ran out of time... that should get you started anyway.

Thanks for all your work up to this point! I can probably do the rest of the cleanup.

Here is an update:

#23.b - Fixed by adding getter/setter for logger property and made it protected.
#23.c - Fixed by moving logger to its own file and implements LoggerInterface and Traits like other core/contrib modules.
#23.e - I did n't change it, but I agree the name is bit odd and I would a) have all those methods in same command file, if that's OK otherwise b) move the class to src/Commands

Thanks for continuing to work on this, and sorry I've been silent for a while -- I was on vacation for a few weeks.

So... There are 2 coding standards messages from that last patch, and then some test failures having to do with a missing drush_log() function [maybe the config_update_ui.drush_testing.inc file isn't loaded for those tests?]. I'll let you sort those out, but this is definitely getting closer!

A few other small notes:

a) In config_update_ui.drush.inc :

+/**
+ * Bootstraps the Drush Commands CLI service and set the logger to Drush 8.
+ */
+function drush_config_update_ui_cli_service() {

The verb "set" in this comment should be "sets".

b) In the new Drush commands class: It looks like there is no @usage for config:list-types -- probably should be?

c)

 +/**
+ * Class ConfigUpdateUiCliService.
+ *
+ * This class handles all the logic for Drush commands.
+ */
+class ConfigUpdateUiCliService {

We don't normally start doc blocks with the name of the class. It should start with a one-line description instead.

Also, I think it would be useful to say that if you use this class, you must call the setLogger() method before doing anything else.

d) And in that same class:

+  /**
+   * Associates a logging object.
+   *
+   * @param \Psr\Log\LoggerInterface $logger
+   *   The logging object.
+   */
+  public function setLogger(LoggerInterface $logger) {
+    $this->logger = $logger;
+  }

I think it would be useful here to suggest which two classes can be used for Drush 8 and Drush 9, and say that if you use the one that you've added to this patch, that is how this class figures out you're using Drush 8. This is not documented anywhere, except in the code itself, and it would be good to have in the API documentation.

e) In another project, I'm extending a class that I'm using from another project... I have grown to hate "private function" declarations, because . Can you instead make them protected? You just never know if someone is going to need to override them.

f)

+/**
+ * Class ConfigUpdateUiDrush8Logger.
+ *
+ * This is a stand-in for Psr\Log\LoggerInterface, which Drush 9 uses.
+ */
+class ConfigUpdateUiDrush8Logger implements LoggerInterface {

In docs, fully-namespaced class names should start with \ -- I noticed this one here, but there may be other instances of it.

See also comment (c) -- don't start the docs with the class name.

I went ahead and made the changes I requested in #30 to this patch.

However, in doin this I found that there is a problem with defining the D8 logger as a service (added to config_update_ui.services.yml) -- the testing platform for some reason decides to use it in non-Drush tests, and it doesn't work without Drush being available. So, I had to take a different route there. Let's see if it passes this time...

Would help if I uploaded the patch file. :)

OK, now we're getting somewhere. But the get_class() call in ConfigUpdateUiCliService::outputRows is failing because
http://us1.php.net/manual/en/function.get-class.php
it returns the full namespaced name. It's not the usual way to do things either. So let's try this.

I get a different error when I try to run this test at home (probably vendor and core are out of sync, ugh)... let's try this... just changed that one line in ConfigUpdateUiCliService where it tests the logger class to use the fully qualified namespace name. Interdiff:

   protected function outputRows(array $output) {
-    if ($this->logger instanceof ConfigUpdateUiDrush8Logger) {
+    if ($this->logger instanceof '\Drupal\config_ui\ConfigUpdateUiDrush8Logger') {

Hm. Not sure what to do here... The problem is in the tests, we're testing the Drush 8 functions (without Drush, just testing the PHP functions that Drush would call, with stubs in for the logger). But the new class for doing the actual work in Drush is failing to detect that we're doing D8 via either get_class() or instanceof. Maybe there is a better way? This is in function ConfigUpdateUiCliService::outputRows().

Probably we should just set a flag in the constructor, or add to the setLogger() function? That seems more reasonable. I'll try that... let's see...

Aha! Finally a new error. Looks like the Drush 8 logger class isn't complete:

Drupal\Tests\config_update_ui\Functional\ConfigUpdateTest::testConfigReport
Error: Call to undefined method Drupal\config_update_ui\Logger\ConfigUpdateUiDrush8Logger::success()

I wish my environment was letting me run tests right now, this would be a lot faster... Anyway, I can't work on this today but will get back to it in a few days.

So.... The Drush logger class in this patch implements the PSR log interface, by using the Drupal logging trait. That looks good.

But the code in ConfigUpdateUiCliService calls a method success(), which exists on
https://github.com/consolidation/log/blob/master/src/Logger.php
(the Drush logging class extends this)
but as far as I can tell, doesn't exist on any logging interface anywhere. And there is zero API documentation for the method. Doh!

So, how is this being used? It looks like in the old drush.inc file for Drush 8, we did things like:

drush_print(dt('No added config'), 0, STDERR)

But in the new code, it's doing

 $this->logger->success(dt('No added config.'));

I don't actually know if that is the right thing to do here. It doesn't seem like this result should be *logged* per se. The result needs to be sent back to the user and displayed. I'm not sure if that is what the success() method does? Maybe.

Anyway, I guess what I will do is add a success() method to the Drush 8 logger class that reproduces the old drush_print, and leave it at that. Here's a new patch. Maybe this time the tests will pass...

OK, the tests passed! There's now just a coding standards message with this interdiff in the new success function:

-  public function success($message, array $context = array()) {
+  public function success($message, array $context = []) {

Applying that...

OK. So the automated tests are now passing.

I think we need to test this manually with both Drush 8 and Drush 9. The automated tests verify that some of the command functions for Drush 8 work, but I think in this case since the whole Drush infrastructure has been updated, we should do a manual test at the Drush command line and make sure that:
- the commands all still appear in drush help
- the individual command help works
- the commands work, including some error or empty output cases

I will see what I can do about testing sometime soon... if anyone else wants to test manually, please do! And post here with what you tested and what worked/didn't work. Thanks!

I have completed a manual test using Drush 8. I tested "drush help" and the functionality of all of the commands, and they all are working as they used to without this patch. So, that is good!

We still need to test Drush 9.

I have tested the commands in Drush 9.

First problem: the usage lines were wrong -- they all started with "Drush" instead of "drush". That was easy to fix in the @usage lines in the ConfigUpdateUiCommands.php file.

Second problem: the output is formatted in a really annoying and/or wrong way. It is hard to read. The default output looks like this:

$ drush9 cra system.all
-
  item: seven.settings
-
  item: system.action.user_add_role_action.administrator
-
  item: system.action.user_remove_role_action.administrator

You can try some other formats and it is even worse for some, like:

$ drush9 crd type system.all --format=list
0
1
2
3
4
5

Totally useless!

$ drush9 crd type system.all --format=table
 -------------------------------------- 
  Item                                  
 -------------------------------------- 
  contact.form.feedback                 
  core.extension                        
  core.menu.static_menu_link_overrides  
  filter.format.basic_html              
  filter.format.full_html               
  filter.format.restricted_html         

So, I think this patch needs some work. The output should not be wrapped in these "item" things. ?!?

Meanwhile here is a patch that updates the @usage lines (I didn't bother to make an interdiff).

I looked into the output formatting, which is provided for Drush by this project:
https://github.com/consolidation/output-formatters

It looks like if the data is really just a flat array of strings, the right thing to do is just to return it as an array of strings.

This simplifies the patch a bit. Now the output is more reasonable.

$ drush9 crd type system.all --format=list
contact.form.feedback
core.extension
core.menu.static_menu_link_overrides
filter.format.basic_html
filter.format.full_html
filter.format.restricted_html
node.settings
system.date
system.file

or without the format option:

$ drush9 crd type system.all
- contact.form.feedback
- core.extension
- core.menu.static_menu_link_overrides
- filter.format.basic_html
- filter.format.full_html
- filter.format.restricted_html
- node.settings
- system.date

I have now tested various commands with Drush 9 and they all seem to work fine.

Any thoughts on this new patch?

Actually, this patch doesn't need the member variable drushVersion any more. Removing that...

And... removing

use Consolidation\OutputFormatters\StructuredData\RowsOfFields;

from ConfigUpdateUiCliService.php to take care of coder message.

OK, I think this latest patch is working... Any comments @vijaycs85 or @anemirovsky? I will leave this open for a week or so, and then commit it if there are no objections before then. Thanks again for all of both of your work on this issue!

Updating issue summary.

Just tested against a fresh addition of the module to a drush 9 install. Worked perfectly (commands now appear in drush list). Curious about output differences though with config-diff vs what I see in the UI doing a diff on the same config name. This is probably another issue though, not for this patch.

Yes, please create an issue for diff differences! Thanks for testing.

I think this has been reviewed enough now, and waited long enough for comments, and I have committed it. Thanks all!