drush_backend_invoke buffers output till end of command. Show progress.

If you upgrade to drush-HEAD, then you can add '#interactive' => TRUE (put this in your $data array passed to backend invoke) to go into interactive mode. The disadvantage here is that then your script does not get to see and process the output of the command, but the user does get to see the output as it is being generated, and can provide keyboard input to the command.

Give it a try.

I'd actually like to open this back up because it came up for me while I was working on #1053072: Concurrently execute a single drush command on multiple sites (con'd). This is an area where drush can be inadequate compared to tools like Capistrano, where you can run a command on multiple remote hosts at the same time, see the output in realtime, and ctrl-c out of the command if you need to.

Is there a technical reason we can't print the buffers out as we collect them?

That would work, but it would require a reworking of the backend invoke mechanism. The main problem with showing output as we get it is that in --backend mode, drush buffers all of its output so that it can place it into the json-encoded output. This makes the data available to be processed by scripts.

We could change the way that --backend works -- probably by the introduction of a new flag. Drush could then print out ordinary output as usual until the command was finished, then print the drush backend invoke start-marker followed by the json-encoded data structure including the logs, etc.

Once you had that in place, then _drush_proc_open could be modified to read and print the output in real time, up to (but not including) the backend invoke start marker. Everything after the marker would be interpreted as JSON and turned into a data structure; $data['output'] could be added on after the fact.

This sounds like a solid improvement. Make it so, wizzes :)

It is complicated by log messages. Hovever, here's a stab at a solution.

Haven't tried it yet, but #5 looks pretty good. Would be interesting to try backend + interactive; that would likely work, allowing user input and backend results during the same call.

Would be great to get this in Perhaps Mark or Greg can provide a review.

This works great. I tried briefly to make it work interactively, but for some reason it did not work. That might be something to look into later; the patch is a great enhancement in its current form.

Re-rolled against HEAD.

This isn't quite right yet. Previously, when sql-sync called sql-conf, the output from sql-conf was never displayed. In this patch, it is; that will need to be fixed.

This is better. Now output is not passed through in $integrate mode, same as before. Log messages are still recorded in real time.

Calls to ob_start() and ob_end_clean() nest, so I did not need to wrap the calls to them in drush_bootstrap_full.

Re #9:
Wouldn't it make more sense that sql-sync called sql-conf with a quiet switch?

Alternatively, a switch for backend invoke to not show output. May be that I've not understood integrate properly, but it sounds odd that it should affect whether output is displayed or not. There might be a usecase for using integrate while still showing output.

Yes, maybe, but I thought it was better for now to maintain current behavior, which tied integrate with output.

I did consider using --quiet with the call to sql-conf, but I think I decided on #10 to preserve compatibility, so that you did not have to upgrade your local and remote drush in lock-step. If others think that #12 is an improvement, we could go that route.

I just realized that I was properly too eager to delete code. The integration function should still reprint log messages, if it didn't see the magic log message marker, in order to let newer local drush work with older remote drushes. Perhaps with an notice for the user that upgrading the remote drush will enable realtime logging.

But if we fix that, wouldn't using --quiet work for the different permutations of new/old drush?

New invoking old:
Output it printed realtime, log is collected and printed after command completion.

Old invoking new:
Output and log is collected and printed after command completion. However, the user will see the DRUSH_BACKEND_LOG_START>>>message<<

We might consider having the backend command output a DRUSH_BACKEND_DISCARD_START>>> If you see this message, your local drush is too old to understand the new inline log message format, please upgrade to enjoy realtime logging.<<

Unless I'm talking out of my backside here, I've spent the last week renovating house, and are working off my patchy memory of the issue.

#14:

New invoking old: Old does not output anything until it outputs the backend log start marker. No realtime output -- in fact, you don't see any output at all, because new ignores the 'output' item in the result.

Old invoking new: Old does not try to output anything until it reads in the backend log start marker. No realitme output.

The suggestion in #14 does not help the user, because the new message is not seen. There is no change that you could make that would allow the message to be seen when one of the ends was "old".

I can still add the 'quiet' enhancement, though; it would just require that the log message call 'ob_end' in quiet mode before they output, and 'ob_start' again when they are done. Information is lost, but that's okay, because quiet mode is supposed to discard output.

Well, the killer is the old calling new, there's really no way the new can both output and collect output (using ob_*) at the same time.

Some of the other issues could be fixed by some creative coding, but it would make the code rely too much on magic for my liking.

However, I have another idea that only relies on one sneakyness.

Change the meaning of the backend command line switch from a simple switch to a 'requested protocol version option'. As I read the code, the existing 'old version' shouldn't care whether it gets --backend or --backend=2. That's the sneaky part.

That enables the backend invoked drush to figure out what's calling it, and behave accordingly. Old drush will just do its thing as usual, new drush can determine that it's an old drush calling it when the DRUSH_BACKEND context === TRUE, or new if it's === 2. Or alternatively casting to int as TRUE should be 1 then. When it knows that it can use the new realtime logging or behave as the old drush, accordingly.

Secondly, the backend invoked drush should for --backend=2 (or any other value distinct from --backend) start its output with a protocol identifier something like:

DRUSH_BACKEND_VERSION>>>2<<<DRUSH_BACKEND_VERSION\n

This allows the calling drush to determine that the backend invoked drush understood the version. It shouldn't do any special processing of the output (like handling the magic log marker) until it has seen the version marker. If the command completes *without* outputting the version marker, the calling drush should fall back to process the JSON output like the old drush.

It means more compatibility code, but at least it will be explicit. It will also allow for future bumping of the communication protocol, if new features pop up.

If the calling drush requests a version that the backend invoked drush doesn't know (be it 3, 5 or 'banana'), it should fall back to *not* outputting a version marker, and behave like the old drush. This means buffering, but will ensure that it still works. It also means than any future version of drush only need to support 2 protocol versions, the old (current) one and any given future one, in order to function with any other version of drush. Unless someone thinks we really need a protocol negotiation protocol? :-)

How does that swing with people?

I like your thinking.

The protocol version 2 (--backend=2) is not necessary to solve our immediate problem. The new code can detect the old protocol because there will be no output before the start marker, and there -will- be an 'output' object in the json data. It would be simple enough to print and merge the 'output' object once it is available; this would restore compatibility with new -> old invokes.

To "future proof" our code, we could adjust the format of the current realtime log messages so that they are wrapped in a drush "backend packet" that includes a start packet marker, a packet type identifier (currenlty only "log" supported; we could add "backend verison" later), maybe some optional parameter space followed by data and then an end-packet marker. Sort of like HTML tags, but without nesting. If we just swallowed and ignored any unrecognized packet, then we could easily add more realtime metadata to the backend invoke stream later as needed.

Example:

DRUSH_BACKEND_MSG:LOG, T="0.13">>>%s<<<DRUSH_BACKEND_MSG_END[cr]

I added a timestamp to the log message just to demo parameters. Another example with no payload in the data:

DRUSH_BACKEND_MSG:META, DRUSH_VERSION="5.1"[cr]

Other message formats would work equally well. Want to try your hand at coding that up?

As for a protocol negotiation protocol, when we have a need for that, we should just implement SpNego for drush. (Just kidding!)

Hmmm, yes...

But when old invokes new, new will still output regular output immediately, which means its missing from the output part of the JSON response. As they say, we can't have our cake and eat it too.

One possible solution for dual-state cake: Setting a callback in ob_start(), and the chunk size to 2. The callback should then collect the output for adding to the JSON reply. The chunk size should ensure that the buffer is flushed repeatedly, simulating no buffering.

If that doesn't work, I can't see a way for old invoking new to work, without using the --backend=2 trick.

As for the generalized packet format, I'd suggest something more like:

\0DRUSH_BACKEND:[JSON]\0\n

Yes that's null bytes, they're unlikely to appear in normal output, and we can escape them in the content of the [JSON] if need be. I was thinking of retaining just one JSON encoded message blob instead of using command/command arguments and introducing a new sub-parser. The reader would then just json_decode the data and check $data['command'] for a known command. Much like D7 AJAX command arrays. With proper naming it could be as simle as:

$data = json_decode($string);
if (isset($data['command'] && function_exists($function = 'drush_backend_command_' . $data['command'])) {
  $function($data);
}

The individual commands can then throw as many parameters as they want in the array.

> Want to try your hand at coding that up?

Love to, but I'm rather tied up currently, so it'll be next week at the earliest.

Yes, good point; when new is invoked by old, it should include its output in the 'output' object of the JSON result. The problem with this is that we'd have to go back to ob_start / ob_end for --backend calls and supress the new log packets. I'm inclined to just say that new cannot be invoked by old; perhaps my desire for compatibility in #13 was baseless, since it did not actually go far enough. We would need to keep a fair bit of old code around to allow it to work.

I like your improved packet format in #19.

Consider also #1063380: Backup directory security warning not appearing when running pm-update using @sites alias

New attempt that should address some of the issues brought up. Rolled against 4.x head (9f3adc9).

Let's see if I forgot something.

Removed stray print_r.

And a fix for backend commands running backend commands (hairy).

Code looks good.

There are a couple of places that use the word "command" differently than drush currently does; I would name these something else to avoid confusion. The function drush_backend_command would be better named drush_backend_print_output. The function drush_backend_parse_commands would be better named drush_backend_parse_packet_messages (or embedded_messages?). I would also make the pattern drush_backend_message_.

Does this work with #1063380: Backup directory security warning not appearing when running pm-update using @sites alias? Note that the situation described in that issue is strictly non-interactive, so this might already be okay. I don't have time to test it right now, though.

Overall, this is looking really good.

You're right.

drush_backend_command -> drush_backend_packet
drush_backend_parse_commands -> drush_backend_parse_packets
drush_backend_command_log -> drush_backend_packet_log, etc.

Don't know about #1063380: Backup directory security warning not appearing when running pm-update using @sites alias, but it might.

I think I still need to find some places where $integrate should be checked.

I'm wondering about #9? Is that fixed by this, or should there be a -q switch added to the sql-conf call? (I don't know where the problem lies).

Another thought: _drush_proc_open reads stderr after stdout, causing stderr to appear after normal output. Wouldn't it be smarter to redirect stderr to stdout on the command, and just read stdout? If there is a portable way to do that. Would simplify creating a multiplexing _drush_proc_open (for invoking multiple commands on multiple hosts) later.

Drush currently redirects stderr to stdout when doing a backend invoke, but that can cause some problems, e.g. if the remote host is Solaris w/ csh instead of bash. I'm not saying that we are going to formally support bash, but I think it would be better to send log messages out to stdout instead of stderr in backend mode, as I believe this patch already does. As long as log messages are on stdout regardless of the value of $integrate ("interactive" mode -- there have been some suggestions to split this mode into two separate flags), then we should be okay.

I'm not sure if you changed anything that broke sql-conf, but I'll look at it when I test this. The problem with adding -q to sql-conf is that at the moment, -q breaks --backend, unless you happened to fix that here too. Adding -q to sql-conf is viable if fixes like that are made at the same time.

Added handling of quiet mode, by using another output buffer handler.
Renamed functions and vars.

drush_backend_packet_set_error - not called and not needed?

drush_backend_parse_commands - rename per #26.

Looking pretty good.

The packet is created in drush_set_error. I guess that's what might fix #1063380: Backup directory security warning not appearing when running pm-update using @sites alias as it ensures that errors is set in the invoker too.

Renamed drush_backend_parse_commands.

Removed escaping of special chars, as json_encode already does it for us.

Testing #31 to try fixing #1063380: Backup directory security warning not appearing when running pm-update using @sites alias I found that drush @sites cc stops working.

Without #31:

You are about to execute 'cc' non-interactively (--yes forced) on all of the following targets:
  #d6
  #adepsi
Continue?  (y/n): y
#d6     >> Enter a number to choose which cache to clear.
#d6     >>  [0]  :  Cancel         
#d6     >>  [1]  :  all            
#d6     >>  [2]  :  theme-registry 
#d6     >>  [3]  :  menu           
#d6     >>  [4]  :  css-js         
#d6     >>  [5]  :  block          
#d6     >>  [6]  :  module-list    
#d6     >>  [7]  :  theme-list     

#d6     >> Cancelled

#adepsi >> Enter a number to choose which cache to clear.
#adepsi >>  [0]  :  Cancel         
#adepsi >>  [1]  :  all            
#adepsi >>  [2]  :  theme-registry 
#adepsi >>  [3]  :  menu           
#adepsi >>  [4]  :  css-js         
#adepsi >>  [5]  :  block          
#adepsi >>  [6]  :  module-list    
#adepsi >>  [7]  :  theme-list     

#adepsi >> Cancelled

With #31:

$ drush @sites cc  -y 
You are about to execute 'cc' non-interactively (--yes forced) on all of the following targets:
  #d6
  #adepsi
Continue?  (y/n): y
\n
\n

(added \n to denote the white lines)

I've traced it down to:

1. the fact that _drush_proc_open() only prints the output if interactive/integrate.

backend.inc:330:
        // Pass output through in $integrate mode.
        if ($integrate) {
          fwrite(STDOUT, $string);
        }

2. _drush_backend_invoke() --the caller of _drush_proc_open()-- calls drush_backend_parse_output(), that only includes the output in its output array, again, if interactive/integrate.

backend.inc:226:
      if ($integrate) {
        _drush_backend_integrate($data, $outputted);
      }

Hmm, yeah, that kinda snuck in.

Not difficult to fix, but let's just take a step back and think:

What's the usecase for running a backend command and not displaying output? It's useful for one command to parse the output of another. I believe sql-sync runs sql-conf to get the mysql command to connect to the db.

What's the usecase for running a backend command and not integrating the log messages? None, I'd say, it's pretty much the reason it exists. Else you might as well use passthru() or the backtick operator.

If that't the case, I'd say that _drush_proc_open should always re-log log messages, and have a switch for whether to display command output defaulting to true, and the integrate switch should be removed. Or changed to a 'don't integrate' switch.

Opinions?

The use case for the non-integrate case is when the result of the backend invoke should not affect the result of the command that called it (or equivalently, if the calling command handles the error conditions). pm-update calling updatedb is an example where integrate is appropriate, and sql-sync calling sql-conf is an example of where non-integrate is appropriate. The integrate flag should stay, with integrate meaning that log messages and error results should be merged into the caller's context.

As for the non-interactive case of multiple-command execution, if I had time to fix this, I would add a special '#prefix' option to $data, and put "dev >>>" (or whatever) in that. That way, the part of backend invoke that reads and prints the command output could handle output prefixing. It would then be a simple matter to make the multiple-command code use that feature, and as a side benefit, you'd see your output in real time, even though it is non-interactive output.

I might be looking the wrong place, but it seems to me that pm-update is calling updatedb without integrate: http://drupalcode.org/project/drush.git/blob/refs/heads/master:/commands...

Furthermore, I haven't been able to grep my way to an example of using backend with integrate set to false.

In any case, that's really a case of the drush_set_error handling, which is already separated from the logging in the latest patch.

So we may need optional set_error, but should that really also kill logging? The log from the backended command (possibly prefixed so it's obvious that it's a backend command) might be useful for debugging, even if the invoking command is handling it if it errors out.

The reverse case (no logging, but set_error'ing) seems superfluous to me.

As for the multiple-command execution, that's where it makes sense to not having errors set (it would probably set its own error).

We need to check with the Aegir folks before changing the way that integrate works. It would be safest to keep the behavior of integrate the same.

I'll dig through their code and see what I find.

But so far, I figure that we can redefine integrate to 'integrate errors' and always integrate log entries and displaying output, without it breaking anything. Things will just get more verbose.

I don't think that we should integrate log entries when $integrate is FALSE. I think it would probably be okay to change the way that output works vis-a-vis log and stdout wrt $integrate, as long as there was a way to make it quiet again (e.g. fix --quiet with --backend).

I've grepped aegir, and not found a single instance of running backend commands with integrate set to false. There is some instances of using drush_get_error after a backend command to handle errors.

I do understand the reasoning for not changing the meaning of integrate, but I think there's a fundamental problem with it as it is.

Seems to me that the finer distinction between output and logging in backend commands is lost on most end users. Drush make and Aegir both uses logging for pretty much all output, so if you run them non-integrated (as to not inherit their set_errors), you loose all output.

The only usage of non-integrate I've seen is in drush_do_site_command, and I'm not quite sure why it's not integrating. Most of the callers isn't doing error checking anyway, causing errors to be silently ignored, which seems like a bug to me. Especially in drush_remote_command, where a failure to execute the command on a specific host would make the user rather interested in any drush_log(.., 'error') entries.

I believe the current behavior is a source of subtle bugs, when commands use non-integrating backend commands, and forgets to handle errors properly (#1063380: Backup directory security warning not appearing when running pm-update using @sites alias is an example, where the attempted fix is to print the log that drush_do_site_command suppressed).

If it is indeed the case that Aegir does not care about the $integrate FALSE case, then it is okay with me to fix up the way this code branch works (or perhaps fails to work) in drush core.

It would be good if someone from Aegir could chime in here and confirm this is okay.

Cross-posted #1166530: Aegir use of $integrate = FALSE in drush backend invoke.

Possibly a bit out of scope for this bug, but wouldn't it make sense for the integrate switch to be moved to $data['#integrate']? I see that $data['#integrate'] is checked at least in one place. If there really is a need, output suppressing and logging suppressing switches could be implemented in the same fashion.

Yes, the various backend calls were passing too many parameters, so $data['#foo'] is the new preferable mechanism for shuttling metadata to the backend invoke functions. Poor man's OOP; we should perhaps recode all of this someday, I don't know. :p

Regarding quiet, perhaps if we do rework as you suggest, if #integrate is FALSE, we should set #quiet to TRUE if it does not already have some value. This might be the best compromise in terms of making the code work the way it should while still retaining some flavor of the existing behavior.

Edit: --quiet is an ordinary flag, so it should be 'quiet', not '#quiet'.

Not to fond of setting -q if the #integrate is false, if -q is not specified. Seems like a switch with sideeffects.

However, another idea, now that you're saying 'too many parameters', changing the signature:

From:

function drush_backend_invoke($command, $data = array(), $method = 'GET', $integrate = TRUE, $drush_path = NULL, $hostname = NULL, $username = NULL) {

To:

function drush_backend_invoke($command, $data = array(), $options = array()) {
   if (!is_array($options)) {
     $options = _drush_backend_oldstyle_options(func_get_args(), 1);
   }
   $options += array(
     'method' => 'GET',
     'output' => TRUE,
     'log' => TRUE,
     'errors' => TRUE,
     'prefix' => $command . ': ',
   );

   ...
 }

function _drush_backend_oldstyle_options($args, $offset) {
  $options = array();
  list($data, $options['method'], $options['errors'], $options['drush_path'], $options['hostname'], $options['username']) = array_slice($args, $offset);
  $options['errors'] = array_key_exists('#integrate', $data) ? $data['#integrate'] : $options['errors'];
  $options['output'] = $options['log'] = $options['errors'];
  return $options;
}

(and all the other drush_backend_invoke functions)
Might also consider a _drush_backend_default_options, as to not repeat ourselves. That there should be completely backwards compatible, but wont fix the bugs we've fixed with the latest patch, but that can be worked on. I've added switches for output (-q doesn't work if the invoker wants to use the output, but not display it to the user) and logging (for completeness) and setting errors. And thrown in a prefix argument for good measure.

Two birds with one stone, we get the method signatures cleaned up (without loosing backwards compatibility) and get more flexible option passing.

Whuddya think?

Yeah, that's pretty good, except that $data is really "options", so maybe the new array should be $meta_options or something.

Don't like the meta-ness of $meta_options. I'd say that the invoke command get first dips on the "$options" name, as it's options for the invocation, which comes before the options for the command. But I agree that $data is a bit to generic, $command_options perhaps (as $args is taken by invoke_args)?

Well, pondering it, I prefer the Solomonian: drush_backend_invoke($command, $command_options = array(), $backend_options = array()), even if it is a bit more verbose, it's as clear as it gets.

However, we'll have to change "$options['output'] = $options['log'] = $options['errors'];" to "$options['output'] = $options['log'] = TRUE;" in order to fix the issues that's cropped up with commands failing oddly when using $integrate = FALSE.

I'll work on it.

Your suggestions sound good; I'll look forward to the patch.

Well that made the patch size explode..

Code looks good. The only thing I noticed is that in _drush_bootstrap_drupal_full(), you have wrapped ob_start and ob_end to not be called if they were already called by DRUSH_QUIET. I know I did this in some previous patch, so perhaps you picked it up from me, but it is not necessary -- ob_start and ob_end can be nested.

I'll try to find some time to test this soon.

Well, this is getting pretty slick. Attached is a new patch rolled against head.

I love the way the code is working, but there are a couple of minor things that still need fixing.

_drush_backend_oldstyle_options should be removed. It does not always work; if a client calls a function using the old-style options, but does not pass all of the old options (relying on default values in the function arg list), then you will get an error in the array_slice (not enough items). I also don't like the 'trickyness' of this function; it makes the code harder to read and follow. If the old function name is kept for backwards compatibility, then it should keep its old signature. The new function, with the $backend_options item, should be given a new name even if the old function name is not kept (better to get a fn-not-found error than have args interpreted incorrectly).

I renamed some of the elements of the backend options for consistency; now the backend options look a lot like a site alias. So much so that I wonder if the new array is needed at all; perhaps we could just merge the backend options into the site alias, and call drush_backend_invoke_sitealias as needed?

Now that backend options exist (either as their own array, or as part of the site alias), it seems that #integrate and the other metadata options in the $command_options should be moved to the backend options array. If backwards-compatibility is desired, then #integrate and friends could be copied to the new location.

Finally, drush @site1,@site2 status fails to bootstrap. It seems that drupal_error_handler is called before core required modules are available, so filter_xss is not found when called. Reproducible with drush @site1 status --backend --quiet. That's as far as I got.

I just thought that perhaps I was wrong in #49; when I put back the if (!drush_get_context('DRUSH_QUIET', FALSE)) { wrapers around the ob_ functions in bootstrap full, drush @site1 status --backend --quiet started working again, but the multi-site invokation drush @site1,@site2 status is still failing as described above.

In any event, it does seem that nested calls to ob_start are bad when you have a callback set, as you do; perhaps this is what is happening for the multi-command case too? Sure enough, if I make sure that @site1 and @site2 are both remote, then the multi-site command works again (but is missing its prefix). Not sure about the right fix, but there's the cause, anyway.

FWIW, I'd rather have easily grokkable code than backwards compatibility. It is reasonable to expect similar versions of drush on both sides of the call.

Also, I'd love for you guys to look at tests/backendTest.php and suggest additional tests. So far, we have gotten away with not actually performing a remote call there. It would be nice to preserve that but I'm not sure it is possible. If not, maybe we trick drush_is_localhost() to think that 127.0.0.1 is a remote domain during the tests?

I agree that we should remove backwards compatibility from the function names/parameters in the code. I am more ambivalent on whether it should be possible for drush-5 to call drush-4 via backend invoke; however, since it would take a lot of code, and a bifurcated code path that behaved quite differently in each branch, I think you are right that it is wiser to break compatibility and move forward to clean up the code.

Regarding local calls, in #1163234: remote custom php path breaks drush, a "backend" call is forced if --php-options are set. Something similar is going to be necessary to fix backend calls that redispatch (per #51, above). Both of these are "local" backend calls, though; perhaps what we need is another flag to $backend_options that force "remote" backend calls w/ ssh if set. We could then use that in the tests.

Here is a new patch that rolls in #1163234: remote custom php path breaks drush and fixes the problem with drush @site1,@site2 status. Note that the multi-site code is still running with deferred output, and adding the prefix on at the end of the call. The prefix-adding code could be moved to _drush_proc_open; if this was done, then multi-site commands could be called in a way that allowed output progress to be shown.

Code cleanup per #50 and tests per #52 still needed.

Breaking parameter compatibility it is then, but I'd suggests a:

if (!is_array($backend_options)) {
   drush_set_error("A suitable text.");
  return drush_set_error('DRUSH_FRAMEWORK_ERROR', dt("Some suitable text"));
}

It'll ensure that those that have created commands using the backend options will get an error message instead of weird failures.

However, I'd prefer to keep compatibility between different versions locally and remote. As an example, I'm working in a shop where we're recently created a new drush based deploy script (which of course use backend invoke) for a new large D7 project. At the moment theres at least 3 companies involved in the project, plus freelancers. We're also going to use the same script for the existing D6 installations, which is 21 pubilc libraries of varying sizes (some with their own development team, others relying on service providers), involving at least 7 development shops (and assorted freelancers). Getting everybody to upgrade Drush in lockstep is somewhere I don't want to go.

But I believe it is already working, and is not too horrible to look at.

I haven't quite decided what I think about merging site alias and backend options yet.

I'll have a looksee at testing, cleanup and prefix when I get some time.

I'd certainly be willing to consider a compatibility mode along the lines of your --backend=2 suggestion above. If it did not make for too much duplicate code, it would probably be okay. The contract for backend invoke, in terms of parameters and the json result text, has not changed too much, so I think this should be possible.

Regarding parameter incompatibility, I would prefer to just rename the function to something else rather than add additional code to test for the wrong parameters. I could go with your suggestion if others thought that keeping the name of the function was important. drush_backend_invoke is the "important" name, and it's gone at the moment. :)

I don't think my idea of merging the site alias and backend options was good, but I do think that it is good for the two structures to be similar (parameters with the same purpose should have the same name).

Progress snapshot: none of the code cleanup work done, but fixed some bugs esp. w.r.t. multi-site command output, which is now shown with the label prefix in real time.

Fiddling with backwards compatibility; not completely there yet, but largely working.

Note in the new backend_invoke, we use --quiet to suppress output; however, in drush-4.x, --quiet turns off all output, including our most-critical backend results json. Therefore, I modified the new code to convert --quiet into --backend-quiet, which is treated like --quiet by drush-5, but ignored by drush-4.

sql-sync (fetching remote db via backend invoke sql-conf) from drush-4 to drush-5 works. The same operation from 5 to 4 also works, but the output is printed (since --quiet is converted to --backend-quiet and then ignored). There are two ways to fix it: have drush-5 detect that drush-4 is being called, and suppress the printing of the output. Or, stop adding --quiet to backend calls to suppress output, and tie output to $integrate as it was in drush-4, like I suggested above.

multi-site commands from drush-4 to drush-5 does not work -- no output. multi-site commands from drush-5 to drush-4 works, although the prefix label is missing.

single-site remote commands work in both directions.

There's no need for --backend, as I see it, _drush_proc_open makes a note about whether it saw any realtime output, and tells _drush_backend_integrate, which does the logging and printing if there was no realtime output. That should fix inter-version compatibility.

So we create one backend_invoke that takes a site alias, and axe all the others?

Regarding output-label: I already added the prefix option, which was meant for the same purpose. I defaulted it to "command-name:", as I think that's a good default, to make it obvious which output is from backend commands. I also planned to have the log command apply the same prefix.

Also, I created the $backend_options['output'] option to suppress normal output, as I figured out that using --quiet is problematic. A command might want to get the output of another for parsing, but not show it to the user, and the cleanest way is to let _drush_proc_open handle it.

I came to the same conclusion about the --quiet flag; it should be indicated somehow (e.g. in backend_options) and then handled by _drush_proc_open. If the option is not set, then the default is quiet when $integrate is FALSE and not quiet when $integrate is true.

The output label should default to empty. I am a little ambivalent about whether the log messages should have the output label or not; I kind of like the way it looks now, with "real" output labeled and the logs not labeled, but I can see the utility of doing it the other way too. I suppose this does not matter too much to me.

--backend is needed on the receiving side, to tell it to output json results. I agree that it does not need to be set on the calling side.

Regarding the code cleanup, I had an idea just this morning that I think works that is halfway between your original idea and my thoughts. If we keep the signature like so:

Original:

function drush_backend_invoke_args($command, $args = array(), $data = array(), $method = 'GET', $integrate = TRUE, $drush_path = NULL, $hostname = NULL, $username = NULL, $ssh_options = NULL) {

New:

function drush_backend_invoke_args($command, $args = array(), $command_options = array(), $backend_options = array(), $integrate = NULL, $drush_path = NULL, $hostname = NULL, $username = NULL, $ssh_options = NULL) {

In this instance, it is okay to check the type of $backend_options to see if it is a string or array. Your conversion function will now work, because func_get_args will always be filled out by the default parameters. However, despite the benefits of brevity and reusability of your conversion function, I think I'd still rather see a series of if (isset($integrate)) { $backend_options['integrate'] = $integrate; }. It's more readable. I don't feel too strongly about this, though. (Edit: If we are just calling through to the 'new function', though, I suppose this dual-mode arg list is not necessary.)

These functions can then just call through to the new function you mention that takes a site alias. As long as we are working on backwards compatibility of the backend communication, I think I'd also like to see the functions with the old signatures stick around until drush-6.x. We can mark them as deprecated, and they won't "get in the way" much.

Not sure when I'll be able to work on this next; feel free to pick it up again if you have time. I'll post here if I start working on it again.

Unless you two want be the only ones on the planet who grok backend.inc, I suggest you remove deprecated functions, and strongly consider ditching backwards compatibility. A squeeky clean backend.inc is hard enough to grok on its own.

I opened #1175446: Best practice for upgrading shared drush to next major version so we can discuss compatibility. I don't think all sites need to upgrade in lockstep. See that issue for a strategy.

I think we are pretty close to maintaining backwards compatibility in the backend invoke with only a little code. Conceptually, the 'new code' will take the output from the union of the standard output before the backend invoke marker (the interactive / real-time output) and the contents of the json 'output' element (the buffered output). This will handle new calling old with little adjustment needed. Old calling new is just slightly more complicated; we only need to 'turn off' the the real-time output (via ob_start) and accumulate the buffered output as before. As for the deprecated functions, they will only be a few lines of code that call through to the new functions. We can move them to the bottom of the file beneath a marker that says "Grok not, all ye who read past here."

Because of the large number of modules that implement drush commands, and the impracticability of upgrading all of them in lockstep, I think it is important to both do as you suggest in #1175446 and maintain a certain level of caution with backwards compatibility and deprecated function names, where possible. Certainly some things will break from one major version of drush to the next, but I think that the backend invoke protocol is something we should try a little harder to keep compatible -- not forever, but at least overlapping one major drush version change. I for one am willing to do a little extra maintenance work to make this happen.

Had a bit of time to work on this, but have to stop now. Almost done, though. --backend-quiet is gone again, in favor of using $backend_options['output']. New can now call old, and I did one test that seemed to show that old could call new, but I think it might have been a bad test, because I expect that should not work at this point. Need to test more. Calling sql-sync from new to old or old to new does work, though, so perhaps that is a sufficient level of compatibility. If you wanted to break backwards compatibility of the backend invoke protocol, there is very little code that you would be able to remove anyway, so I think keeping at least this level of compatibility is good.

Still to do: put the function signatures back the way they were, and write a unit test or two.

All cleaned up. As mentioned previously, compatibility with backend invoke in drush-4.x is done in such a straightforward way, you will probably not even notice the code that makes it possible. Similarly, there are only three deprecated functions at the bottom of backend.inc helping to bridge the gap between drush-4 and drush-5 functions.

Works pretty well; just needs review, testing, and perhaps a unit test or three.

I fixed this up some more; --backend with --pipe now works, and all unit tests pass. There was one other remaining issue where the new backend packet mechanism was causing bootstrap to fail if a log was omitted prior to the bootstrap full phase; this problem was avoided by insuring that the packets were output to STDERR instead of STDOUT, which avoided the "Cannot modify header information - headers already sent" message. This message was undesirable but innocuous when it was first introduced by a patch above, but in Drupal 6.22 it became a problem due to a call to filter_xss that comes too early. I'll take this up in a Drupal core issue (if there isn't one already; doesn't seem to be a problem in Drupal 7), but it seems best to avoid perturbing the bootstrap like this.

Regarding making backend calls to a "remote" machine that is actually local (per #52), the problem with using ssh to run tests is that this would introduce a new requirement that you have a public / private key set up from your local machine to your local machine, which to me does not seem like a good thing to require of users. I added a test that checked to see that the output of the drush command appears before the backend invoke json output. The backend invoke mechanism itself is already extensively tested by many other unit tests that call it implicitly.

Everything is in pretty good shape now.

For reference, the filter_xss issue is #1174496: WSOD: drupal_error_handler() assumes filter_xss() is defined.

Is there some way to test the new feature (showing progress during a run)?

Hopefully Xen can review this.

+++ commands/core/upgrade.drush.inc
index 0de3bab..893eb71 100644
--- commands/core/variable.drush.inc

Trampling on a recent commit.

+++ includes/backend.inc
@@ -60,12 +60,18 @@
 define('DRUSH_BACKEND_OUTPUT_DELIMITER', 'DRUSH_BACKEND_OUTPUT_START>>>%s<<<DRUSH_BACKEND_OUTPUT_END');
+define('DRUSH_BACKEND_PACKET_PATTERN', "\0DRUSH_BACKEND:%s\0\n");

Lets add Doxygen for constants like D7 does.

+++ includes/backend.inc
@@ -138,24 +159,69 @@ function drush_backend_output() {
+ * Output buffer functions that discards all output but backend packets.
+ */
+function drush_backend_output_discard($string) {
+  $packet_regex = strtr(sprintf(DRUSH_BACKEND_PACKET_PATTERN, "([^\0]*)"), array("\0" => "\\0"));
+  if (preg_match_all("/$packet_regex/s", $string, $matches)) {
+    return implode('', $matches[0]);
   }
 }

I don't see an ob_ function here. Lets not call this output buffer function.

+++ includes/backend.inc
@@ -259,57 +344,100 @@ function _drush_proc_open($cmd, $data = NULL, $context = NULL) {
+	  $function($entry, $backend_options);

leading tabs here, and 3 lines below

+++ includes/backend.inc
@@ -259,57 +344,100 @@ function _drush_proc_open($cmd, $data = NULL, $context = NULL) {
+ * n.b. Prefer drush_invoke_sitealias_args to this function.

Put parens at end of function name so that a.d.o will hyperlink to function page.

+++ includes/backend.inc
@@ -330,47 +458,53 @@ function drush_backend_invoke_args($command, $args = array(), $data = array(), $
+ *	  Optional. Defaults to 'GET'.

Many tabs in this Doxygen block

+++ includes/environment.inc
@@ -642,22 +642,33 @@ function _drush_bootstrap_drush() {
+    // strait away.

straight

+++ tests/backendTest.php
@@ -62,6 +62,25 @@ class backendCase extends Drush_TestCase {
+    ¶

leading tab, and 3 lines below

drush_backend_output_discard is used in _drush_bootstrap_drush as the function callback to ob_start like so:

ob_start('drush_backend_output_discard', 2);

This is for --backend calls with --quiet, to insure that log messages are still recorded in the output. So, this function is nominally related to the ob_ calls; do you still think it should be renamed?

The other suggestions from #67 are included in this patch.

ah, ok. no need to rename.

sub

fixes #1063380: Backup directory security warning not appearing when running pm-update using @sites alias. Haven't look further the patch.

$ drush @sites pm-update
You are about to execute 'pm-update' non-interactively (--yes forced) on all of the following targets:
  #default
  #other7
Continue?  (y/n): y
Backup directory /var/www/drupal7/backup found. It's a security risk to store backups inside the Drupal tree. Drush[error]
now uses by default ~/drush-backups. You need to move /var/www/drupal7/backup out of the Drupal tree to proceed.
Note: if you know what you're doing you can explicitly set --backup-dir to /var/www/drupal7/backup and continue.
Backup directory /var/www/drupal7/backup found. It's a security risk to store backups inside the Drupal tree. Drush[error]
now uses by default ~/drush-backups. You need to move /var/www/drupal7/backup out of the Drupal tree to proceed.
Note: if you know what you're doing you can explicitly set --backup-dir to /var/www/drupal7/backup and continue.

To see progress during a run, apply latest patch from #1002658: Drush does not correctly handle D7 project status queries and just drush up on a d7 site. l10n-update is other candidate that probably need some work.

Looks pretty good too me... In the attatched patch:

Fixed typo and whitespace.
Remove extra empty lines in output.
Added output-label to log entries.
Added output-label to errors.
Added default output-label.

I still think that the output-label should be defaulted to "command: " for local commands, and "command@hostname: " for remote commands, to make it obvious to the end user that a backond command is the source of the output/log. A command can still set it to the empty string if it really wants to suppress it, but I believe that default prefixing is what most people would want.

For everyones sanity, I've also attached the diff from show-progress-in-backend-invoke-68.patch so the above fixes can be seen in isolation.

Good additions in #73, and all tests still pass.

The only thing that I don't like at this point is the default output label. I would prefer the output label to be empty unless the command is a multi-site command; I also have some ideas about allowing users to specify a template where they can specify substitutions such as %remote-host, so everyone can have the kind of labeling they desire. However, I would prefer to go ahead and commit as-is, and fine-tune the label output in a separate issue. #73 greatly enhances the usability of backend commands, and getting this committed will unblock a number of dependent issues.

With #73 the output for #1002658: Drush does not correctly handle D7 project status queries is:

pm-updatestatus-batch-process: Checked available update data for Actions permissions.                              [ok]
pm-updatestatus-batch-process: Checked available update data for Admin.                                            [ok]
pm-updatestatus-batch-process: Checked available update data for Bulk Export.                                      [ok]

The label should be empty in this case (no remote, no multisite, just a batch process).

#75 could be fixed by removing the default output label (one line deletion) per my comments in #73. Perhaps we should start out this way, and discuss how to allow the user to set default output labels (via drushrc.php) in a separate issue.

Any objections to me committing this per #76?

Re #75, there is some oddness. Invoking a command on a site alias doesn't print labels, though it's a backend command.

Rip it out, we can add it again in another issue.

Re #78, there is a code path that short-circuits the call to backend-invoke if the site alias is on localhost; run with -d and you'll be able to see for sure if your command is being run as a backend command.

If there are no objections, I will commit shortly per #76 (w/out the default output label).

#76ers rocks

Tried this on test-run command and it works - I got lots of progress info. test-run uses backend via drush_do_command_redispatch('test-run', array($class));. The label for the output here is test-run:. It would be better if the label were the $class thats being tested, instead of the command name. I imagine updatedb would want a similar label when it calls back into itself.

Greg is welcome to commit this and work on followups in new issues.

Committed. Thanks, Xen for the patch, and all of the reviewers too -- this is a great enhancement to drush, and it cleaned up the backend invoke API quite a bit too.

Oh, and since this is compatible with existing versions of drush, it could in theory be backported. Some additional testing, esp w.r.t. old calling new, would be in order first. There might just be too much here in the way of changes to backport, so I'm not going to change the status. Mark can if he wants this.

I've had some failures using the new backend, features-update-all being one case, but was too much in a hurry to investigate. But it might be my code that's out of date. I'll try with master and see if it gives any problems.

Whether to backport is the question. Depends on what the timeframe for v5 is.

There is a patch to fix #84 committed to master in features, but it's not in a stable release yet. It was caused by the Windows escaping patch, not this one.

Add "Needs change notification".

Did we ever resolve #81 (better control of label). chx was asking about it.

I hinted at this in http://drupal.org/node/1338816#comment-5236044. The short version is that I could not get interactive mode working with proc_open and pipes; it only worked for me when passing through STDIN, STDOUT and STDERR per the final committed solution. With extra work, there's probably a way to make it work.

Without interactivity (--yes forced), then you can label it as you wish, just by setting the appropriate option in backend invoke.

The problem is that backend gets called via drush_do_command_redispatch('test-run', array($class));. drush_do_command_redispatch() has no facility for passing in $backend_options. Maybe test-run should not be using redispatch so it can get better label?

Made a start with #1343116: Better label for test-run. Maybe someone could fix that issue up and get it committed.

change record posted