Drush 9 port of commands

OK, thanks, good to know!
When will it be released? Is the class structure already fixed, and is there some documentation?

As far as I know it's pretty close a beta-6 release was already tagged. There's documentation on how to port the commands: https://weitzman.github.io/blog/port-to-drush9

Here's a pretty 1:1 port. As a note for any future work, the table formatting is fairly picky; for instance, without the @return annotation in the method, it doesn't output correctly (missing table headers, columns not aligned, etc.).

Reviewing and fixing

The new patch contains next changes:

1. +++ b/src/Commands/SearchApiCommands.php
   @@ -0,0 +1,721 @@
   +use Exception;
   ...
   +      throw new Exception(dt('There are no indexes defined. Please define an index before trying to enable it.'));
   ...
   +      throw new Exception(dt('You must specify at least one index to enable.'));
   ...
   +      throw new Exception(dt('There are no indexes defined. Please define an index before trying to disable it.'));
   ...
   +      throw new Exception(dt('You must specify at least one index to disable.'));
   ...
   +        throw new Exception(dt("Couldn't create a batch, please check the batch size and limit parameters."));
   ...
   +      throw new Exception(dt('You must specify both an index and server.'));
   ...
   +      throw new Exception(dt('Invalid index ID "@index_id". The following indexes are defined:', ['@index_id' => $indexId]));
   ...
   +      throw new Exception(dt('Invalid server ID "@server_id". The following servers are defined:', ['@server_id' => $serverId]));
   ...
   +        throw new Exception(dt('No indexes present.'));
   ...
   +        throw new Exception(dt('Invalid index ID "@index_id".', ['@index_id' => $index_id]));
   

   Usually we use the full qualified reference: \Exception and avoid use \Exception;.

2. +++ b/src/Commands/SearchApiCommands.php
   @@ -0,0 +1,721 @@
   +   * @command search:api:list
   ...
   +   * @command search:api:enable
   ...
   +   * @command search:api:enable:all
   ...
   +   * @command search:api:disable:all
   ...
   +   * @command search:api:status
   ...
   +   * @command search:api:index
   ...
   +   * @command search:api:reset:tracker
   ...
   +   * @command search:api:clear
   ...
   +   * @command search:api:search
   ...
   +   * @command search:api:server:list
   ...
   +   * @command search:api:server:enable
   ...
   +   * @command search:api:server:disable
   ...
   +   * @command search:api:server:clear
   ...
   +   * @command search:api:set:index:server
   

   About naming:

   • The colon (:) from the command name should split a main command from its sub-command. The module machine name is search_api, so search and api are not separate things because api is not a subcommand of search. The prefix of all commands should be search-api:.
   • Just replacing the dash (-) from the old command with colon (:) is wrong because of the same logic. For this reason search:api:reset:tracker
     should be search-api:reset-tracker because "reset tracker"
     is the command, so the two words should be together, separated by dash. the same applies to search:api:server:list, search:api:server:enable,
     search:api:server:disable,search:api:server:clear, search:api:set:index:server

3. +++ b/src/Commands/SearchApiCommands.php
   @@ -0,0 +1,721 @@
   +  public function apiList() {
   ...
   +  public function apiEnable($indexId = NULL) {
   ...
   +  public function apiEnableAll() {
   ...
   +  public function apiDisable($indexId = NULL) {
   ...
   +  public function apiStatus($indexId = NULL) {
   ...
   +  public function apiIndex($indexId, $options = ['limit' => NULL, 'batch-size' => NULL]) {
   ...
   +  public function apiResetTracker($indexId, $options = ['entity-types' => []]) {
   ...
   +  public function apiClear($indexId) {
   ...
   +  public function apiSearch($indexId, $keyword) {
   ...
   +  public function apiServerList() {
   ...
   +  public function apiServerEnable($serverId) {
   ...
   +  public function apiServerDisable($serverId) {
   ...
   +  public function apiServerClear($serverId) {
   ...
   +  public function apiSetIndexServer($indexId, $serverId) {
   

   For the same reason mentioned before, we should remove the "api" prefix from the method names. Note that "list" is a reserved word for method name in PHP >= 7. So let's use listCommand().

4. +++ b/src/Commands/SearchApiCommands.php
   @@ -0,0 +1,721 @@
   +   * @usage drush search-api-list
   ...
   +   * @usage drush search-api-enable node_index
   ...
   +   * @usage drush search-api-enable-all
   ...
   +   * @usage drush search-api-disable node_index
   ...
   +   * @usage drush search-api-disable-all
   ...
   +   * @usage drush search-api-status
   ...
   +   * @usage drush search-api-index
   ...
   +   * @usage drush search-api-reindex
   ...
   +   * @usage drush search-api-clear
   ...
   +   * @usage drush search-api-search node_index title
   ...
   +   * @usage drush search-api-server-list
   ...
   +   * @usage drush search-api-server-e my_solr_server
   ...
   +   * @usage drush search-api-server-disable
   ...
   +   * @usage drush search-api-server-clear
   ...
   +   * @usage drush search-api-set-index-server default_node_index my_solr_server
   

   We should replace the old canonical command name with the new one in @usage.

5. +++ b/src/Commands/SearchApiCommands.php
   @@ -0,0 +1,721 @@
   +   * @throws \Exception
   ...
   +   * @throws \Exception
   ...
   +   * @throws \Exception
   ...
   +   * @throws \Exception
   

   @throws should go to the end of docblock and needs explanation.

6. +++ b/src/Commands/SearchApiCommands.php
   @@ -0,0 +1,721 @@
   +  private function getIndexCount() {
   ...
   +  private function setIndexState(IndexInterface $index, $enable = TRUE) {
   

   s/private/protected

Some commands were not aware if the param is optional or not.

At first glance this looks great but dependencies are not injected properly. Assigning to me to do a thorough test and dependency inject all the things.

I'm also not sure if we want to add all of these things in one single class, adding a new 1k-line class doesn't sound like the best idea. Is there a way we can split this up into smaller components?

Perhaps splitting up like this:

• SearchApiServerCommands
  • search-api:server-clear
  • search-api:server-disable
  • search-api:server-enable
  • search-api:search-list
• SearchApiIndexCommands
  • search-api:clear
  • search-api:disable
  • search-api:disable-all
  • search-api:enable
  • search-api:enable-all
  • search-api:index
  • search-api:list
  • search-api:reset-tracker
  • search-api:search
  • search-api:status
  • search-api:set-index-server ?? Not sure if this command should stay here.

On the other hand, I recently talked with @bircher about the way he was doing the drush/console commands in config split. He said that he has all of the commands in a service (http://cgit.drupalcode.org/config_split/tree/src/ConfigSplitCliService.php) and all of them have integration for drush 8 (http://cgit.drupalcode.org/config_split/tree/config_split.drush.inc), drush 9 (http://cgit.drupalcode.org/config_split/tree/src/Command) and drupal console (http://cgit.drupalcode.org/config_split/tree/src/Commands).

I would love to also go into that direction. Because right now we have duplication of all the code (which is not awesome). We also have 0% testcoverage for anything related to those commands, if the integration with drush/console is just a thin wrapper around a tested service that's not that big of a problem.

However, that is a very big refactor, I don't want to start on that without getting a thumbs up from thomas. I also think we can do that as a followup while keeping this one smaller and do just the addition of drush 9.

So maybe for now, we should just do #9 (or whatever @pfrenssen comes up with) and do this refactor later?

In any case, thanks @claudiu.cristea and @kevin.dutra for kicking this off. This is a great start!

I'll review the patch after @pfrenssen posts his version.

I'm also not sure if we want to add all of these things in one single class, adding a new 1k-line class doesn't sound like the best idea. Is there a way we can split this up into smaller components?

I really don't see the point here, except, probably, code readability. We split, then we have 2 services.

EDIT: @pfrenssen, yes I wanted to add injection. Finally I forgot.

Results from manual test:

1. $ drush sapi-l --format=list
   This outputs numbers instead of the machine names of the search indexes.

2. $ drush sapi-dis
   $ drush sapi-disa
   $ drush sapi-en
   $ drush sapi-ena
   

   These work great, I also like that the commands are not returning any output, this is according to UNIX best practices.

3. $ drush sapi-s
   • The help text is inconsistent regarding the index argument, it calls it either id or indexId. I would go for index. The other commands are also having this issue.
   • The --format=list option doesn't make sense for this command, since there will not be any status being output.
   • Why is this not integrated with the list command, so we can get all data in a single call? If we for example want to retrieve a JSON object with the indexing limit and the indexing status we need to call both drush sapi-l and drush sapi-s.
4. $ drush sapi-i
   Help texts are not correct, it mentions that we can do $ drush sapi-i index 100 10 while in reality it is $ drush sapi-i index --limit=100 --batch-size=10.
5. $ drush sapi-r
   Is it really necessary to have this large amount of aliases?
   • search-api-mark-all
   • search-api-reindex
   • sapi-r
   • search-api-reset-tracker
6. I would call the command "search-api-reindex" and keep only the alias "sapi-r". This would be also consistent with the other commands that only have a single short alias.
7. $ drush sapi-c
   Works great. I can't see any difference afterwards in drush sapi-s though after executing this or drush sapi-r. Both report 0% indexed with 0 items, but they actually work as expected, the search index is empty after clearing but not after marking for reindexing.
8. $ drush sapi-search
   This is awesome, but the "--format=list" option returns numbers instead of the search result IDs.
9. $ drush sapi-sl
   Same problem, the "--format=list" option returns numbers instead of server IDs.
10. $ drush sapi-se
$ drush sapi-sd
$ drush sapi-sc
    • It would be better if no output was generated unless the --verbose option is passed. Commands shouldn't be chatty if things go as planned unless requested.
    • If an invalid server ID is passed then the server could not be enabled, but the command still returns exit code 0, which indicates that the operation has succeeded. It should return an error code in this case, this can be done by throwing an exception.
11. $ drush sapi-sis
    Works good. I did notice that the error message that is shown when a server ID or index ID is invalid is incomplete. It says "The following servers are defined:" but then doesn't list the servers.

I am stopping for today. I will unassign since this is worked on by multiple people.

We need this in for our project urgently, so if nobody else is going to finish this during the night I will pick this back up tomorrow morning (EU time) and will have this done by tomorrow around noon. I will assign it to me as soon as I start working on it so we won't do duplicate work.

I'm also not sure if we want to add all of these things in one single class, adding a new 1k-line class doesn't sound like the best idea. Is there a way we can split this up into smaller components?

@claudiu.cristea is correct, since this collection of Drush commands is actually a Symfony service it is better to keep it in one single file.

So both @claudiu.cristea and @pfrenssen commented on the first part of the comment I posted earlier, but you didn't comment on the second part.

I actually would love to see us use this approach. I think this makes sense.

Do both of you agree with this approach (only converted the list command and only tested the drush 8 version).

Also leaving @claudiu.cristea's patch up in the files list. If ya'll disagree we can disregard this approach.

I completely agree with this approach.

@borisson_ what do you think of merging the sapi-status and sapi-list commands? For backwards compatibility they could be aliases of the same command with different defaults for displaying the information.

I'm going to work on moving this forward but I am timeboxed so I might not be able to finish the whole thing. I'll try to get as much done as possible in the time I have.

They output different things, and I assume they are used in different scenario's. I think we could do that as a followup but for now let's keep them as 2 different commands.

@pfrenssen: as soon as you have a patch I'd be happy to push this on if needed.

This went a lot deeper than I expected. Since Drush 9 is still very bleeding edge there is a whole herd of unshaven yaks waiting right around the corner when you start prodding the code.

I managed to get the `sapi-l` command working properly for all output formats, but this involved creating my own `OutputFormatter` class in order to support both the inline formats (like 'table' and 'csv') and the structured formats (like 'json' and 'yaml'). I also got XML to work but this requires a patch in consolidation/output-formatters (ref https://github.com/consolidation/output-formatters/pull/60).

Now that we can output data for machine consumption it is important that our commands can output the machine names of the data. Up to now it was only outputting human readable labels, but for automating server jobs we need machine names. So I have extended the data being returned with machine names and defaulted to the human readable fields so that the command still works as before.

It's pretty cool, on the one hand you can get nice human readable data:

$ drush sapi-l
 ------------- ---------------------- ------------------------ ------------- ---------- ------- 
  ID            Name                   Server name              Type names    Status     Limit  
 ------------- ---------------------- ------------------------ ------------- ---------- ------- 
  published     Published entities     Server for unpublished   Content,Rdf   disabled   50     
                index                  entities                 entity,User                     
  unpublished   Unpublished entities   Server for unpublished   Content,Rdf   disabled   50     
                index                  entities                 entity,User                     
 ------------- ---------------------- ------------------------ ------------- ---------- ------- 

But we can also get JSON or YAML for specific data we need in order to automate stuff:

$ drush sapi-l --fields=status --format=json
{
    "published": {
        "status": "disabled"
    }
}

$ drush sapi-l --fields=id,server,types --format=yaml
published:
  id: published
  server: solr_published
  types:
    - node
    - rdf_entity
    - user

Note that it will be a lot of work to backport this all to Drush 8. I would actually prefer to keep the Drush 8 integration with the new `CommandHelper` service out of this issue and handle it in a followup.

All remarks related to commands other than `sapi-l` is not yet addressed.

Note that it will be a lot of work to backport this all to Drush 8. I would actually prefer to keep the Drush 8 integration with the new `CommandHelper` service out of this issue and handle it in a followup.

Oh yeah, that would make it too hard. Let's revert the drush 8 portion of this patch.

Ported `drush sapi-en`.

I think we should keep all logging and translation of strings out of `CommandHelper` actually.

Reset the changes to the drush 8 commands, adds all of the things to drush 9.

I'm actually at home from work because I'm having a fever, so I hope that everything is somewhat ok. Going to write some kernel tests for the service so I'm more confident in the work I did just now.

So please review this very thoroughly.

Start of the kernel tests for this, not all of them are passing - but I don't really know why.

Fixed some coding standars thing, I'll try to pick these tests up again tomorrow.

More testcoverage. Still that one fail that I don't know why.

@drunken monkey any idea why the indexing is not working in this test?

Making real good progress @borisson_!

Yesterday evening I was looking into tackling the portability of translations + making logging possible for Drush 8 but I forgot to post my patch.

Here it is, this is against the patch from #21. It's not finished but this is something we can look at after @borisson_ is done with his work. It introduces a new `CommandLogger` which can be used to pass messages between the `CommandHelper` and the calling code. This is conceptually cleaner than setting the logger in every command, and it will also work for Drush 8 which relies on procedural code for logging.

Note that this is just an idea from yesterday evening and is now outdated, I just wanted to post this because I am going to a festival and won't be able to contribute for a few days. #29 is still the current patch!

Shortly looking over this.

First of all, thanks a lot, everyone, for your great effort on this!
Seems this is really important to have for a lot of people, so I'll also be more active here now.

On the other hand, I recently talked with @bircher about the way he was doing the drush/console commands in config split. He said that he has all of the commands in a service (http://cgit.drupalcode.org/config_split/tree/src/ConfigSplitCliService.php) and all of them have integration for drush 8 (http://cgit.drupalcode.org/config_split/tree/config_split.drush.inc), drush 9 (http://cgit.drupalcode.org/config_split/tree/src/Command) and drupal console (http://cgit.drupalcode.org/config_split/tree/src/Commands).

I would love to also go into that direction. Because right now we have duplication of all the code (which is not awesome). We also have 0% testcoverage for anything related to those commands, if the integration with drush/console is just a thin wrapper around a tested service that's not that big of a problem.

However, that is a very big refactor, I don't want to start on that without getting a thumbs up from thomas. I also think we can do that as a followup while keeping this one smaller and do just the addition of drush 9.

Thumbs up, seems like an awesome idea! I'd love to support Drupal Console, too, but I already get enough complaints about the Drush integration that I'm not using. If all of this uses the same code for the most part, and is properly covered with automated tests, it would make my life considerably easier.

Regarding the latest patch: It generally already looks very good, but I (of course) still have some complaints:

1. Unless I'm mistaken, the coding standards still require a first-line comment for test methods, even if @covers says it all. (Please point the change/documentation out to me if I'm mistaken.)
2. Indexing probably didn't work because the index status was set to 0?
3. Any reason why you didn't use ExampleContentTrait in the test?
4. Why the namespace Drupal\search_api\Utility; near the end of the test class? Doesn't seem to serve any purpose. (Also, I was actually quite sure this was illegal in PHP. But seems I was mistaken there.)
5. Services should be ordered alphabetically.
6. I guess \Drupal\search_api\Commands\SearchApiCommands not following all (doc comment) coding standards is on purpose? If the comments are taken as a command definition, then there's of course not really a way around that.
7. Since all other services of ours (except plugin managers) currently come with interface, the CommandHelper should get one, too. (Then, of course, all type hints (also in doc comments) should be updated to use the interface.)
8. After all the refactoring in #2898082: Re-organize test namespaces you want a new namespace with a single class? :P Please either use …\System or just the parent namespace (for now).
9. Is the separate exception really necessary? I'm not really against having multiple exceptions, but I don't really see any reason for that here.
10. Also: When did we decide to use llamas instead of pink ponies? :(

I didn't test any of this myself, tbh, I'm not that into Drush et al. I'll just take your word (and the test bot's – yay! \o/) for it once you and a few others say it's RTBC.

1. There's a bunch of examples in core where this is also done, not that is a good reason, I've seen this used in other places as well. I remember something like this in a recent discussion but I think it's allowed. I don't have a link to an issue though.
2. Oh, probably :D
3. Do we have that for kernel tests? I didn't find that, I don't think we can use the one from the functional tests in the kernel tests
4. I did that, so that we can override the dt-function. Otherwise it doesn't work
5. Sure.
6. Yeah, that's because of the drush command structure, I think.
7. I've said it before, but I don't foresee anyone creating their own implementation of the service. For now we can do this without an interface and iterate to make it better later, if needed.
8. \System sounds better, yeah.
9. I figured it would be more specific, I don't mind using SearchApiException, as long as we're not just using \Exception. I wanted to specific enough
10. I'll refactor that!

This also needs better/more test-coverage still, not all the methods in the utility are covered yet.

Core broke facets rest-integration, and because of that I have branch failures - after I figure that out (#2916212: Fix rest tests.), I'll come back to this issue.

@ 1: #2108785: Remove the requirement for doxygen for test methods. Being discussed for four years now, but still not nearly decided. (You'll also notice I voted against it.)

@ 3: BackendTestBase and ContentEntityDatasourceTest both use it.

@ 4: Ah, I see … Then maybe just use the root namespace instead, is that possible? Pretty ugly in any case – but probably the refactoring which should cleanly separate the service from Drush will make this obsolete anyways, right?

@ 7: It's not just about allowing others to override the service cleanly, it's also just a matter of coding standards: classes should never be used for type hinting. I.e., if we want to have a type hint (we do), we need an interface.
But feel free to ignore it for now. I'll just add it right before committing and you won't be able to stop me. ;P

@ 9: Looked over it again, and I guess a separate exception could indeed make sense there. Almost all of them are for invalid user input – having a separate exception for that does sound reasonable. (Maybe just not catch SearchApiException exceptions from elsewhere?) I guess I'm good either way.
But one more thing I noticed during that second look: you (or whoever) copy-pasted too much in disableIndexCommand(), so now the exceptions talk about "enabling".

Ah, of course, the missing namespace, like you explained …
However, reverting to what you had before, I actually got syntax errors – exactly like I thought there should be. Didn't have that before, though – very strange.
Anyways, I could only make it work with the block-based namespaces. And the other three tests are still failing.

The syntax errors are just phpstorm not understanding it. See \Drupal\Tests\Core\Session\AccountProxyTest, \Drupal\Tests\aggregator\Unit\Plugin\AggregatorPluginSettingsBaseTest, (among others in core) where the style I used i also in use.

When looking up examples in core I also saw this style:

if (!function_exists('Drupal\Tests\Core\Asset\file_create_url')) {
  function file_create_url($uri) {
    return 'file_create_url:' . $uri;
  }
}

That looks much better, but I can't get that to work.

I think I now resolved all the issues already brought up. I also have green tests locally.
Updating the IS with what we still need to do.

Still to test:

\Drupal\search_api\Utility\CommandHelper::indexItemsToIndexCommand
\Drupal\search_api\Utility\CommandHelper::resetTrackerCommand
\Drupal\search_api\Utility\CommandHelper::searchIndexCommand

The other methods are tested now.

We still need to find a way to figure out the translations and logging in an abstract manner, but at least this is a decent start.

The patch from migrate_tools is only supporting Drush and doesn't have an abstracted service to deal with all command line tools so it can just rely on Drush to translate messages.

Since `CommandHelper` is a service we shouldn't set a logger in it on call time, a service is a singleton so the logger will be persisted for all subsequent calls to it, even if it is not coming from the same source. For example a drush command could set `CommandHelper::logger`, but then a hook might fire as part of the drush command, which triggers some other code to call a method on the `CommandHelper` again, but then the logger will still be the one from the drush command. You can see this from the patch that it doesn't work right, the logger needs to be set again and again in every single method that uses something from the CommandHelper.

Basically we have a circular dependency: `SearchApiCommands` depends on `CommandHelper`, but this also depends on `SearchApiCommands` for supplying the logger.

There are two possible solutions, either we reduce the functionality of `CommandHelper` to the bare minimum, and keep all the logging and translation out of it, doing the loops and logic in `SearchApiCommands`.

The other solution is to not make `CommandHelper` a service, but a regular object that is persisted on the `SearchApiCommands` object instead of on the shared container. We can instantiate it at calltime:

class SearchApiCommands {
  protected function commandHelper() {
    if (empty($this->commandHelper)) {
      $this->commandHelper = new CommandHelper($this->entityTypeManager, $this->logger, $this->translator());
    }
    return $this->commandHelper;
  }

  protected function translator() {
    if (empty($this->translator)) {
      $this->translator = new SearchApiDrushTranslator();
    }
    return $this->translator;
  }

  public function enable($index) {
    $this->commandHelper()->enableIndexCommand([$index]);
  }
}

class SearchApiDrushTranslator implements SearchApiCommandTranslatorInterface {
  public function translate($string, $args) {
    return dt($string, $args);
  }
}

Unfortunately I won't be able to work on this any more for the time being, I've been assigned to another ticket at work.

Thanks a lot for your continued work on this!

The syntax errors are just phpstorm not understanding it. See \Drupal\Tests\Core\Session\AccountProxyTest, \Drupal\Tests\aggregator\Unit\Plugin\AggregatorPluginSettingsBaseTest, (among others in core) where the style I used i also in use.

The syntax errors were not just in PhpStorm, but also when running the tests. Otherwise, I probably wouldn't have said anything.
Anyways, it's clear I was completely mistaken, and the PHP docs also list this as discouraged but entirely valid, so sorry for the (small/potential) confusion. I'd still like to get rid of it, if possible, and otherwise the function at least needs some documentation, but hopefully any translation/logging solution you come up with will take care of that.

Would of course be awesome if Drush itself could include something to help with this problem which, after all, many modules will probably face. Is there maybe already an issue for that, or do we want to create one?

Regarding the problem explained in #42: Is this really a valid scenario? As I understood it, the new service was just for use by the various console interfaces (i.e., Drush and Drupal Console), not for any other part of the code to call. So, having a hook invocation lead to a call to the service doesn't seem like something that could happen, or that we want to support. A request that involves the command helper will always be initiated by either Drush or Drupal Console – or, potentially, even some other application – but then that request will always be "owned" by that specific application. When you execute a Drush command, you want that whole request logged via Drush. And we don't want to use the command service for any part of the framework – it's just a one-way interface from CLI applications to the framework.
Or did I misunderstand the problem? (Or miss something else?)

Regarding the problem explained in #42: Is this really a valid scenario?

Yes if this is a service then it is available by the full Drupal ecosystem and everybody can call its methods whenever they want. And it's methods are really handy too, if I need to clear a search index I can just call:

\Drupal::service('search_api.command_helper')->clearServerCommand('my-server');

If I turn the question around I can let you answer it yourself :)

Should CommandHelper be a full lazy loaded service that is available on the container so it can be used by the entire Drupal API?

As I understood it, the new service code was just for use by the various console interfaces (i.e., Drush and Drupal Console), not for any other part of the code to call. So, having a hook invocation lead to a call to the service doesn't seem like something that could happen, or that we want to support. A request that involves the command helper will always be initiated by either Drush or Drupal Console – or, potentially, even some other application – but then that request will always be "owned" by that specific application. When you execute a Drush command, you want that whole request logged via Drush. And we don't want to use the command servicecode for any part of the framework – it's just a one-way interface from CLI applications to the framework.

So yeah, if you want it to be used only by CLI tools, then don't make it a global service.

Looks like the search method doesn't do actual searching (or at least it doesn't in the test). The result count is correct, but the results aren't added to the result items.

I also fixed the other things that needed tests, I also fixed #42/ #44.

@drunken monkey: if we end up using this patch (or something like it), can we also credit @bircher for the inspiration?

@pfrenssen: Does the latest patch address your concerns about the service?

We can't use the logger from the __construct for some reason. Fixes #48.

@pfrenssen: Does the latest patch address your concerns about the service?

Looks great now! Thanks!

Reviewed this and worked on it a bit, but I now realize it's probably still a bit away from being considered RTBC?
Anyways, I hope the changes and my comments below are still helpful.

1. There are still five @todo comments in the patch. Do you intend to still work on those, or do you want to add the code with those unresolved?
2. Especially the one comment about using dt() in the code seems important. It would really be nice to get rid of that hack in the test file – especially if it's gonna turn the whole file red in my PhpStorm. DX--.
3. While it's now officially OK to use camelCase for normal variables (and method arguments), I really don't want this style mixed with snake_case in the same file. To stay consistent with the rest of the module (minus one or two files where I made the same mistake myself) I'd prefer if you used snake_case throughout the patch. But at least it should be consistent.
4. I'd find it better if the resetTrackerCommand() method took datasource IDs, not entity types. It would be more flexible regarding non-entity datasources and also keep the code simpler. We might to provide a way to get a list of datasource IDs (for an index), though.
5. Is there a reason the SearchApiCommands methods take a single ID, but the CommandsHelper can accept multiple ones? Seems to me that's a) not necessary and leads, in its current form, to the ugly [NULL] special case.
6. Why does listCommand() have options which are then ignored?
7. The coding standards compliance in the whole patch is rather lacking, especially regarding doc comments. I guess that the descriptions for command methods don't start with third-person verbs is on purpose (since, I assume, those are taken as the help text for the commands and Drush likes them like that), but all the missing or inaccurate parameter, return value or exception documentation has to be improved. I already did it for a bunch of methods, but got dispirited upon realizing how many were missing or wrong (or had bad grammar). PHPCS complains about the same thing, btw.
8. I guess it's also still planned to replace the functions in the old search_api.drush.inc file with calls to the new helper class?

The test was failing because you're using the test backend for the server, which doesn't do an actual search (of course). See \Drupal\search_api_test\Plugin\search_api\backend\TestBackend::search().

@ #49: How should $this->logger be already available in the constructor?

1. We should fix them.
2. I looked at the config split code and come up with a solution I hope.
3. Sure - fixing that
4. I'd like to keep this the same as drush 8
5. No, we should fix that.
6. Eh, because I didn't get there yet? :-P
7. I'll fix those in a next iteration
8. yeah, but let's do that in a followup?

But yeah, this is definitely not done yet, I'd like to spend more time on this. Hopefully this patch pushes things in the right direction.

I now added translations to the exceptions - not sure if we should leave those untranslated.

This patch removes some of the todo's where they were no longer relevant.

Awesome, thanks a lot! Looks a lot better already.

I fear the current translation code will have the same problem I described in #2917041: logException() trait method (probably) not working with translation – i.e., the translatable strings won't get properly extracted that way. I think you should either always do $t = $this->translationMethod; in methods where you use it, or just provide a t() method on the object which uses that property. Then, I think, this should be detected properly.

In the rest of the module, as also specified in the coding standards, exception messages are not translated. However, with Drush, when they directly refer to illegal values passed by the user, it seems they could also count as "user-facing", so probably they should be translated, yes.

Doing the Drush 8 migration to use the new helper class in a follow-up would be fine, if someone says they'll definitely work on it soon and get that sorted out. Otherwise, we'll end up with two duplicate sets of code to maintain. (On the other hand: I guess Drush 8 will reach EOL soon, then, so I guess we could then also remove that integration? Might also be OK, depending on the time frame.)

Anyways, thanks for your reply and your continued work! Then I guess, just set it to RTBC when you find you're finished (and another person has already reviewed) and I'll review it again.

Also ported the drush 8 commands.

Fix cs issues for drush 8 code.

Nah, this is not committed yet - so we should fix it here. I knew this solution was too good to be true. Do you happen to know what the php5.6 compatible solution is?

It would also be super awesome if you could test the patch and verify that it works. We really should get this in before we tag a new release. Drush 9 usage will only go up.

Yeah - that sounds like a great idea! That would make this easier, let's make a new method:

protected function translate($message,
 $arguments) {  }

Thomas, can you have a look at the latest patch?

Thanks a lot, great job!

Regarding the translation helper problem, you apparently didn't notice my paragraph on that in #56:

I fear the current translation code will have the same problem I described in #2917041: logException() trait method (probably) not working with translation – i.e., the translatable strings won't get properly extracted that way. I think you should either always do $t = $this->translationMethod; in methods where you use it, or just provide a t() method on the object which uses that property. Then, I think, this should be detected properly.

Changed the method name to t(), accordingly.

Otherwise, I mostly cleaned up the doc comments and (to a smaller extent) the code. I also got rid of all the setLogger() calls in every single command method – just setting it in the command class's setLogger() method should do the job just as well.

1. Is it necessary to have a new …\Consolidation\OutputFormatters namespace, with just a single class (in two directories)? Couldn't that live in an existing namespace – probably Contrib?
2. As already said, please correct variable (and argument) style to snake_case. (Maybe there's an automated script for that?)

I'm seeing this notice when running with this patch:

 [notice] search_api should have an extra.drush.services section in its composer.json. See http://docs.drush.org/en/master/commands/#specifying-the-services-file.

So the patch should probably be updated to specify these services.

Note that this notice that @jhedstrom mentions will only appear when you use the new Drush 9.0.0-rc1 release.

@ mpp: Thanks a lot, great work!

@ #74: Thanks for testing! Does the attached patch resolve that problem?

Yes the notice has disappeared with this new version, thanks

I tested those commands and they work great :
* sapi-c
* sapi-l
* sapi-i

Thanks for your feedback!
Not sure anymore: Was this RTBC then? Does someone else want to review? Or is there still something to do?

I think it is ok, it was RTBC in #70

OK, then. Good to hear.
Committed.
Thanks a lot again, everyone, for all your hard work in here!