Add a hook_drush_output_formats

This patch will work well to use the formats. My main concern would be that any other drush commands that used --format would not indicate any other formats. Not sure if we can consider making the --format help text dynamic?

so instead of the above implementation based on the function name we could have a new info hook, something like hook_drush_formats() that could be implemented by modules and drush core to declare formats/callbacks etc...

So we could have something like:

function MODULE_drush_formats() {
  $formats['properties'] = array(
    'label' => 'Properties',
    'callback' => 'drush_entity_format_properties',
  );
  return $formats;
}

All of the various format info could then be used to populate --format help info text etc...

Sorry for the confusing function names. drush_invoke is for invoking Drush commands. You're looking for drush_command_invoke_all or drush_command_invoke_all_ref, which invoke command hooks.

Kind of odd that the function that invokes commands does not say "command", and the function that invokes hooks does.

I was just about to post pretty much the same answer! :)

@clemens.tolboom I have nearly finished a patch that I have been working on this afternoon that already uses drush_command_invoke_all. Hopefully will post tomorrow.

@greg.1.anderson In order for core output formats to be invoked from output_HOOK I have created an output.drush.inc file that goes in the commands/core directory. Is there a better way to do this?

@clemens.tolboom: Actually, drush_command_invoke_all is about hooks; however, in Drush, all hook functions must belong to some commandfile. This is analogous to Drupal, where every hook must belong to some module. The Drush commandfile that contains the hooks does not necessarily need to contain any commands; it just needs to be named *.drush.inc. See examples/sync_enable.drush.inc for an example.

@damiankloip: commands/core/output.drush.inc sounds like an okay place to put the default output hooks. I would guess, though, that you'd want to make the output hooks just thin functions that called through to a utility output routine that did the actual formatting. I would put the later in includes/output.inc. If the stuff that remains is really small, you might just put it into commands/core/core.drush.inc. If there are a bunch of hooks, though, having a separate file is probably a good idea.

Ok, so here is my first initial patch, based on the above patches from @RobLoach and @clements.tolboom.

As @greg.1.anderson described in #11, the core implementation of hook_drush_output_formats lives in core.drush.inc, and the function callbacks live in output.inc file.

In output.inc there is currently drush_print_r and drush_format_print_r, which is used by drush_format. I think we could just use one but I didn't want to just get rid of drush_print_r current implementation as it is slightly different to that used by drush_format.

I have kept the hook implementation using an array containing a function callback to use, so this can be specified, rather than conforming to a particular function naming convention. Although this can easily change to something like Clemens described in #4.

I have also added details to drush.api.php etc...

Looks pretty good to me. Interested in how the other maintainers are feeling about this.

Looks good to me too. Are going to convert existing functions to use this in another patch? We have variable commands, cache commands, user-information, ...

This function is passed $input (string to be formatted). Callback is passed an array, not a string. At least thats what drush_format_print_r() documents.

Use all capitals for JSON, not Json. It is an acronym.

@moshe - I was going to create another patch with any changes to existing drush code in another patch yes. Provided this one got the nod of approval for the implementation of it :)

I will look at the documentation and naming etc... of drush_format_ functions and JSON naming.

Should we have 'drush_print_r' and 'drush_format_print_r'? It is currently like this in the patch, mainly to preserve other calls to drush_print_r.

You should change existing calls to drush_format_print_r() and then remove drush_print_r

ok, no problem, Will get onto this a bit later.

#17: Really? I would think that if we were moving to a single function, drush_print_r would be preferable. The current signature of drush_format_print_r is call-time compatible with drush_print_r, so I don't see the value of keeping the "_format_" when we already have "drush_".

@clemens.tolboom I think you might be right, it goes with the whole _output_formats thing...

Careless, I forgot about $handle in drush_print_r. I'm indifferent about the resolution of this.

So am I right in thinking we get rid of drush_print_r in favour of drush_output_print_r, then any calls to drush_print_r in drush code get changed and wrapped in drush_print too? Just to make sure!

Change existing calls to drush_print_r to drush_output_print_r; don't expand-in-place the old version of drush_print_r everywhere.

Do we really want to remove drush_print_r()? How about we just mark it as deprecated so we dont break a ton of custom/contrib commands?

I don't feel strongly, but a major release like Drush5 is a perfect time to break.

Sure, just saying there's very little reason to. Let's just make it a wrapper around drush_format, mark it as deprecated and remove it in 6.

Here is an updated patch.

I have changed the few core instances to use drush_format instead (they have been wrapped in drush_print too, so they preserve same output as drush_print_r) E.g. in drush_sql_conf function. So now you could also use the --format option with commands like this too (with --strict=0 option atm). We could add this to the options in hook_drush_command too?

OR would you like to keep the core uses of drush_print_r as they were and use drush_print_r wrapper function I have added back to output.inc? I think it would be ok to change core implementations but leave drush_print_r wrapper in for other drush extensions (as said in #28).

I was thinking the drush_format_get_output_formats function could be used in hook_drush_command info to display the types; So in drush_entity I was thinking of using something like this:

$formats = implode(', ', array_keys(drush_format_get_output_formats()));
$output_format = "Define output format. Known formats are: $formats";

// Then .....

'options' => array(
      'format' => $output_format,
      ),

You get the idea!

I'm not sure if drush_help should be changed for drush 5? That's why I was thinking that when the format option is used in hook_drush_command users can call the drush_format_get_output_formats to populate the list.

Agree that it would be good to have a way of declaring which commands can use which types. Although not sure how this would work atm.

Would love to see properties as a --format! :) Although, as this is very handy for drush_entity we could put it in there easy enough (I did this yesterday) with the new hook for formats.

Here is latest patch based on comments from @moshe in #drush...

Main change to note: changed print_r function to handle regular string with labels (as used by vget).

+++ b/commands/core/core.drush.inc
@@ -1150,3 +1150,23 @@ function drush_core_execute() {
+function core_drush_output_formats() {
+  $formats['export'] = array(

Initialize $formats to avoid a warning.

+++ b/docs/drush.api.php
@@ -318,3 +318,23 @@ function hook_drush_engine_ENGINE_TYPE() {
+function hook_drush_output_formats() {
+  $formats['print_r'] = array(

Btw it is pretty related to #1364808: Proposal for an output formats engine.

Same thing.

core_drush_output_formats has been removed from core.drush.inc, in favour of drush_output_formats in output.inc which invokes hook_drush_output_formats from there. static $error code and print_r fallback also removed.

And the patch would be good.....

Re rolled with comment from @jonhattan in #35 fixed.

Still seeing test failures. For example, try cd tests && phpunit --filter=testVariable

@moshe, I am just looking at this now. I'm not sure why this is failing/getting the null value. When running all of the commands manually they work fine. What am I missing here?

This test fails on the current dev branch without this patch applied. So maybe it is a problem with the test?

@clemens, Not sure what you mean exactly. You think we need to have another output format for piped?

Here is a slightly updated patch, to make sure export returns a value (Don't think it did before). Not sure if the tests will pass yet? I don't trust my machine atm as it's failing for that test on the current master branch without this patch!

I am still seeing tests fail. To isolate, try this with and without the patch - phpunit --filter=testVariable .. It is possible that we have a problem only with code that runs through drush_print_pipe(), as this code does. We are doing unwanted quoting of the string to be printed.

The specific failure in the test is a syntax error in eval'd code, caused to over-escaping:

string(58) "'$variables[\'date_default_timezone\'] = \'US/Mountain\';'"

@jhedstrom @moshe, I knew it was something going on with the eval of the different strings etc... :)

Here is an updated patch that now passes testing.

I only got one failure back which was in makeMakefileCase::testMakeNoPatchTxt - "Unexpected exit code: /usr/bin/drush --nocolor make /usr/share/drush/tests/makefiles/patches.make --no-core --no-patch-txt --test --md5=print" - It doesn't look like it's anything to do with this patch though (correct me if I'm wrong).

Hopefully this patch is ready to go now?

Here is re rolled patch to remove --pipe format in drush_format and some doxygen comment amends.

@moshe, Can you give some thoughts/pointers on these points?

Are we just removing the use of --pipe when it is used as an alias of the export format? or all instances of where --pipe is used? Or were you thinking we should make pipe an output format that uses the current drush_print_pipe function? Or does drush_print_pipe need to use drush format instead? I'm not sure of the various implications at the moment.

Depending on this I will need to update tests, other pipe uses, hook_drush_command implementations etc.. accordingly. The tests that use --pipe as an export output alias will pass now when the tests are changed. It depends what else you would like me too change too.

Latest patch is failing similar to the failures we were seeing before #46. I think you can change all commands that use --pipe to --format. We may need the commands a bit so they can return reasonable json, etc. An example is drush_field_info() which would start emitting json and such instead of csv. The csv format would go away for now.

Yep, that's what I was saying about that yesterday, if we remove the --pipe option from drush_format we get failures as the test is still using the --pipe option. I have tested this on the local with --format and the tests pass, so the new patch will include all of this too.

I will change everything to use drush_format. What about the newline output of drush_print_pipe? Will this be missed?

Do you think it's an idea to start with an initial commit of the patch in #46 that deals with the output formats hook. Then all of the replacement of drush_print_pipe should be in another patch as I think there will be much more debate around this.

I still have a sandbox at http://drupal.org/sandbox/damiankloip/1485656 where I am continuing this work around universal drush_format usage.

Flip back to needs work etc... if you don't agree!

Superseded by #1364808: Proposal for an output formats engine and #1866344: Make --pipe awesome with output format engines.

Updated issue summary.