Drush integration

Sounds good. You're right, Drush integration is important. Don't know anything about how to do this, though, so: open for patches! ;)
The fact that Search API hasn't got just the one index would have to be taken into account, in any case.

I'll take a stab at this, since we need these ASAP.

Oh, awesome.
Subbing this hard

...That sounded less weird in my head

Since there is no simple way (that I know of) to git add in a patch, here's the drush file. Just change the file name from .inc.txt to .inc.

The following commands are supported.

searchapi-list -- list all indexes
searchapi-status -- show status of indexes
searchapi-index -- run and index (or all indexes)
searchapi-reindex -- mark an index (or all indexes) for re-indexing [UNTESTED]
searchapi-clear -- clear and mark an index (or all indexes) for re-indexing [UNTESTED]

See the note in the header about #704848: Command getting run twice, too.

@TODO: testing and some documentation.

@drunken monkey
These sample commands give you a good idea how to write more commands for servers, etc. It's pretty simple except for the drush function naming bug.

I didn't manage to set up drush, so I can't really test whether these really work. Look good, though. Can anyone else confirm that they work?
I looked at the code, however – here are some comments:

  $indexes = search_api_list_indexes();

search_api_list_indexes() is deprecated, use search_api_index_load_multiple() instead.

    elseif ($limit === 0) {

I don't know about the drush internals, but looks to me like this will always be FALSE, as the passed parameters will probably be strings. In this case, you should use $limit == 0 && is_numeric($limit).

    search_api_index_reindex($index->id);
    search_api_index_clear($index->id);

Use $index->reindex() and $index->clear() instead.

Oh, and: with git diff --cached you can create diffs between the index and staged changes.

Powered by Dreditor.

We have to pass an array of $ids to search_api_load_multiple(), so you can't call it directly.

Fixed that with a small helper load function. Attached file again, since I'm working on a non-Git source.

We have to pass an array of $ids to search_api_load_multiple(), so you can't call it directly.

Just pass FALSE as the $ids parameter to load all indexes.
But thanks for making me discover that this is undocumented, for whatever reason. Fixed this in dev.

Nice. The command in the attached should all be good to go.

We might want to wait on the resolution of the drush issue before committing, though.

Yes, that, and you should probably include a bit more help regarding the arguments that the commands can take.
Also it would be good to get some feedback of another user here, whether the commands all work.

Tested it out. Works really well!
This'll be very handy :)

Only used the commands as is previously - which was perfect.
The arguments it takes are index IDs.
Not sure if this is acceptable or not in drush, but if you pass it an ID that doesn't exist it complains (different errors depending on the command in question)

sapi-s:
WD php: Warning: Division by zero in drush_searchapi_status() (line [warning]
118 of /var/www/sites/all/modules/search_api/search_api.drush.inc)

sapi-r:
PHP Fatal error: Call to a member function reindex() on a non-object in /var/www/sites/all/modules/search_api/search_api.drush.inc on line 154
Drush command terminated abnormally due to an unrecoverable error.

sapi-c:
PHP Fatal error: Call to a member function clear() on a non-object in /var/www/sites/all/modules/search_api/search_api.drush.inc on line 165
Drush command terminated abnormally due to an unrecoverable error

The arguments it takes are index IDs.

Side note: I think that machine names will work equally well.

And thanks for testing – so, another todo: unknown indexes should also be handled correctly (i.e., fitting error message and abort).

@Shadlington

Did you have any indexes when you ran those commands? Or were you running something like drush sapi-i FUBAR?

@drunken monkey
We could make it take machine names, yes, with a little fiddling.

I had indexes. I tested both without arguments and with correct index IDs.

Then I put in a random number for each command and got the errors without making any further changes to my setup.

We could make it take machine names, yes, with a little fiddling.

What I meant is that, as far as I can see, the commands will already work with machine names – no fiddling necessary. The search_api_*_load[_multiple]() functions all work with both IDs and machine names, as documented. (But, as said, I can't test this, so can't say for sure.)

By the way:

    $indexes = array(search_api_index_load($index_id));

I'd use $indexes = search_api_index_load_multiple(array($index_id)); – this is what happens internally, anyways.

Powered by Dreditor.

You should install Drush. It will change your developer life.

Yup, changing that line fixes the error.

Added some error handling that helps work around #704848: Command getting run twice as well. This should be ready to go.

    'examples' => array(
      'drush searchapi-list',
      'drush sapi-l',
    ),

The examples section should always be in the form of:

  'example command' => dt('Description of what the example command does'),

Without the description, drush outputs spurious index numbers for each example.

/**
 * Implements hook_drush_help().
 */
function search_api_drush_help($section) {
  $items = search_api_drush_command();
  $name = str_replace('search_api:', '', $section);
  if (isset($items[$name])) {
    return dt($items[$name]['description']);
  }
}

This should be what drush gives you if you do not implement hook_drush_help at all; you should be able to remove this function.

  if (search_api_drush_static(__FUNCTION__)) {
    return;
  }

Yeah, that works... a bit ugly, but it does work. Seems to me that the rejected workaround of renaming your file to searchapi.drush.inc is preferable, but your preference here. It's too bad that drush does not give any way for a command file to specify the minimum version of drush that it can be used with. #704848 will probably be fixed in drush 4.5 (track that issue to see what other maintainers have to say about that).

If you want at some future time to remove your static fix, you could create a file search_api.drush.load.inc and inside it define a function search_api_drush_load that tests DRUSH_VERSION and returns FALSE if your commandfile should not be loaded. You'd probably want to put in an error message advising the user to upgrade their version of drush too.

See Greg's comments. (Thanks for reviewing!)

Here's the patch with the examples described and the help hook implementation removed.

Thanks!

Attached is a slightly modified version in patch form. I changed the example descriptions to ones I find more fitting, and also removed some check_plain()s (which, I take it, aren't useful for a command line script, or are they?).

I don't think the check_plain() calls hurt. Best practice and all.

I don't think that calling check_plain() in a command-line setting can be called "best practice". After all, if the call should have an effect, you will get an output like "Pins & needles" instead of "Pins & needles".

And what if you name a search index ";DROP TABLES;"?

Some variant of that attack vector may work. By default all Drupal output should be check_plain() or check_markup(), and I don't see why we would change that for the CLI.

Check drush docs for best practice.

And what if you name a search index ";DROP TABLES;"?

Then the table would say ";DROP TABLES;" in the name column. Why should that be a problem? This hasn't got the least bit to do with check_plain(), which only concerns the presentation component.

If you can show me the relevant place in the drush docs, we could change this. (Or, rather, I'd take the discussion to the Drush issue queue.)

I suppose there might be some validity to securing a call to drush if you are planning on calling your drush command from cron, and you might be feeding user data directly to it. However, it is our general philosophy that in such a case, it is the caller who must sanitize the data, not drush. A drush user is generally expected to be a privileged user; we do not guard against a drush user sneeking in a ";DROP TABLES;" attack, because the drush user can usually just type drush @site sqlq 'drop tables;' if they really want to do that. Besides that, would it not be rare for a non-admin to be allowed to name search indexes?

See examples/policy.inc for an example on how to add additional domain-specific validation to a drush command.

Thanks, Greg. Here the sanitized value is user input from the UI.

@drunken monkey

Extrapolate the logic, ;DROP TABLES may do nothing, but could an illicit command get passed to the CLI because of the name of an index?

;mv index.php /dev/null

Maybe. I don't know. Sure it's a pretty rare attack vector, but I would still err on the side of caution.

Reviewing the specific usage of check_plain in #22, there is no chance of the user input being in any way executed when passed to drush_print_table. Simple strings of characters, when passed to a web browser, may include Javascript with xss, etc., but simple strings of characters emitted to the console are not an attack vector. You only need to worry about user input when you are going to pass user input to some command that you are going to exec, and in that case you want to pass them through drush_escapeshellarg, not check_plain.

I'm just being paranoid. I blame jet-lag.

@greg thanks for the clarification.

@drunken monkey

I think the only bit left is the memory handling loop for indexing.

Thanks a bunch for settling this, Greg!

I think the only bit left is the memory handling loop for indexing.

Ah, yes, that bit is still left. Otherwise this seems pretty commit-able, though. (Although, again, I couldn't verify that it's really working. Just have to trust you guys on that end. ;))

You should really setup drush, its pretty damn awesome :)

This adds success messages for sapi-r and sapi-c. Uploading as a text file, but including changes from #23.

Ah, yes, good addition. Thanks!
However, please always use Linux line breaks when contributing to Drupal. Otherwise files can be hard to review and would maybe lead to odd patches if they got committed and worked upon in the future.

OK, lest this lies around for some more months, let's just commit this for now.
Supporting indexing in batches would then be another feature request.

Filed: #1186206: Support batch processing of Drush commands