Page MenuHomePhabricator

Deprecate GlobalPreferencesFactory::setUser() [medium]
Open, Needs TriagePublic


Due to MediaWiki's reliance on hooks and hooks unpredictable nature, it gets hard to guarantee that between your code calling setUser() and the end of its GlobalPreferences usage, there wouldn't be a hook call that would result in some other code altering the preferences factory state. Therefore, I propose to deprecate this pattern in favor of passing the user object as a parameter to functions that require it.

Event Timeline

MaxSem created this task.Nov 16 2019, 6:07 AM
Restricted Application added a project: Community-Tech. · View Herald TranscriptNov 16 2019, 6:07 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
ifried renamed this task from Deprecate GlobalPreferencesFactory::setUser() to Deprecate GlobalPreferencesFactory::setUser() [medium].Apr 9 2020, 11:23 PM
ifried assigned this task to Samwilson.
ifried moved this task from Ready to In Development on the Community-Tech (Kanban-2019-20-Q4) board.

The GlobalPreferencesFactory::setUser() method has a note:

Note that not many of this class's methods use this, and you have to pass $user again.
@todo This should really be higher up the class hierarchy.

And it does seem like another fix to this (other than to remove the setUser() method) is to move it up to PreferencesFactory. Preferences (whether global or not) can't exist without a user, so it makes sense to set it for the whole class. It would also mean that the following DefaultPreferencesFactory methods wouldn't have to have it provided, which would clean things up a little bit:

public function getFormDescriptor( User $user, IContextSource $context ) {
private function loadPreferenceValues( User $user, IContextSource $context, &$defaultPreferences )
protected function profilePreferences( User $user, IContextSource $context, &$defaultPreferences, $canIPUseHTTPS )
protected function skinPreferences( User $user, IContextSource $context, &$defaultPreferences )
protected function datetimePreferences( $user, IContextSource $context, &$defaultPreferences )
protected function renderingPreferences( User $user, MessageLocalizer $l10n, &$defaultPreferences )
protected function editingPreferences( User $user, MessageLocalizer $l10n, &$defaultPreferences )
protected function rcPreferences( User $user, MessageLocalizer $l10n, &$defaultPreferences )
protected function watchlistPreferences( User $user, IContextSource $context, &$defaultPreferences )
protected function generateSkinOptions( User $user, IContextSource $context )
public function getForm( User $user, IContextSource $context, $formClass = PreferencesFormOOUI::class, array $remove = [] )

Does this sound like a better solution?

Change 588194 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/core@master] [WIP] Add PreferencesFactory::setUser()

Change 588194 merged by jenkins-bot:
[mediawiki/core@master] Add PreferencesFactory::setUser()

Change 594612 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/extensions/GlobalPreferences@master] Remove method that's now present in parent class

Change 594612 merged by jenkins-bot:
[mediawiki/extensions/GlobalPreferences@master] Remove method that's now present in parent class

Change 588194 merged by jenkins-bot:
[mediawiki/core@master] Add PreferencesFactory::setUser()

-	 * @param User $user
+	 * @param User $user Deprecated since 1.35, and will be ignored if $this->setUser() has been used.
	 * @param IContextSource $contextSource
	 * @return mixed[][] An HTMLForm descriptor array.
	public function getFormDescriptor( User $user, IContextSource $contextSource );

How will this parameter be removed? (In terms of deprecation, migration, and removal)

How will this parameter be removed? (In terms of deprecation, migration, and removal)

Going to flag @Samwilson if he hasn't seen this. I assume the question is for him...

I was thinking the deprecation process would be along the lines of:

  1. Mark as deprecated in 1.35 (done)
  2. Other things using these methods will be updated (in progress)
  3. Add wfDeprecated() when they're called with the User parameter (i.e. get rid of type hints and check type of first parameter)
  4. After 1.36, remove code

But we can still revert this, if you feel like it's the wrong approach. The user would then be passed in to all of the GlobalPreferencesFactory methods that require it (as suggested in the description of this task). I'm sorry I didn't get more feedback on this other approach before implementing it.

@Krinkle do you think this should be reverted, or changed?

Krinkle added a comment.EditedMay 22 2020, 4:41 PM

@Krinkle do you think this should be reverted, or changed?

I haven't fully analysed the GlobalPreferences side of the core service yet, so apologies if the below seems wrong.

As I understand it, this change adds ephemeral state to the PreferencesFactory service class (the setUser method). And unlike DI methods such as passing a Logger in the constructor or via setLogger, this User object isn't insignificant to the behaviour of the class, and also isn't meant to be sticky or assumed as preset at run-time. That is, setUser isn't called from the service wiring (for the same reason that wgUser is deprecated, the service must not assume or vary vary by web request, and be usable for other users as well).

Instead, setUser is meant to be called everytime the service is interacted with. And if I understand correctly, if a caller were to not do this and end up operating on a User object left by a previous caler, that would be unintentional, right?

If all that is right, then yes I think this should be reverted. It feels like this is the kind of thing one would want to pass as parameter to the relevant methods, or if there is a lot of stuff to maintain over several calls, then you'd want a method the produces an instance with that state encapsulated, like a factory class - which this is, so that adds some confusion for me as well, e.g. someFactory->forUser(x)->someMethod().

If passing as parameter feels like a compromise, then I've likely missed something. Is/was there something in the GlobalPreferences implementation here that called for the setUser method? Let me know :)

Change 598898 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/extensions/GlobalPreferences@master] Remove GlobalPreferencesFactory::setUser()

Change 598951 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/core@master] Remove PreferencesFactory::setUser()

Change 598898 merged by jenkins-bot:
[mediawiki/extensions/GlobalPreferences@master] Remove GlobalPreferencesFactory::setUser()

Change 598951 merged by jenkins-bot:
[mediawiki/core@master] Remove PreferencesFactory::setUser()

Both patches have been merged, and I think there's nothing left to be done.

There's no new functionality here, so QA is about confirming nothing's changed.

I haven't observed any differences in behaviour in:

Which I believe covers every component changed in this task.

I also searched other places in the codebase (using which might need updating as a result of this change. I found only T254988 (which isn't in production, although might in future).

I tested the core functionality of the UIs and APIs and the specific hooks and methods which have been modified. These include:

  • hooks which modify Special:Preferences to, for example, disable options which are already set globally
  • methods which create the user-specific links to custom CSS/JS

Initially, I could compare behaviour before and after this change by comparing beta with testwiki. But, after the train, I used testwiki and compared it with my own understanding of how the functionality works.

Most recently tested environment: MediaWiki 1.35.0-wmf.36 (08f1868) 18:37, 9 June 2020; GlobalPreferences 0.1.2 (227b187) 00:38, 9 June 2020.