Factoring this functionality out of the User class would indeed be useful. However, the UserName object as you describe it wold not be a pure value object: it would implement application logic, and depend on configuration. Application logic should be in a stateless service instead, especially if it depends on configuration. I see two options:

1. create a UserNameUtils service similar to the LanguageNameUtils service. No UserName class is needed.
2. create a UserNameFactory service that instantiates UserName instances, by performing all necessary normalizations and checks up front.

Option (1) seems more lightweight to me. However, option (2) may be preferable if there is substantial benefit to be gained from re-using intermediate results, e.g. isIP() can rely on a canonical name if it has already been computed. Whether this is worthwhile depends on usage patterns.

class UserName as ValueObject is a good way to improve strict types in PHP, give us the map of coupling, prepare this "domain" for re-engineering, etc.
But probably this is more than we need for a 1 string? :)
New stateless instantiable UserNameService* is more preferred

I'm not sure how we can get IP for the NAME field. Probably that is a more important point for reengineering :)

Factory from Option (2) is a good service to make an instance of UserName from DB without validation.
But public function SomeFactory::validate() is looks not pretty

For UserNameUtils, I don't know if this will remain stateless, since it will still interact with some globals
For UserNameFactory, it is indeed helpful to have the intermediate results - User::isUsableName also requires User::isValidUserName to pass, and User::isCreatableName requires User::isUsableName to pass

In T245231#5885545, @DannyS712 wrote:

For UserNameUtils, I don't know if this will remain stateless, since it will still interact with some globals

As long as it only reads these variables, it's still stateless. Also, is there a reason the values can't be injected instead of accessing the globals directly?

For UserNameFactory, it is indeed helpful to have the intermediate results - User::isUsableName also requires User::isValidUserName to pass, and User::isCreatableName requires User::isUsableName to pass

This can easily be done without remembering state.

In T245231#5887516, @daniel wrote:

In T245231#5885545, @DannyS712 wrote:

For UserNameUtils, I don't know if this will remain stateless, since it will still interact with some globals

As long as it only reads these variables, it's still stateless. Also, is there a reason the values can't be injected instead of accessing the globals directly?

It makes it easier to just call, eg, ( new UserName( 'DannyS712' ) )->isCreatable() rather than needing to have a factory construct the UserName with the globals

Change 573015 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add a UserName class to replace static methods in the User class

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

Change 573015 abandoned by DannyS712:
Add a UserName class to replace static methods in the User class

Reason:
Redoing as a service

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

Change 574261 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add a new UserNameUtils service

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

Rewritten as a service

In T245231#5894688, @DannyS712 wrote:

It makes it easier to just call, eg, ( new UserName( 'DannyS712' ) )->isCreatable() rather than needing to have a factory construct the UserName with the globals

But then UserName needs to know about the globals, which is should not.

In T245231#5910831, @DannyS712 wrote:

Rewritten as a service

Looks good at a glance!

@Daimona @daniel I'm hoping this lands before 1.35 is cut - is anything else needed?

Change 574261 merged by jenkins-bot:
[mediawiki/core@master] Add a new UserNameUtils service

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

@DannyS712 On my local vagrant environment, when logging in I am sometimes getting the following error:
[error] [075838679bc5a73b91a04ac4] /wiki/Special:CentralLogin/complete?token=f6b859992df76f12f180865152bcbbc6 ErrorException from line 334 of /vagrant/mediawiki/includes/context/RequestContext.php: PHP Warning: Recursion detected in RequestContext::getLanguage
(full stack trace attached)

I guess UserNameUtils::isUsable() is called during the login process, and it needs to find out the user's language, so it kicks off the login process again leading to recursion.

I have seen bugs very similar to this in the past, e.g. T180050, T226777, which might have a solution.

FYI, I have only been able to reproduce this on my vagrant environment (MediaWiki 1.35.0-alpha (7899d71) 06:27, 31 March 2020). I cannot in other local environments or on beta.

Full stack trace:

Heh, depending on the main RequestContext isn't ideal... We could perhaps use an anonymous class implementing MessageLocalizer and proxying the call to wfMessage. And maybe do that conditionally, only when the main context isn't available. This, together with the fact that we only need to use the content language for the message, might be enough to fix (haven't tested).

In T245231#6014349, @Daimona wrote:

Heh, depending on the main RequestContext isn't ideal... We could perhaps use an anonymous class implementing MessageLocalizer and proxying the call to wfMessage. And maybe do that conditionally, only when the main context isn't available. This, together with the fact that we only need to use the content language for the message, might be enough to fix (haven't tested).

T247127: Add a MessageLocalizer service has a patch to do exactly that (proxy wfMessage) - https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/577687/ - not ideal, but works and should resolve the issue above, since the problem is the RequestContext::msg sets the Message's context, and previously it had not context set, because one isn't needed

UserNameUtils is using the TitleFactory.

The Title object is only used for title validation. I would assume a TitleValue should be enough for that. It is possible in this case to use a TitleValue? It should be cheaper to instance and have no global state

In T245231#6020751, @Umherirrender wrote:

UserNameUtils is using the TitleFactory.

The Title object is only used for title validation. I would assume a TitleValue should be enough for that. It is possible in this case to use a TitleValue? It should be cheaper to instance and have no global state

I don't think so:

// Ensure that the name can't be misresolved as a different title,
// such as with extra namespace keys at the start.
$title = $this->titleFactory->newFromText( $name );
if ( $title === null
	|| $title->getNamespace()
	|| strcmp( $name, $title->getPrefixedText() )
) {
	return false;
}

TitleValues are constructed with a namespace specified, and the point of this check is to ensure that when no namespace is explicitly specified the name would be in the article namespace (i.e. not a prefixed namespace) to avoid a user named Template:Foo or Talk:Bar - TitleValue offers no such parsing of the string input, instead requiring a namespace to be specified

Yes, you have still to parse the title, but just using MediaWikiTitleCodec as TitleParser and get a TitleValue from it.

There is no need to know about page ids or protection or other things which could all be provided by the Title objects.
The TitleFactory needs later a load balancer and all the heavy things.
It seems overkill to require that when just checking if a string is a valid page name with or without a namespace.

Change 586141 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/core@master] Use TitleParser in UserNameUtils

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

@AMooney, I understand that code review is in progress for this ticket, but there's no one assigned to it. Should it be moved back to Code Review Needed?

Change 586141 merged by jenkins-bot:
[mediawiki/core@master] Use TitleParser in UserNameUtils

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /586141