Drush features-add and features-export are confusingly similar, and overly verbose

Yes, indeed. It should just be features-export, which does the right thing.

But it's always seemed odd that the export/add command works as a component listing command (and a bad one at that). Wouldn't it be better with a component listing command, with options to list all, non-exported or exported (with the name of the defining feature)? There's other uses for listing components, like figuring out which feature defines a given thing (which we use grep for now).

All in all, it is a bit odd that features list things when given insufficient args.

Secondly, I did promise a proper pattern matcher way back. Having to specify each field of a content type is tedious, and being able to shorthand 'variables' to 'var' is a real crowdpleaser.

So, a smart move would be a general internal listing function (which the listing command calls) and a matcher function which filters the result of the listing function. Then the export function should just call the listing function and hand over its arguments to the matcher, and get the list back. Which sounds simple, but gets really iffy with ambiguous patterns where you want to ask the user. My last attempt got really ugly fast, though the user experience was great.

Not really, it was almost 1.5 years ago...

The actual adding code is about a fifth of the function, it just loads the feature and adds in the new components. The rest is finding the components, which I want to redo.

Regarding the switches, there should be two one char aliases. The question is: which two chars?

@tim.plunkett

Because I wanted something a bit more helpful than features-export, and it should've been added to export too. But I'm rewriting that stuff right now, so don't waste too much effort on that.

There is none, I'm working on the pattern matcher, which should be used by both the lister and exporter. It's kind of a blocker for this one, if you wanna be strict about it, but if you want to get something done, you can either use the find stuff from features-add, or just let it be like the old export. Both ultimately work from an nested array of source and components, which is also what my pattern matcher produces in the end. Giving it a second thought, it should probably just use the code from the old export, now there's a list command.

The call_user_func_array thing is an attempt at making it fall back to exporting if the feature doesn't exist, but I'll be rather redundant if it's rolled into features-export.

Forgot to add:
While it might seem a second issue, I really need a listing command to implement it into in the first place. So that's why I'm working from here.

I'll take the pain of merging it when I get that far.

Well, we could split the issue, but I'm too lazy..

Anyway, pattern matching implemented in the attached patch, try it out. Here's how it works:

dependencies:image does the obvious and picks dependencies:image.

Sources and components can be shortened, but only if there is exactly one match:
So dep:image works, but d:image doesn't (more than one source contains a 'd', 'field' for instance).

* and % can be used as a wildcard (there's no difference, but some might prefer to avoid having the shell interpret * and don't want to escape it). Wildcards, as opposed to shortened names, needs to match the whole string, so while ep:image also works, ep%:image doesn't, it needs to be %ep%:image.

There's no limitation on combinations of shortening and/or wildcarding of source and component, so %:%field% will match all components from any source, that's contains field in its name. The only exception is that while dep:%something% will error out if no compontents match, %:%something% will ignore sources without matches.

Experience suggests that one leans towards using shortening for sources and patterns for components, but they works both ways.

Lastly, a pattern without a colon will be interpreted as
:%, that is, all components from the matching sources, which makes the list command handy, as just naming a source will list all its components (may add an option for this behavior, may not be as handy for dumping).

features-add have been changed to use the command, but it haven't been properly tested, but you should get the idea. It's also lacking a bit in the documentation department yet, but is pretty well commented, if I may say so myself.

Anyone? Anyone? Büller?

I've done some basic testing of this patch, but need some more eyes on this.

Regarding the original topic title, I would actually like to retain both the features-export (fe) and features-add (fa) commands via drush since some people might be used to typing one or the other. I know that I tend to automatically use 'drush fa' and wouldn't want this to go away. Consolidating it in the code is fine, but let's not annoy people and get rid of anything they are used to using.

The patch seems to work for my basic tests. The wild card matching in "drush fc" works well and is a nice addition.

Before marking this RTBC I would like to see some additional drush help added to indicate that the wildcarding is supported and how it works. But I think we are really close on this one! Let's crank it out so we can remove this blocker to the 1.0 release!

I think we could refactor export a bit more:

Export should use the same pattern matching function.

Is there really any reason for listing existing features when insufficient arguments are supplied? Shouldn't we just error out?

Is it really useful that export will create a feature with the same name as the exported component when only one argument is supplied?

Lastly, we should refactor the 2 functions into one, and add the name and alias of the removed one as aliases to the remaining. Or drop fa and just go with fe (I favor this, personally, 'features-export' describes both actions).

@mpotter
Have you tested fe and fa with the patch? I've got a seriously broken fe here, but I haven't used it in ages.

@Xen: was mostly using fa, but I thought fe also worked. I'll try to look at it but am pretty swamped this week.

@mpotter
That's odd. Mine seems completely broken. It pretends it does something, but the relevant components doesn't end up in the module.

I'll fiddle with it.

Well, there seems to be some issue in _drush_features_export, trying to dump dependencies:search to a new feature, doesn't add it, but adding it to an existing does. It's a separate issue, however.

New patch. Export and add merged, under the features_export name. Sorry to the people that's gotten used to fa, but it'll make more sense in the long run.

Added short options to fc -e and -o for --exported and --not-exported. If someone can come up with a more mnemonic short option, I'm listening.

Also fixed up help and added help on pattern syntax.

Now, everybody test, dammit. :-)

(and we'll attach the patch too...)

New patch. Export and add merged, under the features_export name. Sorry to the people that's gotten used to fa, but it'll make more sense in the long run.

I'd like to see either 'fa' still work, or else at least an output message telling people to start using 'fe'. Otherwise this just causes us all a new support issue that will cause more tickets to be filed from people who think 'fa' is broken.

+1 to #21's comment. In the past deprecated drush commands warned the user that they were deprecated, but otherwise provided the same functionality while listing what they were deprecated in favor of. From a UX perspective (and to minimize erroneous "fa is now borked" bug reports), this definitely seems like the way to go.

Fair enough, makes sense to let the old command point people at the new.

Anyone got an example of how it's usually done?

Looks like you want deprecated-aliases in Drush 4, shell-aliases in Drush 5. If I was spread any less thin I'd whip up a patch, but http://drupal.org/node/1384070 seems as though it should point an enterprising developer in the right direction.

Well, that issue actually removes support for deprecated aliases, Seems that the new way is to just remove obsoleted commands and let the user figure it out.

Which means there's nothing more to do here, as we'd not follow drush cores lead by implementing a deprecated function...

Whether it's the right way to behave, is something to be taken up in the drush issue queue.

While I am all on board for a consolidated and enhanced features-components function, I'm not sure I grok fully the features-managing functions in drush.

Given this patch, this is the current lineup of feature-modifying drush commands, right?
features-update: without changing the components of a feature, re-export the feature to code.
features-export: Create a new feature with the specified components or add components to an existing feature.

To me these functions seem very unfortunately named; they seem to do the opposite thing that they are called.

This may not be well baked, what what about the below:
features-create (fc?) (creates an empty feature, optionally take description, package, etc)
features-component-list (fcl)
features-component-add (fca)
features-component-rm (fcrm)
features-component-mv (fcmv)

fair enough.

Only thing i noticed so far in #20 was there is a case statement that got turned into Case. It was kinda hard to read though because of the amount of changes in a single area.

Noticing what seem to be a lot of strange bugs. I removed the Case and some lines about prompting to overwrite a folder that were asking twice.

Is this intended behavior?

Joel-Collinss-MacBook:features joel$ drush features-export asdf1 "user_permission:use page manager"Created module: asdf1 in sites/all/modules/asdf1                                                                            [ok]
Joel-Collinss-MacBook:features joel$ drush features-export asdf2 "user_permission:use page manager"
Created module: asdf2 in sites/all/modules/asdf2                                                                            [ok]
Joel-Collinss-MacBook:features joel$ drush features-export asdf3 "user_permission:use page manager"
No user_permission components match use page manager                                                                        [error]
No components to add.                                                                                                       [error]

Took me a while but this fc command is pretty cool!

I took the liberty of adding a bunch of options to features-export, hoping to get the feature created with the correct meta properties like package and description. Maybe i've been looking at this for too long though. I wont have hurt feelings if someone tells me these additional options aren't helpful.

New patch (continuation of #20), makes the error messages a bit more truthful by mentioning what we were looking for. So the above mentioned error becomes 'No unexported user_permission components match "use page manager"', which should make things a bit more clear. Also fixed the Case and added some quoting to the error messages.

@joelcollinsdc
Re #29, the prompting was added because _drush_features_export only prompts for overwriting existing directories when there's actually a directory. If no directory exists, it creates it without asking per default, and we don't want that.

You shouldn't be able to export the same thing twice. Tried doing the same, but with a drush cc all between each command?

Re #30, I like the idea of being able to specify properties on the command line. Actually, I think it should prompt for these values if no command line args are specified, with another switch to force current behavior. The current behavior doesn't encourage devs to properly name the feature.

From the help for fe:

"If called with a single argument, attempt to create a feature including the given component with the same name."
Does this work?

$ drush fc "user_permission:administer panels styles"
Available sources
user_permission:administer panels styles

$ drush fe "user_permission:administer panels styles"
No components supplied. [error]

For the record, i'd recommend just removing that text from the help, as this seems like a bad practice.

Did you have a chance to look at the --allow-conflicts option I added in a patch?

@joelcollinsdc
Help got a bit out of date. Patch updated and rerolled aganst 7.x-1.x.

I can't really see the need for the --allow-conflicts option? Does it allow for exporting, say a view, that's not in the database, but defined by some other features module? Because then it would be handy for the thorny 'splitting a features module' issue.

Maybe you can tell me a better workflow. I'm trying to refactor a install profile that is feature-heavy. This means a lot of "moving" things between features. I'm not sure what the best order of operations is... delete items out of the source feature and then add them to the destination feature? What if the item is a full exportable, do i have to force save it to the database first?

I was hoping I could create my new features using the features-export command and then go back and clean up the older features. Seemed simpler that way.

Well, you've hit a thorny issue there... Deleting exports from a feature is messy, and, as far as I understand it, not really supported.

As it is, it's either doing it by hand, by copying code around and fixing up the info files. And that's painful. An alternative is using the ftools module to unlink the exports and then reexport them. I'm told it works as advertised.

I donno if the --allow-overrides thing would really help in that situation, but you can try to patch it in and test it out. But it really belongs in a new issue.

Another thing we should add: listing the title of the component, at least in the fc command. node_view_panel_context_2 isn't very helpful, and Features UI uses nice human readable names.

Trying to get this in a form that can be committed. When I installed #33, I got warnings in Drush when using "drush fc --help" because of the "short-form" arguments. I have removed those for now. I also re-added the "drush fa" command with help text about it being deprecated. I know this might go against "tradition", but it will reduce our support load dealing with people who use "drush fa" and wonder what happened to the command. We can remove it later after people have moved to using "fe".

Otherwise, functionally it seems to work.

Excellent! I like this better. Marking it as Reviewed. Will commit it soon.

@mpotter:
Which drush version? I don't get any warnings, and I'm rather attached to short form options.

@tim.plunkett
Please don't. The idea of cc printing out a numbered list on missing arguments was pretty stupid, and it shouldn't be duplicated. Drush is a command line tool, not an interactive program. When no arguments are given, drush should print an error message and exit (as "drush user-create" does) or print a listing of options of some sort and exit (like "drush" does). fe currently mimics the latter and lists all components in existence. It works like any other standard Unix command line tool.

Committed to a84b263

No, this is *not* better.

I'm going to let you guys hash this out. We had a lot of people who really wanted this functionality and this issue had stalled for over a month. Tim was the original requester and needed the user prompt functionality, and the people I have surveyed liked it too.

Personally I hated the huge dump of stuff that "drush fc" showed before. So you are going to need to make a good case for this. I thought it was a good compromise to add your pattern matching along with Tim's interactivity.

This has been committed and it's still MUCH better than having nothing at all. So please open another ticket to contribute new patches to further refine this issue.

NOTE: "drush fc %" displays a full list just like "drush fc" did in the previous patch. So the functionality is still there.

@mpotter:
You also want me to open another issue on the short options?

@tim.plunkett
#1487972: Don't ask for compontent sources in fc

@Xen: Yes, please open an issue for the short options issue. My "drush status" tells me v4.5. If there is a compatibility issue then a patch will need to address that somehow.