@Samwilson TechCom is considering this RFC for public discussion next Wednesday, October 25, at 2pm PDT. Will you or someone else involved with this project be available on IRC?

@daniel: No, I'm afraid I'm off all next week. Could it be the week after? (Or maybe @MaxSem is available?)

Yes, I will totally be there.

@Samwilson we can do it either way - do you prefer to wait a week, or shall we do it with @MaxSem?

Let's leave it till the following week; I'd prefer to be there. Thanks!

Ok, let's target November 1st then.

As to the proposal as such: I personally agree that a PreferencesFactory service is the correct approach. Note that there's an open bug for the mechanism for overriding services, though: T153256: Unable to overwrite services using MediaWikiServices hook. Perhaps I should find time to finally look into that.

Confirming that this RFC will be the topic of the IRC discussion on #wikimedia-office on Wednesday, November 1str. Note that this will happen at the usual time for the US (2pm PDT), but one hour earlier than usual in Europe (22:00 CET).

Minutes from the IRC meeting: https://tools.wmflabs.org/meetbot/wikimedia-office/2017/wikimedia-office.2017-11-01-21.01.html

• https://phabricator.wikimedia.org/T178449 (DanielK_WMDE, 21:01:39)
• <DanielK_WMDE> but it makes sense to not interfere with extensions that add preferences. that's a different thing from adding a new preference *mechanism*. i expect much less trouble there. few extensions will want to do that. (SMalyshev, 21:27:56)
• the list of available prefs depends on user rights/groups. preference declarations could reflect that. (DanielK_WMDE, 21:32:19)
• separating the idea of opetions for a given user and declarations of preferences in general seems liek a good idea (DanielK_WMDE, 21:32:44)
• defining a PrefernceFactory service is preferred over introducing a new hook point (DanielK_WMDE, 21:33:38)
• <TimStarling> the default "username" preference is the actual user name <DanielK_WMDE> TimStarling: seems like we'd want to allow callbacks for the default value (DanielK_WMDE, 21:37:33)
• <Krinkle> I assume site config would dictate which factory is used, through service wiring or wgConf, not unlike how we have configurable objectcache/jobqueue/filebackend. (DanielK_WMDE, 21:46:12)
• [14:53:25] <DanielK_WMDE> samwilson: if there is two preference pages, each should use a different PreferenceFactory. (SMalyshev, 21:56:17)

Outcome: needs more thought. @Samwilson will continue with code experiments. Meanwhile, this RFC goes is "under discussion".

Couldn't make it to the RfC discussion unfortunately, some comments:

• Can we use this chance to replace the Preferences mess with a service + value object combination? Ie. instead of a factory that creates an object which has complex persistence logic, all that logic would go into the service, it would return a simple hashlike object, rendering preferences into HTMLForm definitions would happen in a separate helper class (no mixing of business and view logic) and the legacy Preferences class would be a deprecated wrapper around the service. (This was brought up a couple times in the discussion but doesn't seem like any decision was made.)
• Re: Special:Preferences vs. Special:GlobalPreferences, using different services seems impractical (would require getting a Title object in the service bootstrap phase which seems like a bad idea). A better way is to add a context parameter (global vs. local) to the service method for getting preferences. (And then internally it might or might not make sense to implement that by a bridge service which has a local and a global service instance and decides depending on the context which one to forward the call to; or a decorator around the core service... but the service locator should not know about any of that.)
• Separating abstract preferences from the preferences of a specific user seems like a solution in search of a problem to me. When would you ever need it? The set of potential preferences is not even necessarily finite (think Gadgets 3.0 where any user can define a gadget for themselves and that will add another preference item which the preference system cannot know about until you ask for the preferences of that user specifically).

<Krinkle> I assume site config would dictate which factory is used, through service wiring or wgConf, not unlike how we have configurable objectcache/jobqueue/filebackend. (DanielK_WMDE, 21:46:12)

• objectcache/jobqueue/filebackend are pre-dependency-injection and not great examples IMO. OTOH the way we do dependency injection doesn't really support resolving conflicts between extensions. Normally you'd have a preferences.extension1 service and a preferences.extension2 service and the DI logic would pick one by default and you could override it in the service configuration; our service configuration is based on anonymous functions defined in plain PHP files so there isn't really a way to reference them. Punting the problem to something like ObjectCache::newFromParams or ObjectFactory basically means that your dependency injection container calls another dependency injection container to do the dependency injecting for him; at that point it would be better to just admit that opting for hard-coded DI was a mistake, and rethink that.

Change 389665 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/core@master] Convert Preferences class into PreferencesFactory service

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

I think the service and a couple of value objects (PreferenceDescription and PreferenceDescrpition?) are the way to go, but that doing so in one leap is too much. https://gerrit.wikimedia.org/r/389665 is an attempt to move toward this better architecture, but not do it all at once; it is fully backwards compatible.

It introduces the PreferencesFactory service, and deprecates the \Preferences class. The new service class (which I was hoping Gerrit would see as a move, as pretty much all the code is an exact copy from Preferences) basically does nothing other than remove all static scoping. This patch also adds the $wgPreferencesFactoryClass config var.

Is this an okay direction to be going in? I don't want to make this a huge patch that rips apart Preferences in one fell swoop and creates the whole new architecture, so I'm hoping that this is a step towards that and at least doesn't make things worse.

(This patch is not complete in terms of commenting, because I just want to gauge people's opinions of it.)

Converting everything to non-static is always a good first step in moving towards a service-based architecture.

Are you proposing to make that move in small steps, or to make the first step, which is enough to get GlobalPreferences to work, and then hope that some unspecified team continues the move at some unspecified time in the future? :)
(This is the same topic that has been brought up by Daniel in Avoiding the Tech Debt Trap. We have lots of tech debt piled up; it's unfair if dealing with it falls on whichever team has to touch the given component next. It's probably fatal in the long run if it does not fall on anyone and people just keep rolling it forward while doing surface-level patches. So what to do about it?)

Are you proposing to make that move in small steps, or to make the first step, which is enough to get GlobalPreferences to work, and then hope that some unspecified team continues the move at some unspecified time in the future? :)

Damn, you saw straight through me! ;-)

No, actually I do want to carry this through to a better design. I'll devote volunteer time to it if CommTech can't spare me.

I think the patch above is a good first step (and of course, still subject to some polishing). I would consider the required next steps to be something like:

1. Add this service with full B/C with \Preferences;
2. Move form stuff out of PreferencesFactory and into PreferencesForm;
3. Create new \MediaWiki\Preferences\PreferenceDescription class and move the preference-structure stuff out of PreferencesFactory into that;
4. Make it possible for extensions to add preferences not via the GetPreferences hook but something like $preferencesFactory->add( $preferenceDescription );
5. Remove the \Preferences class (in 1.32?).

There is lots more that could be improved, but I think the above would leave things in a state slightly better than now and without too many obvious "this is just a temporary thing" bits of code (or at least without any new ones).

That said, the problem at hand for me is actually that of GlobalPreferences, and that is what needs to be solved. The quick hacky solution was to add a new hook; the better one was to create this service. I think if the tech debt created here is in the form of a hollowed-out deprecated Preferences class, and the GlobalPrefs problem is fixed, then that's not such a bad thing. Things are slightly better than they were, maybe not good enough yet but at least moving towards good enough.

It would be great to be able to get this sorted by the end of the year.

After discussion on this patch I've removed the configuration variable for setting the class of the PreferencesFactory, and instead going with the previous way of redefining the service where required. Is this okay?

This patch is not complete, because it doesn't yet contain all required documentation and tests have not been fixed up (or rather, new ones added, because the old ones are still required for testing backwards-compatibility), but I'd like to get more feedback on it before going further.

@daniel Could this perhaps be scheduled for next week, Wednesday 6 December?

For reference, the changes made to Preferences in moving it to PreferencesFactory (i.e. the output of git diff master:includes/Preferences.php includes/preferences/PreferencesFactory.php) is in P6412. (I can't figure out how to do cross-branch, cross-file diffs in Gerrit or Diffusion.)

Hey @daniel, this came up in our team meeting today. Any updates about getting this scheduled?

@Niharika I was on vacation and missed two TechCom meetings. I'll bring it up today. Chances for next week are good I think, if nothing more urgent comes up.

This RFC has been scheduled for public discussion on IRC on Wednesday, December 13.
As always, the discussion will take place in the IRC channel #wikimedia-office on Wednesday, at 2pm PST / 23:00 CET / 22:00 UTC.

Pinging @Samwilson and @Niharika

Cool, thanks!

Thanks Daniel!

@daniel: Would be awesome if we could get a clear path forward, as this RFC is the only thing currently blocking work on GlobalPreferences.

Uhm... there was a lot of feedback from the last discussion. But it seems no changes have been made in response to that. Am I missing something?

It would be good to at least have an updated proposal or an updated patch to discuss.
Or are we only talking about https://gerrit.wikimedia.org/r/#/c/389665/ ?

RFC discussions are typically good at finding issues, and maybe giving vague hints at solutions. I don't think a complete design ready for implementation will come from it. What does help is having concrete questions to answer. What is currently barring the way forward? Which questions need to be resolved?

@daniel: Basically, we just need to know if https://gerrit.wikimedia.org/r/#/c/389665/is a good jumping off point. Is this something we can build on going forward or do we need to take a different approach? It sounds like most people think the patch is going in the right direction, but we need a bit more certainty before we build all of GlobalPreferences on top of it. Is there anything significant that Sam needs to change about his current PreferencesFactory approach before that could be merged?

At a glance, that patch is a first refactoring step, and as such is OK - no need for an RFC discussion about just that. But I don't think that patch is sufficient to allow you to do what you want to do. In particular, it does not introduce the idea of a PreferenceDescription as mentioned in T178449#3739845. The proposal that we should discuss I think is how PreferenceDescriptions should be introduced and how it would fir in with the existing info in the database and existing UI as well as with the future infrastructure and UI you want to build. This is what was discussed during the last session. I assumed that you were requesting the new IRC session to discuss a proposal for this.

@daniel: Thanks for the clarification! Just wanted to figure out what we can actually push forward with in the meantime. I don't want us to add a bunch of technical debt by prematurely moving in the wrong direction just to support GlobalPreferences, but I also don't want us to stall GlobalPrefs if we can actually get it up and running in the meantime (prior to a full prefs overhaul). This probably makes me the kind of manager that you warned about! Honestly, though, I do care about the preferences codebase and know that it needs a complete overhaul. I'm going to push for some attention to this (along with some other critical orphaned features) during the upcoming Annual Planning meetings.

(Sorry I didn't reply earlier, I've been on holiday. Thanks @kaldari, what you say is exactly right.)

In T178449#3830523, @daniel wrote:

At a glance, that patch is a first refactoring step, and as such is OK - no need for an RFC discussion about just that. But I don't think that patch is sufficient to allow you to do what you want to do. In particular, it does not introduce the idea of a PreferenceDescription as mentioned in T178449#3739845. The proposal that we should discuss I think is how PreferenceDescriptions should be introduced and how it would fir in with the existing info in the database and existing UI as well as with the future infrastructure and UI you want to build. This is what was discussed during the last session. I assumed that you were requesting the new IRC session to discuss a proposal for this.

That's great news if there's agreement that this is a good way forward; that wasn't obvious to be from the last IRC meeting or comments on the patch — there are things like not having a config variable to define the PreferencesFactory class, and not adding a PreferencesFactoryInterface to formalize its structure.

Anyway, I'll get the rest of the documentation written in https://gerrit.wikimedia.org/r/#/c/389665 and hopefully it can be merged soon.

The introduction of a service definitely is enough (minimally) for GlobalPreferences to do what it needs to do, so we can get moving with that as soon as this is merged. I wanted this first patch to be as small as possible, because of the annoyance of tracking the same code moving from Preferences to PreferencesFactory (see diff in P6412). The subsequent parts of refactoring Preferences will happen in stages after this patch. Perhaps this leaves the possibility of the deprecated Preferences class hanging around for a long time, but I'll be doing my best to keep working on this.

It is still worth having the IRC meeting tomorrow, just to be quite clear about the next steps.

@Samwilson Hi! I'm not saying the patch is perfect. I'm saying that the issues it has (like no unit tests) do not warrant an RFC discussion, they can be resolved on the patch. What could use an RFC discussion is the architecture needed for representing, managing and extending PreferenceDescriptions, and building a UI from that. When scheduling this, I failed to realize that there is no clear proposal for this yet. In my mind, that's a precondition to a useful RFC discussion.

I've clarified the task description by separating between "Problem statement" and the considered/proposed solutions.

I don't oppose solution 2 (I've only barely started reading into it), but based on the IRC conversation from today, I'm curious why solution 1 is insufficient.

I previously incorrectly assumed that the rationale for solution 2 is that the system needs to register additional special preferences for each "real" preference. Given that this is the case, it seems a designated hook intended to be read-only after registration has completed, seems adequate.

The task description says solution 1 is inadequate because it doesn't allow adding modules to the preferences UI, but that seems unrelated to the problem at hand.

Thanks @Krinkle, new desc makes sense.

As for the simple option 1, of a new hook: I'd say that the main argument against it is that Preferences is pretty messy and the Services option gives more flexibility and (hopefully) a slight improvement in code quality. But yeah... if TechCom wants to say "add a new hook" then I'm quite happy to do that instead! :-) Would be much quicker and we wouldn't need to refactor all this stuff. It'd almost be a one-line change:

   Hooks::run( 'GetPreferences', [ $user, &$defaultPreferences ] );
   self::loadPreferenceValues( $user, $context, $defaultPreferences );
+  Hooks::run( 'GetPreferencesWithValues', [ $user, &$defaultPreferences ] );

Notes from the public IRC meeting on December 13: https://tools.wmflabs.org/meetbot/wikimedia-office/2017/wikimedia-office.2017-12-13-22.03.html

This meeting was about guidance for the refactoring needed to allow an extension to override preference storage, as well as exploring the topic of global preferences in general.

@Samwilson seemed satisfied with the guidance provided by the people in attendance of the meeting.

Change 389665 merged by jenkins-bot:
[mediawiki/core@master] Convert Preferences class into PreferencesFactory service

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