Issue #2866: Make user:login backwards-compatible

[GBirch]

Changes uli behavior so that if a) name option is not passed and b) argument to command is an integer, uli will treat the argument as a uid. Updates function documentation to reflect same.

Adds test for same and updates some comments on other parts of the test case which appeared to be out of date.

Fixes what appears to be a bug at line 35, which was a) not passing an array of arguments to drush_invoke_process, and b) passing the name option as if it were the argument.

Adds some additional text to the testing README.md that I would have found helpful.

[greg-1-anderson]

Could you perhaps make this function a bit more backwards compatible still? Drush 8 allowed specifying a user name or id, and also allowed specifying both a user and a path. To effect this, you could declare $name and $path as optional parameters. What Drush 8 did was assume that if only one of these were provided, it was a path, not a name. This could be done simply by assigning $name to $path and clearing $name when there is only one option. Or, like Drush 8, you could declare array $args, and figure out which variable was $args[0] and which was $args[1].

[GBirch]

As requested. Have updated tests and documentation accordingly. This PR also seems to solve issue #2878 .

[greg-1-anderson]

drush uli /path should be interpreted as a path, not a user, even if there is no user option.

[GBirch]

It is, see lines 94-96 and 110-113.

[weitzman]

Sorry i wasn't explicit earlier. I'd rather not keep the old behavior where arg1 can be any number of things. My only desired nod to backward compat is to give nicer error messages in a validate hook when arg1 isn't obviously a path.

[GBirch]

I don't know if I get a vote, but from a usability standpoint, I really like typing uli # to login as user with uid #. Having to type uli --uid=# is both wordy and will require quite a bit of re-training, as well as likely to break scripts.

[greg-1-anderson]

The Drush 8 behavior is designed to reduce typing:

drush uli username /path
drush uli /path
drush uli username

We could keep things almost as short if arg1 is always a path (if specified at all), and arg2 is always a user (defaults to uid=1). Then:

drush uli /path user
drush uli /path
drush uli / user

Folks would just have to get used to the change in parameter order.

[GBirch]

Would you require paths to be prefixed with the slash?

[weitzman]

I see these proposals as barely better than status quo. Its not that hard to type --name

[GBirch]

Umm, from the perspective of someone who has had to type uli commands many times a day, I would disagree with that. But I don't know what the game plan was here, what the pain points were that caused you guys to completely change the syntax of a much-used command.

[greg-1-anderson]

I would still prefer to keep 100% backwards compatibility here.

[greg-1-anderson]

It is almost possible to use a @hook init to fix up the parameters to whatever form you liked. The only thing preventing that is that hooks cannot add arguments; they can only add options.

If the built-in command was uli [path] [user], then it would be possible to write a hook to reverse these under some heuristic. That would be the opposite parameter order from Drush 8, though, so uil [user] [path] would be closer to backwards compatible.

[GBirch]

Moshe and Greg, I'm happy to work on this some more, once you have a consensus on the direction you want to take. For what it's worth, I would vote for maximum backwards compatibility consistent with a readable and maintainable code base. Which is what I tried to do with the last PR (although I did use assignment within conditions, which I'm feeling really bad about).

[weitzman]

uli [path] [name] sounds good to me. folks can user user-information command to look up name by email/ui if needed.

[GBirch]

That's exactly the syntax that causes ugly breakage if someone doesn't know that you've changed the syntax, or their fingers click uli # by habit. I am still struggling to understand why you would want to change the syntax of a basic command that people use a lot. What's the point?

…

On Thu, Sep 28, 2017 at 7:14 AM, Moshe Weitzman ***@***.***> wrote: uli [path] [name] sounds good to me. folks can user user-information command to look up name by email/ui if needed. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2868 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACh9MUUSpwsJhmCjbeet2EQ9FMhx0hG-ks5sm3-ggaJpZM4OyTQM> .

[greg-1-anderson]

I'm not sure why we wouldn't at least look up the email address / uid as a service in the two-parameter form of the command.

@GBirch The point of not reversing the parameter order is that this introduces potential ambiguity as to what the parameters mean. There is little ambiguity between a uid (integer) or email address (contains "@") and a path, although conflicts are still possible in edge cases. If we allow paths to omit the leading /, though, then paths and usernames become ambiguous.

Again, I'd prefer to maintain compatibility here, but I also see the benefit of not inferring the one-parameter variant. If we look up the uid as a service, then the one-parameter username variant is preserved, and the one-parameter path variant must become drush uli 1 path. That's not terrible, so I'm okay with reducing ambiguity, as long as we can use 1 as a username.

Again, if individual users don't agree with this, it would be easy to write a policy file that moved the user argument to the path and set the user argument to 1 when someone provided just a single path parameter. Perhaps the leading / could be required in this circumstance. That would eliminate ambiguity, and there isn't much additional code to it.

[GBirch]

@greg-1-anderson, Sure, ambiguity between names and paths is a problem if you permit paths without a slash. And, as you note, this can also be a problem in edge cases for uids and emails as well. So let's assume it's desirable to change the syntax in some manner to disambiguate. As a result, some existing usages of uli will a) break and b) require more and different keystrokes. So the question ought to be, What disambiguating syntax change is going to cause the least developer unhappiness? Do you have any data on how uli is used in the real world? My experience with uli, and the experience of those I've worked with, is that I use "uli [uid]" all day long, and rarely if ever use "uli [path]", but perhaps that's idiosyncratic. So my preferences for the least irritating syntax change would be, in order: 1. Require a slash before a path. Otherwise keep syntax as is -- invoke uli with one parameter or two. If two, the first is a user identifier and the second is a path. If one, then if the parameter begins with a slash treat as a path, otherwise as a user identifier. 2. uli [user identifier] --path=path In either case the user identifier could be uid, email or name, which is frankly not hard to code. And I note that my preferences are shaped by the "fail ugly" problem that started all of this. Assuming I lose this argument, are there any examples of policy files that do the kind of argument munging you suggest? All I can find are examples of "if test fails throw error." This would be a policy function that rearranges the parameters and then re-invokes the command and prevents the default somehow?

…

On Fri, Sep 29, 2017 at 12:34 AM, Greg Anderson ***@***.***> wrote: I'm not sure why we wouldn't at least look up the email address / uid as a service in the two-parameter form of the command. @GBirch <https://github.com/gbirch> The point of not reversing the parameter order is that this introduces potential ambiguity as to what the parameters mean. There is little ambiguity between a uid (integer) or email address (contains "@") and a path, although conflicts are still possible in edge cases. If we allow paths to omit the leading /, though, then paths and usernames become ambiguous. Again, I'd prefer to maintain compatibility here, but I also see the benefit of not inferring the one-parameter variant. If we look up the uid as a service, then the one-parameter username variant is preserved, and the one-parameter path variant must become drush uli 1 path. That's not terrible, so I'm okay with reducing ambiguity, as long as we can use 1 as a username. Again, if individual users don't agree with this, it would be easy to write a policy file that moved the user argument to the path and set the user argument to 1 when someone provided just a single path parameter. Perhaps the leading / could be required in this circumstance. That would eliminate ambiguity, and there isn't much additional code to it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2868 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACh9MevIg-y-IXfIzAywxZ7Ev0UfVkCmks5snHNAgaJpZM4OyTQM> .

[greg-1-anderson]

I am in favor of uli [user] [path], which is the same order that existed in Drush 8. Perhaps @weitzman just made a typo when he said that uli [path] [user] was okay, because it sounded like he was agreeing with me, not disagreeing.

It is not necessary to require a slash before a path in the two-argument variety of this command. You correctly summarized my position: in the one-argument variety, requiring a slash on the path would be sufficient to disambiguate between user and path.

It's up to @weitzman whether to accept that or not. I can help you with a policy file if it comes to that.

[weitzman]

Right - I meant uli [user] [path]. Lets do that. Lets not have the old options though. Those users will need to lookup via user-information.

I notice that the PR has conflicts. Hopefully those are quick to resolve.