Change white spaces in commands by another symbol

This would also make the command parsing logic much, much simpler.

And hyphens is certainly the better alternative.

i'm happy with this approach. any objections or feedback? i'd like to decide this soon.

note that command aliases have arrived since this patch was made which perhaps obviates much of the need for completion.

how would this simplify command parsing? [Edit: duh, I get it now. the first word is always the full command.]

I think this is a very good idea, but it does break a lot of scripts. Since drush-HEAD is a major version upgrade, so I think this is okay. However, today if there is a problem with drush-HEAD, a drush user can currently roll back to the 2.x branch without much difficulty. If the command names become very different between drush-HEAD and the last snapshot branch, users would have to roll back to some CVS date checkout if they found head broken, which would be frustrating.

Therefore, I recommend that a "drush-3-dev" branch be created, and this patch should be first applied in the first revision that is drush-3 dev. That way, if drush-head breaks, users can roll back to drush-3 dev.

I leave it up to you on the timing of creating drush-3 dev, but drush-HEAD has been working pretty well lately.

Regarding the simplification of command parsing, I could roll a patch for that once this lands. This would also help #658432: drush_backend_invoke() assumes single word command names (there'd be nothing left to do...)

Seems like we have agreement here. Lets discuss broader changes.

Should we namespace all core drush commands? That means, core-status instead of status, pm-enable instead of enable, and so on. I would have rallied against this before we had command aliases but I think it is more plausible now. If we do this, should we require same of contributed commands as well? One benefit that you can easily grep for the commands you want in the command list. I don't think we get the full isolation benefits usually associated with namespacing since we would still support on command aliases.

That's a great idea; it would also allow us to modernize the output of 'help' so that it first printed just the top-level modules.

We could actually fix this in code, at first. Rather than change all the command names, during the get command list process, check to see if the command has a space in it, or does not start with the module name. If so, we can convert spaces to '-' and prepend the module name, and make the original command name an alias iff $options['allow-spaces-in-commands'] is set (initially commented-out in example.drushrc.php). This gives people a deprecated way to maintain compatibility with their old scripts. We can yank the code and change the command names closer to the release of the first drush-3-stable.

I still think it's a good idea to do this in conjunction with the first drush-3-dev tag, to call attention to this major change.

Here is a patch that could be committed without making a drush-3-dev tag. It still allows commands with spaces in their names, but it prints out a nag message warning the user to use an accepted form of the command. The advantage of doing it this way is that this patch also changes spaces to dashes for contributed commands. Later, when drush-3 tags show up, we could go ahead and reject deprecated command names unless an appropriate $option variable is set allowing it.

This patch also deprecates the "sync" command per #672648: Remove 'sync' alias.

Nice work.

Help seems broken for a single command. The code path for help command has always been a bit dodgy.

The patch exempts core core commands from the namespace requirement. I could go either way on that.

The nag message does not list command aliases as a recommended form. Seems like it should.

I'll have to use this for a while to see how it feels. Command listing is a bit heavier, tho we can now make that experience cleaner with namespaces.

Drupal usually doesn't offer any backward compatibility. I'm mulling it over. Seems nice, and costs us little. I don't think it gives backward compat for commands that hook in via drush_invoke() API. Thats fine, just thinking aloud.

Yeah, this is broken in a couple of ways. I meant to comment on excluding core; I mostly did that because if I included core, it broke drush help, and I figured leaving off core was a good enough workaround for that for the time being. I didn't notice that I broke drush help [command]. It does work for core commands--something about adding the namespace breaks help. I'll have to look at that later.

The nag message tried to include aliases in the recommended form, but I accidentally unioned the list instead of array_merge-ing it. Oops.

The backwards compatibility seems like a good idea through the transition time. Ultimately it should be removed, though, so we can clean up the backend invoke code path. If it's too messy to clean up the help and drush_invoke errors, maybe it's just as well to take a deep breath and dive in... but that wouldn't get contributed commands, which would also have to be updated.

I'll see how hard it is to fix up, anyway.

(Edit: Remove late-night hallucination.)

When I woke up this morning, I remembered reading an article by Dres about the Drupal philosophy of non-backwards compatibility, to reduce development support time and to keep the code cleaner. In this instance, we'll get the later eventually, because we'll yank this code after people have time to adjust, but there's no question that there is a development code that would take time away from other things.

I had an idea that I thought would be really easy to do, so I didn't test it as much as I should have because I tricked myself into believing that it was easy.

One of the things about this that is hard is that a lot of commands already have the namespace on front, and I conditionally added the namespace to the front only if it was needed. This complicates drush_invoke, because the command hook is composed from the command name. Making this work with conditionally-added components would be too complicated.

I'm going to look at this a bit more to see what can be done easily, if anything.

Backwards compatability for help dispatching is hard; maybe we should just do this the way the OP did.

Oh, and rolling back to #4, my first thought was that namespacing commands was a good idea, but after trying it, I tend to think that it's too verbose, and most commands would just end up aliasing-away the namespace part anyway, so my vote is to leave it as it is.

When it comes time to fix up help parsing, I'd say that we should define a drush "module" (what do we call this?) as any collection of drush commands defined in a single drush.inc file. We give these names like "core" and "sql", and have "drush help" just print the module names with a summary, and "drush help core" would print all of the commands in core, and "drush help cron" would print the command-specific help for one command in core.

I think that's probably best and simplest for now. So, in summary, cancel #5 - #10 and go back to what I said in #3, plus my comments on namespaces above.

Here is the patch from #0, updated so that it can be applied to drush HEAD.

Here is the patch from #12 with deprecated commands added back in. While it was very complicated to try to fix up $command['command'] on the fly, adding a deprecated alias by replacing the dashes in $command['command'] with spaces worked nicely with very little code.

"sync" has been added to the deprecated list, as in #6.

drush help cache clear and drush help cache-clearboth work without fuss.

Using "drush cache clear" gives the warning message and suggests "cache-clear" and "cc".

If a contrib command contains a space, then a notice is sent to drush_log, and the command is not added to the command list. So, while it would still be nice if jonhattan would evangelize the new syntax as promised, if he misses anyone, they will very quickly notice that their commands have been dropped from "drush help", and they should be able to figure out how to adjust right quick.

I'd recommend committing this after review, and then disable the deprecated command handling once a "drush-3-dev" tag is created.

looks good to me. lets wait a couple days for more input.

Lets simplify drush_parse_command() as well. while() might be unneeded now that commands can't have spaces.

If we simplify drush_parse_command, then backwards compatibility will break. I'd recommend that we test this, commit it, make a drush-3-dev tag for people to fall back on, and then take out the backward compatability and simplify drush_parse_command.

If you want to skip the backwards compatibility phase, bounce this back to me and I'd be happy to simplify drush_parse_command.

So, backwards compat only works for core commands, right? I have devel module installed and that ships with 3 commands that have spaces. Here is what I get when I try to run one of them. Instead of an error, I was hoping to see: NOTICE: "generate content" must be renamed to generate-content in order to work with this version of drush.

When adding to drush_log(), we don't really need the all caps NOTICE prefix in log message.

Backwards compatibility is not just for core commands; it works for any command that has already been renamed to include dashes instead of spaces. For example, "generate content" is unusable until you rename it to "generate-content", but once you rename it, you can call it either as "generate-content" or "generate content". (The later form will go away after a while, when we simplify drush_parse_command.)

The way I was handling the unusable (space-containing) commands was to just dump them on the floor. Your idea is much better. Now, commands with spaces will still show up in "drush help", and you can even read the help message for them. If you try to use them, though, you will get a notice per your idea in #17.

Committed. Nice work, all ... Lets get the word out to command maintainers.

I think we should namespace the pm commands to pm-enable, pm-download, etc.

There is another reason to make a drush-3-dev tag when this is committed. In addition to the reasons given in #3, all drush contrib modules will have to sync to drush-3. While the new dash-containing commands will still work with older versions of drush, contrib maintainers might still want to label their old versions as "drush 2" and "drush 3" specific.

I have another backwards-compatibility idea that I think would work out pretty well with only a few lines of code (shouldn't rathole like #6 did). If $options["allow-commands-with-spaces"] is set to TRUE, then we could allow the old space-containing commands to run. If it's not set, we could have the command dispatch code go through the new cleaned-up version, and disable the generation of deprecated commands with spaces.

Let me know if you'd like to see that, and I'll roll a patch if you do.

Oops. Cross-posted.

I'm sort of interested in that enhancement to backwards compat. I'd have to see the patch to know for sure how ratty it feels. Could get confusing with the drush_invoke API.

I'll roll the patch. drush_invoke won't be a problem; we just need to convert both spaces and dashes to underscores and it will work fine. I realize, though, that we can't clean up the command dispatch code yet, as doing that would remove the nice warning message. Anyway, I'll put it up shortly.

Here it is. It's pretty clean, and addresses both commands that are named with spaces, and commands that are called with spaces after being renamed to the new dashed form. Both of these are prohibited until $options['allow-spaces-in-commands' = 1; is set, at which point both are permitted. The error messages direct the user to example.drushrc.php, where an explanation of the flag can be found.

Committed. Thanks.

Just to reierate, we should change to pm-download, pm-enable, etc.

This is kind of ridiculously broken.

this is the line that most concerns me :

  $hook = strtr($command, "- ", "__");

this is the mechanism by which drush invoke creates the possible hooks it will call for each command.

firstly the double underscore would mean hooks such as :

function drush_pm__enable_validate() {}

with the double __ existing for no actual reason.

secondly, there is a space after the "-" , so it will try and change the function definitions to :

function drush_pm-enable_validate() {} 

so all function exists() checks will obviously fail.

Adrian,

I apologize that the code is not very clear; this really could be a lot better. However, you have confused strtr with strstr. The function strtr as used above will replace every '-' in a string with '_', and every ' ' in the same string with a '_'.

http://php.net/manual/en/function.strtr.php

Since strstr is a very common php function, and strtr is very unusual, I acknowledge that the end result is rather horrific. I will add a comment, at least.

(Edit: Actually, it's str_replace that does replacement; strstr is just a string-finding routine. Still rather horrific.)

It would be useful if #679236: show all possible drush_invoke() variations had already landed; would make diagnosing problems like this easier.

wow. esoteric function. it should be documented, but it seems to be working.

sorry for freaking out. was mostly just shock at all the changes.

isn't the str_replace(array(), array(), $command) a bit clearer ?

Yea, I've used the array version of str_replace before, and I think that's probably clearer. strtr is probably faster, but my guess is that would be imperceptible in this context. As a unix hack, it was easy and clear for me to pull the "tr" tool out of my brain and map that to the php "strtr" function, but this process is what is sometimes called write only code. ;) I have no objection to using str_replace, but since this code will be simplified once the backwards compatibility bit is taken out, so I'd just soon leave it as it is for now.