Drush doesn't allow spaces in commands anymore.

This patch swaps the spaces in "node_export import" and "node_export type" with hyphens. I tested the import and it works. I've got no use for type so I didn't test but it should work just fine.

Comments

jonhattan’s picture

Priority: Normal » Critical
Status: Active » Reviewed & tested by the community

Using it by mysqlf.

Marking as critical because drush 3.0 is out.

danielb’s picture

Is there any way to make "node_export" the command for all the operations, and then supply 'export', 'import', 'types' as flags/args/params that indicate which function to pass off the operation to?
Sorry I don't know much about drush, but it seems a command like "node_export-import" seems hard to remember and looks strange.

Also I notice node_export_drush_help() doesn't seem to cover all the types of usage??

danielb’s picture

I am seeking advice about this on the drush queue #785860: Format of commands - best practice?

danielb’s picture

ok we're going with

node-export
node-export-import
node-export-type

no underscores

danielb’s picture

I'm just rewriting some of the output strings as the wording is unclear.

also I think the syntax of some of the example stuff is wrong and inconsistent and other strange stuff like a variable $file in the last function that is set but never used

i'll try to finish this up tonight

danielb’s picture

Status: Reviewed & tested by the community » Needs work
jonhattan’s picture

Title: Patch for drush no spaces compatibility » Drush 3 compatibility

I agree with 'needs work'. Upon a look to the drush code I see some things to improve.

I can work on that this weekend if you want, as I have some experience with drush and am using node_export for an ongoing project. Some ideas:

command aliases:
* node-export-export -> ne-export
* node-export-import -> ne-import

command arguments and options:
* ne-export 1 2 3 4
* ne-export --type=story --status=1 (ideally language,type,status as in admin/content/node)
* for both commands --file option. If no --file is present, read from stdin.
* it could be also of interest to allow some options from admin/settings/node_export namely "Reset values on import".

function names: Using explicit callbacks is not needed.

danielb’s picture

OK In the meantime I've done some work on it already. I have committed it to CVS and it should appear in the next Development release.

- I have changed the wording of some of the text output as it was a bit brief and read a bit like Engrish, and some of the ideas it was conveying like "writing to a filename" and "author set to a uid" were technically wrong.
- I've removed the output that was in node_export_drush_help() and replaced it with calls to node_export_drush_command() where the same information was already entered. This is for consistency and making it easier to maintain, despite the reduced efficiency of generating the help text. I have not tested this.
- I have added the "--file" option to the import command, for consistency, but also don't know if I have implemented it correctly.

I also agree with you jonhattan about those additional improvements.

We could ditch node-export-type and just use the --type/--types option, that way we can add other means of filtering from all nodes if no nids are supplied? Or --nids could even be a filter itself?

danielb’s picture

Oh man I had a crazy idea, stop me if I'm drunk, which I am, but it's using views to select nodes

--views:field_something_something=value

I just blew your mind

danielb’s picture

ok well that idea might need to percolate a little, doesn't quite make sense, but somehow we could leverage views as an additional way to restrict nodes. I run a module called finder where I've done some stuff like that http://drupal.org/project/finder

Anyway to be productive, here are the fields in the node table we should consider being able to filter nodes on.
nid
vid
type
language
title
uid
status
created
changed
comment
promote
moderate
sticky
tnid
translate

we should allow multiple comma seperated values, and perhaps even operators other than 'equals' ('contains', 'starts with', 'ends with', 'less than', 'less than or equals', 'greater than', 'greater than or equals', 'not equals')

perhaps a flag to say whether we want to "or" or "and" the filters together, "and" would be default

the more complicated stuff like cck values, other tables in the node module (node_revisions, node_access), and other modules integrated with views we can figure out another time

danielb’s picture

Hey jonhattan, did you do anything on this (re: post #7)? If not I might get a start on it soon.

I will cut back to 'status', 'type', and 'language' like you pointed out the admin/content/node page uses. I think anything else would require a more advanced approach.

danielb’s picture

I've done some work to add aliases

danielb’s picture

In admin/content/node "Status" encompasses 4 different things that can be on/off
status, promote, sticky, translate

language can be a variety of values
type can be a variety of values

then there's 'category' which you could use taxonomy_select_nodes() with

One step at a time I guess.

jonhattan’s picture

Sorry for my absence. I've been and still am very busy. My idea was to mimic some options of the command watchdog-show, that I coded some months ago. See drush help watchdog-show. The code is at commands/core/watchdog.drush.inc.

I'll be back to this as soon as I release some work.

danielb’s picture

StatusFileSize
new6.12 KB

Hmm, I'd already started on it. Haven't had a chance to test it since I don't actually use drush (nor do I have access to a command line on my shared host), but I do think it's a good idea I should set up a local server to test this on, maybe tonight.

Here is my patch against the current CVS.

The idea is that it supports these option filters: status, promote, sticky, translate, language, and type
- If using multiple option filters they are combined by 'and', and you can also provide comma separated values (like --type=page,story) and it will return nodes that match either value.
- The nid arguments are also treated as a 'filter', and would be intersected with the nids returned from option filtering.
- If not supplying arguments or option filters at all, it selects all nodes.

If this works out I will do some work on support for a taxonomy filter (tids, vids, and terms), as well as choosing nids via your own custom PHP code and SQL queries. That would pretty much cover anything anybody would want to do.

danielb’s picture

Priority: Critical » Normal
Status: Needs work » Needs review

I haven't tested/released this latest stuff yet, I set up a xampp server but the sql stuff kept crashing the machine :/ annoying. I have a lot of serious crap going on at the moment IRL which has kept me a bit more occupied that I'd hoped.

Also the drush 3 support is there, so i'm taking off the critical priority, it's just that this issue isn't complete.

itangalo’s picture

Status: Needs review » Needs work
StatusFileSize
new20.34 KB
new2.99 KB

-1-

The patch seems malformed. I get the following message when applying it against version 2.21:

Hunk #1 FAILED at 20.
Hunk #2 FAILED at 52.
Hunk #3 succeeded at 66 with fuzz 2 (offset -3 lines).
Hunk #4 succeeded at 98 (offset -60 lines).
Hunk #5 FAILED at 181.
Hunk #6 succeeded at 192 (offset -60 lines).
Hunk #7 succeeded at 220 with fuzz 1 (offset -67 lines).
Hunk #8 succeeded at 242 with fuzz 1 (offset -66 lines).
3 out of 8 hunks FAILED -- saving rejects to file node_export.drush.inc.rej

The rejection file is attached.

The patch applies well against dev version, though. Attached is a patch that changes 2.21 to the dev (as of 2010-Jul-11) with patch applied.

-2-
The patch unfortunately doesn't do the trick with drush integration. I don't know what's going wrong, but drush doesn't seem to like it. It doesn't recognise the node_export stuff, and when listing drush commands (by just typing 'drush') it reports that something is broken and stops just before the node_export stuff.

Sorry for the bad news. Wish I knew how to fix it, or even approaching a solution. :-/

itangalo’s picture

Oh, and I am still learning how to create well-formed patches myself. You will have to use -p1 istead of -p0 to apply the patches above. Sorry about that.

jonhattan’s picture

StatusFileSize
new4.99 KB
new10.54 KB

Attached patch implements my proposal in #7 and also a cleanup to the code. It's so much changed that I also add the complete file for readability.

It implements the same filters as in admin/content/node and uses its own backend, that is, http://api.drupal.org/api/function/node_filters/6 and http://api.drupal.org/api/function/node_build_filter_query/6

Code is still wanting some improvement in help text and of course my engrish :)

A drawback is that it has the same limitation as admin/content/node: no option for --type=page,story or --status=promote-1,sticky-1. It can be solved with no dificulty but I like the beauty of using the drupal api for that and perhaps in practice there's no such need for several status or node types.

lastly, although technically not needed, I've explicitly defined the callback for ne-export command because it seems there's a bug in drush running the command twice.

@itangalo: you can find useful guidelines in the "cvs instructions" tab of the module http://drupal.org/node/258223/cvs-instructions/DRUPAL-6--2

@danielb: extending with SQL support could be enough from my pov.

jonhattan’s picture

Status: Needs work » Needs review
danielb’s picture

Status: Needs review » Fixed

Alright the tail end of this issue has left me confused, I'll commit what I've done and see where to go from there when the complaints roll in.

danielb’s picture

I am creating a new issue to remind me to consider adding support for selecting nodes with SQL/PHP/Views as well as revisiting ideas in jonhattan's patch because I am a bit overwhelmed at the moment.
#956406: Drush improvements

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.