Like maybe we don't wanna ssh into the server and use the maintenance script, so we write a little frontend to call the equivalent internal function... oh wait. This is it. It's been deprecated.

Or maybe we are using an external auth and then just want to save it locally after all, because the external thing is going down in a week and this is easier for everyone. You know. Import it. How are we supposed to do that?

I haven't looked into the details (apologies), but my first thought is: What method is maintenance/changePassword.php using? If those are public and non-deprecated, would those work for your custom frontend?

...it seems maintenance/changePassword.php just duplicates the content of setPasswordInternal(), which apparently isn't deprecated, just really easily confused for setInternalPassword() when you're not looking closely enough.

...nevermind? I guess?

setPasswordInternal is private so probably not much help. (setInternalPassword was also internal, as the name suggests, MediaWiki just hasn't been good traditionally about keeping internal methods private.) Password handling via the User class is deprecated, we just don't bother with deprecating non-public methods.

The underlying problem is that users are not guaranteed to have passwords, or to store passwords in the user_password table which User:setPassword historically dealt with, so anything relying on that method would have been very fragile, so it was removed and replaced with more-or-less compatible logic.

So you'll have to "duplicate" setPasswordInternal (it's two method calls, one of which you might or might not need, plus some error handling, so it's not a whole lot code duplication). If you need access to user_password specifically, you'll probably have to implement that manually, or wait for T183420: Authentication data should not be available through the normal DB abstraction layer (which will presumably introduce some kind of password store class), but it's unlikely you need that.

Okay, restoring this, as evidently this still applies, I was just looking at the wrong method.

Per the above, this method is needed because we currently have no other way to implement this functionality for local accounts besides duplicating the content of the method everywhere. The whole point point of having a method is to not have to do that, so removing it before a replacement exists is nonsensical.

Deprecate the thing when there is a replacement. Otherwise we're looking at the situation where the only path forward is to copy the function source, which in turn means that if and when there is an actual replacement, we're a lot more likely to wind up in the position where we have no way to find out about it beyond everything breaking entirely. A deprecation notice should serve as this alert, not put developers in the position that they don't even get the alert.

Going to un-deprecate, though we are in favor of eventually deprecating the User Object. That is a while off.

To expand a little on what Kate said:

The problem here seems to be that in 1.27, when User::setPassword was deprecated, no good substitute was created in AuthManager. The one substitute I can see, changeAuthenticationData, lives on the User class again for some reason.

I think Isarra is right to ask for simple things to be simple. Perhaps we should have the concept of "simple auth", and a method to check whether the wiki is using "simple auth", and another method to set the password for "simple auth". We pretty much have that with LocalPasswordPrimaryAuthenticationProvider, but it should be simple for application logic to get an instance and set a password.

Until we have that, User::setPassword can't really be deprecated. I suggest making a patch replacing the @deprecated tag with @note tag explaining that generally, it's better to use AuthManager, and referencing this here ticket.

In T219689#5073914, @kchapman wrote:

Going to un-deprecate, though we are in favor of eventually deprecating the User Object. That is a while off.

That's an interesting statement...User might have a bunch of stuff in it that really shouldn't be there, but its hard to imagine getting rid of an object representing a User entirely...Is there anything public I can read about future plans in that area?

I'd rather we didn't undeprecate it, for reasons Gergo already mentioned
and I'll go into further when I have the chance.

In T219689#5075650, @Anomie wrote:

I'd rather we didn't undeprecate it, for reasons Gergo already mentioned
and I'll go into further when I have the chance.

I'd also rather offer a good alternative, but I don't see one....

In T219689#5075644, @Bawolff wrote:

That's an interesting statement...User might have a bunch of stuff in it that really shouldn't be there, but its hard to imagine getting rid of an object representing a User entirely...Is there anything public I can read about future plans in that area?

Of course we need to have an object identifying a user, but turning User into that is much harder than creating a new one. The intent is to replace uses of the User class with uses of the UserIdentity class. User implements UserIdentity, making the transition seamless. But there is also a "plain value" implementation of UserIdentity, called UserIdentityValue (blame me for the unimaginative name).

RevisionRecord already uses a UserIdentityValue to represent the user (or actor) who made an edit. Operations currently performed via the User object should in the future be performed using service objects that take a UserIdentity as a parameter.

I still don't see how

$status = $user->changeAuthenticationData( [
    'username' => $user->getName(),
    'password' => $password,
    'retype' => $password,
] );

(what changePassword.php does now) is an unacceptable level of complexity / code duplication compared to $user->setPassword( $password ) (which also cripples error handling for compatibilty with the legacy method signature). Less elegant, sure (if someone wants to improve that, time is probably better spent on making changeAuthenticationData() autofill the username or making the handling of the retype field more intelligent so it's not required when the data does not come from $_GET/$_POST, both of which should be doable), but copying a single method call into your code is not exactly rocket science.

More to the point, this task is based on a fundamental misunderstanding of what User::setPassword does: it's not a setter for user_password, it updates whatever the primary password store is (if there is one). If the wiki uses LdapAuthentication, it will set the LDAP password; if it uses CentralAuth, it will set globaluser.gu_password. If the wiki uses GoogleLogin, it will be a no-op. And the exact set of data needed cannot be determined without knowing the exact auth config. For example the wiki might authenticate via multiple LDAP domains in which case the notion of changing passwords isn't sensible without specifying the domain. Most of this is not even new to AuthManager, it was just handled in ad-hoc / hacky ways before.

AuthManager provides a decent mechanism for dealing with all this complexity: call AuthManager::getAuthenticationRequests() to generate a form descriptor of the needed auth data fields, then pass the data for those fields to User::changeAuthenticationData(). Undeprecating the pre-AuthManager mechanism that looks like it would work but then might or might not would be a disservice to users, IMO.

That should take care of the first use case mentioned in the task description; the second does not make much sense. Hopefully you don't store the password in cleartext in the external authentication backend, so you need to operate on a lower level of abstraction for migrating it. As I mentioned earlier, ideally we should have a password store service; but we do not have it now, and did not in the past, so there's nothing to undeprecate.

In T219689#5075650, @Anomie wrote:

I'd rather we didn't undeprecate it, for reasons Gergo already mentioned
and I'll go into further when I have the chance.

That was supposed to be a private reply to @kchapman. Gmail's interface hid that the "kchapman" I was replying to was actually T219689+public+somehash@phabricator.wikimedia.org. 😒

Anyway, Gergő's later reply at T219689#5077029 already covered what I'd have gone into.

So if all we need is that, why does maintenance/changePassword.php duplicate almost the entirety of setPasswordInternal()? If the error handling stuff is actually bad, then maybe both of these need to be changed? In which case, again, wouldn't it be a lot easier if there were a single core function that that we could all call, and thus only need to change in one place?

(Instead of, well, duplicating these again in other places...)

Change 502348 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/core@master] Improve changePassword.php error handling

https://gerrit.wikimedia.org/r/502348

Change 502348 merged by jenkins-bot:
[mediawiki/core@master] Improve changePassword.php error handling

https://gerrit.wikimedia.org/r/502348