Page MenuHomePhabricator

Create a User factory
Open, Needs TriagePublic

Description

There should be a UserFactory service for constructing users

In the initial stage, it can just proxy the existing User::newFrom* methods, similar to the existing TitleFactory, which was created in https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/95586dd93721d17135cd4d0dea53f62bd7b781f5

Add TitleFactory
Makes it possible to mock static Title methods in tests, where they are one of the more common reasons for not being able to use MediaWikiUnitTestCase.
Actually introducing dependency injection to Title is left for the future.

Subsequently, it will be converted to an actual factory, replacing (check mark means the logic is now in the factory)

  • User::newFromName - inject UserNameUtils
  • User::newFromId
  • User::newFromActorId
  • User::newFromIdentity
  • User::newFromAnyId
  • User::newFromConfirmationCode - inject LoadBalancer
  • User::newFromRow

This will be another step in splitting up the User "god object"

Event Timeline

Restricted Application added a project: User-DannyS712. · View Herald TranscriptMay 23 2020, 1:28 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
DannyS712 moved this task from Unsorted to Next on the User-DannyS712 board.May 23 2020, 1:28 AM

Change 598144 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] UserFactory v.1 - wrapper for User::newFrom* static methods

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

eprodromou added a subscriber: eprodromou.

@DannyS712 thanks for tagging us. In the future, just tag CPT, and we'll route it to the right place on our boards!

Also maybe idFromName() ? Doesn't return a User though. :/

Change 598144 merged by jenkins-bot:
[mediawiki/core@master] UserFactory v.1 - wrapper for User::newFrom* static methods

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

Change 605297 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Move User::newFromIdentity to UserFactory

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

Also maybe idFromName() ? Doesn't return a User though. :/

Though, I wonder if that method should be replaced with something like:

newFromName( 'Example' )->getId()

Change 605297 merged by jenkins-bot:
[mediawiki/core@master] Move User::newFromIdentity logic to UserFactory

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

Change 630241 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Move User::newFromName logic to UserFactory

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

Change 630241 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Move User::newFromName logic to UserFactory

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

Now has more code to review

Change 630241 merged by jenkins-bot:
[mediawiki/core@master] Move User::newFromName logic to UserFactory

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

Change 630795 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add UserRigorOptions interface

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

DannyS712 updated the task description. (Show Details)Oct 2 2020, 2:12 AM

Change 630795 merged by jenkins-bot:
[mediawiki/core@master] Add UserRigorOptions interface

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

Change 631481 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Migrate more User::newFrom* methods to UserFactory

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

Change 631811 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Inject dependencies into PasswordReset, and cleanup

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

Change 631811 merged by jenkins-bot:
[mediawiki/core@master] Inject dependencies into PasswordReset, and cleanup

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

DannyS712 updated the task description. (Show Details)Oct 3 2020, 2:08 AM

Change 631481 merged by jenkins-bot:
[mediawiki/core@master] Migrate more User::newFrom* methods to UserFactory

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

DannyS712 updated the task description. (Show Details)Oct 7 2020, 8:21 PM

Since User::newFromRow is basically just a wrapper for User::loadFromRow, I don't think it needs to be migrated, which results in needing to create the service even though none of the dependencies would be needed

I suggest next creating a UserStore for database interactions:

  • User::createNew -> UserStore::createNewUser
  • User::addToDatabase -> UserStore::addUserToDatabase
  • User::updateActorId -> UserStore::updateActorId (private method, only needed for ::createNew and ::addToDatabase)

User::newSystemUser could go in either the UserStore or the UserFactory. Thoughts?