[21:01:15] #startmeeting TechCom RFC discussion [21:01:16] Meeting started Wed Nov 1 21:01:15 2017 UTC and is due to finish in 60 minutes. The chair is DanielK_WMDE. Information about MeetBot at http://wiki.debian.org/MeetBot. [21:01:16] Useful Commands: #action #agreed #help #info #idea #link #topic #startvote. [21:01:16] The meeting name has been set to 'techcom_rfc_discussion' [21:01:31] #topic RFC: How to modify all preferences? [21:01:39] #link https://phabricator.wikimedia.org/T178449 [21:01:54] so... [21:01:57] Good morning :) [21:02:11] * DanielK_WMDE blinks and looks at the clock [21:02:13] hey [21:02:31] hehe it's always morning in IRC [21:03:12] samwilson: would you like to explain your proposal? [21:03:28] and perhaps ask the most important questions you want to have answered during this session? [21:03:40] sure. so: we need a way to do things with all preferences [21:03:59] after all extensions have added theirs [21:04:16] there seem to be two main ways to do this, as explained in the tickets [21:04:28] should I go though them? happy to. [21:04:43] please [21:05:08] ok, so the current proposal is to add a new PreferencesFactory service [21:05:42] this would mean that anywhere that wants a Preferences object, would get that service, and use it to get a Preferences [21:05:52] how do extensions currently add preferences? how would they do that after that new service was defined? [21:06:13] Nothing would change on that front. they'd still use the GetPreferences hook [21:06:34] it's only code that is using Preferences that would need to change [21:06:45] and the current proposal is fully backwards-compatible [21:07:13] i assume the current method would be kept and delegate to the new service [21:07:14] the current Preferences class is a collection of static methods, pretty much all of which take a user and a context [21:07:23] so code that uses the preferences wouldn't need to change either [21:07:51] nope, not if it doesn't want to allow different implementations of Preferences [21:08:18] so what GlobalPreferences ext needs to do with preferences? [21:08:20] the only place that's changed so far is for Special:Preferences in core [21:08:38] is legoktm here? [21:09:06] I got that it wants the list of all prefs, including extensions, but not sure what it is going to do with the list then [21:09:07] GlobalPreferences will provide its own PreferencesFactory, which will in turn produce either a LocalPreferences or a GlobalPreferences [21:09:18] oh goood, the current profilePreferences() method is 400 lines >_< [21:09:52] SMalyshev: override them from global settings, i assume [21:10:14] DanielK_WMDE: aha. So why two separate classes then? [21:10:36] SMalyshev: in my mind, two implementations of the same interface. [21:10:44] i think that's what samwilson is aiming at [21:10:49] yep, that's right [21:11:05] one will get preference values from the global database, and one from the local [21:11:13] the proposal is to override the whole HTMLForm that Special:Preferences uses, right? [21:11:32] I assume that PreferencesFactory will not create Preference objects, but some other classs. The old Preference class would stay and delegate to the new code. [21:11:46] and both do extra things with the preferences: e.g. add a 2nd local preference to every globalized preference, to indicate if it should be overwridden globally [21:11:54] ah ok so the ext would create a class that instead of loading prefs from local DB (as currently happens) would combine them with global DB using some magic [21:12:12] (hm, is overwridden doing both over writing and overriding?) [21:12:18] samwilson: side note: the mechanism for overriding services is currently somewhat broken. i have to look into that. [21:12:37] DanielK_WMDE: yes, i noticed that, but found a fix [21:12:51] this seems to work: https://phabricator.wikimedia.org/T153256#3698312 [21:13:03] samwilson: oh! you fixed it? or just worked around it? [21:13:05] what are the other callers of Preferences methods? [21:13:17] so should the clients know about this (e.g. different display of prefs)? [21:13:56] there are probably other callers of Preferences::getPreferences, because that's the most useful method [21:14:06] but this doesn't worry about them yet [21:14:23] becuase it's mainly concerned with constructing the preferencesForm [21:14:28] phpstorm gives me 3 calls in core, and one in MobileFrontend [21:14:34] may not be worth maintaining b/c [21:14:49] yes MobileFrontend was the only ext I could find too [21:15:05] yes, ApiOptions calls it [21:15:08] TimStarling: sorry I'm late, [21:15:28] samwilson: it would be good to have some kind of meta-information about each preference in the new interface. [21:16:10] what sort of information? [21:16:20] DanielK_WMDE: have an array of objects instead of array of arrays? [21:16:26] like how the prefs are linked to each other? [21:16:29] And be sure to l18n it (adding to DanielK_WMDE's comment) [21:16:34] MaxSem: that would be much better [21:16:43] like at least define the internal name, and a system message for a labele and one for a longer description. perhaps also give a data type. this could also be used to mark prefs as "can be global" or "this is taken from global prefs" etc [21:16:45] Disregard my last comment [21:17:08] DanielK_WMDE: I think that already exists in the giant arrays? [21:17:38] yes, the current preferecnes definitions are basically equal to Html form definitions [21:17:49] so they can take all the same options [21:18:15] ah, yes - type, label-message, default, and section. we can add more stuff there. [21:18:33] it does seem a bit like there's some conflating of the idea of what is a preference, a user option, etc. [21:18:41] legoktm: i just want to make sure we have a decent way to get any meta-info. i'd prefer objects, but that's not a strong preference [21:18:47] i.e. there's no separation between the preference definition without it being for a specific user [21:19:37] samwilson: yes, that's what i was aiming at. I want to be able to get declarations from PreferenceFactory, without actually getting Preferences for a given usuer. [21:19:48] from what I see in the code these "preferences" are essentially template for drawing preference UI, not data objects [21:19:49] samwilson: that's why I want to rename Preferences to something more clearly distinct from UserOptions:) [21:19:56] these declarations could inform the choice of what comes from global, and what does not [21:20:35] yes definitely, I think it'd be much better to be able to get them without a User [21:20:38] so ApiOptions currently calls Preferences::getPreferences() and then puts changes into User::setOption() [21:20:45] that feels like it'd be a next step after this [21:20:51] how would that work with global preferences? Would those methods just set the local override? [21:21:16] TimStarling: yes, GlobalPreferences then uses the get and set options hooks to do that [21:22:22] so the other possible way to tackle this, which would be a smaller change, is to introduce a new AfterGetPreferences hook. [21:22:24] related: T62856 - create an API module for GlobalPreferences extension [21:22:24] T62856: API module for GlobalPreferences extension - https://phabricator.wikimedia.org/T62856 [21:23:13] samwilson: but that only works if you know that no other extension uses that new hook to add preferences after GlobalPreferences looks at them [21:23:34] I think API is better than a hook... Hook seems to where we need many extensions to add stuff to common data item, but here it is not the case [21:23:49] DanielK_WMDE: yup. it'd have to be a "no one can use this to modify the preferences... except you" sort of hook [21:24:07] and yeah, then we can have a AfterAfterGetPreferennces hook [21:24:09] :) [21:24:20] you can achieve that by simply returning false from the hook handler [21:24:30] so was thinking about also creating a service for user options - it would take lots of code from User class [21:24:35] oh that's true [21:24:35] that will prevent any further registered handlers to get control [21:24:44] but not earlier ones [21:24:54] but I am always in favor of replacing static methods with a proper service, so please feel free :) [21:25:03] and then users of these handlers would come and complain that their extensions are broken :) [21:25:10] earlier ones wouoldn't be a problem, would they? [21:25:11] hehe yes, it feels the hook way is a bit more hacky [21:25:37] no, earlier ones who do the right thing would be ok. good point. [21:25:57] btw what happens if two extensions try to override a service? essentially the same issue, not? [21:26:13] yes. [21:26:18] SMalyshev: yep. but that's true of them all [21:26:21] even if they wrap instead of override [21:26:34] I think depending on hook order is a bad idea and just asks for trouble in the end [21:26:38] i supposed that cannot be helped [21:26:58] MaxSem: +1 to a service that replaces User::getOption()/User::setOptions() [21:27:18] I think we need a service how its handled im not to sure of [21:27:19] 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. [21:27:19] yes, hear hear [21:27:35] agree [21:27:47] I even had an earlyy WIP but shit you not - I have so many branches I couldn't find it XD [21:27:56] #info 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. [21:28:11] and this is a step towards having a bit of separation between preference definitions and them being tied to a user [21:29:00] we can have that split right away if the factory has two methods, one for getting an object wrapping prefs for a user, and another that returns pref definitions. [21:29:08] samwilson: err, what do you mean? preference definitions are tied to specific users. Some preferences are only available if you're in a specific group/permission [21:29:45] legoktm: good point. that would have to be part of the definition/declaration then [21:29:45] legoktm: yeah, but as MaxSem says, we could move towards being able to get a preference definition without knowing the user [21:30:08] but i'm not suggesting anything right now [21:30:46] samwilson: legoktm is pointing out that the list of available preferences depends on user groups. but still, getting a list of all declarations does no harm. especially if the declaration says what groups it applies to [21:30:48] so putting something like 'rights' => 'block' (or whatever) in the definition so it's user independent? [21:31:22] yes, a permission or a list of groups. permission is probably better, thank you [21:31:34] I think moving to preferences being objects would be a good idea, as part of that [21:31:53] agree [21:32:20] #info the list of available prefs depends on user rights/groups. preference declarations could reflect that. [21:32:44] #info separating the idea of opetions for a given user and declarations of preferences in general seems liek a good idea [21:32:50] so should the current patch try to do more in this way? [21:33:10] they're conceptually objects already, except they're HTMLFormField objects instead of something logical [21:33:38] #info defining a PrefernceFactory service is preferred over introducing a new hook point [21:34:06] legoktm: true, it doesn't seem a vast change to make them some sort of Preference object [21:34:20] or would it be called UserOption or something... [21:34:41] samwilson: do you mean objects for individual options (with value) or preferences (just the declaration), or do you mean an object wrapping all pref declarations / all options for a given user? [21:35:05] I think we should definitly have an object wrapping prefs for a given user. [21:35:48] DanielK_WMDE: the 2nd, I think. that a Preference object is independent of a user or context, and is the thing that extensions provide [21:35:59] We could have all four: UserOption, UserOptions, PreferenceDeclaration, PreferenceDeclarations [21:36:23] here's another fun way preferences depend on the user: the default "username" preference is the actual user name [21:36:25] yes, it seems the current terminology is that an option is tied to a user, but a preference need not be [21:36:43] TimStarling: good point! [21:36:56] TimStarling: seems like we'd want to allow callbacks for the default value, then [21:37:04] but I could imagine wanting to know about the username form field, without caring about the default? [21:37:33] #info the default "username" preference is the actual user name TimStarling: seems like we'd want to allow callbacks for the default value [21:37:35] sure [21:38:28] should the current patch try to introduce any of this? It seems it certainly moves in the right direction, but perhaps other changes are for later? [21:38:55] In reality 'username' shouldn't be a preference, it should just be registered as a form field in Special:Preferences since it only matters for viewing that page [21:39:38] yes, it's not even a preference (or option) is it? it's a "setting"? [21:39:49] it's 'type' => 'info' [21:40:26] yeah, and not accessible through UserLoadOptions etc. [21:41:03] samwilson: looking at the patch, i'm not quite sure what you are trying to do with the changes in Preferences [21:41:17] it's mostly about making it possible to subclass it [21:41:26] Yea... but why? [21:41:49] so globalprefs can have two subclasses, one for local and one for global, and then do whatever it needs to for each [21:42:05] it creates preferences, and changes preferencesForm [21:42:24] *sigh* 40 minutes into the meeting and I'm only just reading the actual gerrit change [21:42:29] in my mind, all that logic would move intoo the factory [21:42:29] e.g. for LocalPreferences it adds a new preference for every globalised preference [21:42:44] and I see that anomie objected to the PreferencesFactory idea [21:42:45] the actual Preference object should just be a plain value object [21:43:38] I'd just like to add that it would be cool if this new interface addresses the problem of https://phabricator.wikimedia.org/T58633 [21:43:43] samwilson: in other words, i see the combining of local and global info inside the factory, not the preference object [21:44:04] right now it very much isn't value object, so where does the logic go? [21:44:10] anomie suggested having a global preferences concept in core (or a preference source concept of which one implementation would be global preferences) [21:44:13] In short: Currently preference labels require parsing ahead of time, which is very wasteful for non-UI consuming, as well as because it means on every page view, just dumping the preferences involes the extension hooks parsing UI interface messages. [21:44:18] anomie said: "The idea of extensions overriding the PreferencesFactory seems somewhat scary, since multiple such extensions would stomp all over each other." [21:44:19] the trouble with that is that callers use e.g. Preferences::getPreferences — how would the factory take care of making that provide different preferences? [21:44:31] I don't see hwo that siutation is different from what we have with the auth framework... [21:45:08] samwilson: Preferences::getPreferences() calls MWServices::getInstance()->getPreferencesFactory()->newPreference(). [21:45:16] it becomes a stub for the service [21:45:32] I assume site config would dictate which factory is used, through service wiring or wgConf, not unlike how we have configurable objectcache/jobqueue/filebackend. [21:45:36] that is how a lot of other static factory methods have been migrated to services [21:45:59] I don't think having it in core will avoid multiple implementations, there is still going to be one override winning. [21:46:00] Krinkle: yes. [21:46:12] #info I assume site config would dictate which factory is used, through service wiring or wgConf, not unlike how we have configurable objectcache/jobqueue/filebackend. [21:46:39] if we have site config we don't need overriding the service, and uniqueness is explicit. Which I kinda like [21:46:41] For the main use case of iterating preferences, setting and getting preferences, there is presumably not a need to be aware of global preferences. Right? Or is there ambiguity that we need to resolve? [21:47:19] Basically the wgConf would inform which class MediaWikiServices::getPreferencesFactory() will end up instantiating or something like that. [21:47:45] what does it mean to set a preference? [21:47:53] SMalyshev: so the suggestion is to pick the implementation based on site config in ServiceWiring.php, insetad of letting the extension itself override the service? That sounds better, yes [21:48:05] Krinkle: the current globalprefs implementation is that it provides a different Preferences for either local or global, because it needs to do things with all prefs in both cases [21:48:06] if there are two preference stores then doesn't everything have to be aware of that? [21:48:13] DanielK_WMDE: yep [21:48:29] Krinkle: if the UI is oblivious about global preferences, then yet. I would like the UI to be aware, though [21:48:32] Right, I was just realising the same. There is actually an ambiguity. "getting" the effective preferences is straight forward (defaults + local, or custom stuff like global or otherwise), but setting presumably can no longer be done without awareness of the backend. [21:48:57] DanielK_WMDE: Yeah, but whomever overrides the backend would presumably also provide or modify the frontend. [21:49:01] TimStarling: only code that is responsible for writing, and reading from the database. Most code should call the User::getOption() which would have already taken care of resolving the local/global pref storage [21:49:14] Although I do quite like the idea that if you were to only provide a backend, that Special:Preferences still ends up showing and altering the local only. [21:49:22] samwilson: Krinkle and SMalyshev suggest you don't override the service, but make the original instantiator in the wiring file depend on config. we do that for a number of other things as well, and it prevents conflicts. i like the idea, what do you think? [21:49:55] so the config would say "use global prefs" or something? [21:49:58] Krinkle: i would expect that to be the case [21:50:11] how does that work for providing custom local preferences? [21:50:12] samwilson: $wgPreferencesFactoryClass='.. class name..' [21:50:16] Or some such. [21:50:19] (or a callback, etc.) [21:50:27] +1 [21:50:43] e.g. this: https://gerrit.wikimedia.org/r/#/c/370152/26/includes/GlobalPreferencesFactory.php [21:51:01] This is the convention we use for all other backends as well (memcached/objectcache, jobqueue, filebackend, rcfeed, and more) [21:51:13] all of which are configurable and extendable [21:51:45] You can of course make a wrapper class that delegates accordingly if you need, no problem. [21:51:54] Which seems to be what that gerrit diff shows. [21:52:06] samwilson: if ( $contextSource->getTitle()->isSpecial( 'GlobalPreferences' ) ) [21:52:07] oh ok, yes I think I see. [21:52:10] that'S scary! [21:52:21] currently in core, Preferences is only used by things that write preferences [21:52:22] you make the behavior of a service depend on which page it was called from?... [21:52:35] TimStarling: Yeah, so back to the question of the generic/stateless "set" pipeline. What will User::setOption() do? [21:52:35] *not* by things that read preferences, that is all User::getOption() [21:53:13] Will it default to local/internal for back-compat? Or will that be one of the things the new PreferenceFactory can decide? E.g. a "default"/"legacy" storeable alias. [21:53:25] samwilson: if there is two preference pages, each should use a different PreferenceFactory. [21:53:51] DanielK_WMDE: yep, that's the prob it's trying to solve. what's the better way? [21:54:15] This will cause a problem if an extension calls getOption, changes it, and then calls setOption. That may be expected to either be local-only or global, there is missing information about intent. And this is not limited to PHP code that we can fix and audit. The same applies to the API. There are many JS gadgets that read mw.user.options and then make an API call to save a preference. [21:54:27] it's probably a bit upsidedown, because the Special:Preferences uses Preferences::getForm to know what to display [21:55:21] Krinkle: if it calls getoption on a globalized pref, it get's the global value; if it calls setOption on the same, it saves it globally [21:55:31] DanielK_WMDE: I think we may need a Preferences interface separate from PreferenceStore interface. The latter would have multiple, and an API or special page would interact with that directly. But the generic 'Preferences' interface would be the one that is configurable and must provide a unified reality. [21:56:17] #info [14:53:25] samwilson: if there is two preference pages, each should use a different PreferenceFactory. [21:56:28] samwilson: How would you know? The values are scalar/primitive, so the value is hard to tell whether it "is" the global or local one. Presumably you'd compare and best guess? Or is there another proposal? [21:56:53] Krinkle: there's a separate marker which says whether the preference is global or local [21:56:54] Krinkle: no, the idea is that most client code wouldn't care -- they just want a option value [21:57:10] Note that when saving you don't know what the previous value was, so you can't compare whether it must've been local or global. [21:57:19] from the API perspective. [21:57:24] the only place that cares is the forms and [21:57:27] samwilson: yes, just turn it around. the respective spepcial page needs to (somehow) get the "right" PreferenceFactory. It doesn't have to be the global one. You control the construction of your special page, you can inject whatever PreferenceFactory you like. [21:57:28] oh yes I see what you mean [21:58:02] DanielK_WMDE: but it's the construction of the normal preferences page that can't be controlled from an extension [21:58:54] what would a global preferences concept in core look like? [21:59:03] we can't really answer that in 2 minutes I guess [21:59:14] samwilson: yes, the normal page will use the global factory instance. ah - but it shouldn't - it should use the "local only" version, yes? [21:59:19] one hour in, this is still about as clear as mud to me [21:59:21] I suppose depending on whether Special:Preferences is intended to the the canonical place or the local-core default one, we could make that configurable as well. wgPreferencePageClass :) [22:00:11] samwilson: so you'd really want *three* factories. local only, global only, and combined. the first two ore only for the UI. [22:00:45] samwilson: messing with the setup of SpecialPreferences is doable. I can help. [22:01:12] I still don't understand whether we're addressing technical debt here or adding to it [22:01:37] oh, we are over time... [22:01:51] TimStarling: that's a good question :) [22:02:02] I guess it depends on whether global is to be enshrined in core? [22:02:19] if it is, it seems like there might be other ways to do all of this? [22:02:38] Krinkle: did i understand correctly, you want this to work somewhat like Config: there's a global PreferenceFactory from which you can get different PreferenceStores from which you can get preferences for a given user? [22:02:40] it would be nicer if we didn't have to have special concepts in core for global prefs [22:02:53] Krinkle: that architecture makes sense to me, but i wonder if it's overkill [22:03:01] but I don't understand enough about inner workings of prefs to know whether it's possible [22:03:10] SMalyshev: I have no idea how that would work [22:03:25] samwilson: this mess is why we have bnee discussing this for years, and nothing came of it :) [22:03:36] anyway. i'll close the meeting in one minute. [22:03:45] my take-away: needs more thought. [22:03:47] DanielK_WMDE: :-) ah well [22:03:48] global preferences are exposed to the user, so every UI dealing with preferences now has to be aware of them [22:03:57] doesn't that make it a core concern? [22:04:04] TimStarling: how many UIs are there for setting preferences? [22:04:05] ugh... yeah I guess so [22:04:24] so what's the conclusion? [22:04:25] :P [22:04:35] RFC is still open [22:04:50] so, what do I need to do next to progress is? [22:04:53] *it [22:04:58] samwilson: do you feel you got useful feedback? [22:05:12] yes. but not total clarity! :) [22:05:16] samwilson: next steps: this goes back to "under discussion" [22:05:26] DanielK_WMDE: Not exactly, but that's possible too. I was merely thinking we'd probably need a generic preference-store interface that can have multiple implementations (default, and for this extension a global one). And separate from that a preference-reader or some such, which can be replaced by configuration to a custom one, so that e.g. in case of GlobalPreferences ext it can read from both stores and decide which one to surface. [22:05:29] samwilson: you update your experiments / spikes, and we talk about it again [22:05:43] ok :) will do. [22:06:05] and we need more than just the preferences forms to know about global-or-not? [22:06:07] great! sorry we couldn't provide total clarity ;) [22:06:23] samwilson: ask the mobile people... [22:06:24] T16950 is the root of all phab tasks? I guess it should be discussed there [22:06:24] T16950: Support global preferences - https://phabricator.wikimedia.org/T16950 [22:06:35] :) thank you all for your time. it is very useful. [22:06:44] ok. closing meeting... [22:06:50] #endmeeting [22:06:50] Meeting ended Wed Nov 1 22:06:50 2017 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4) [22:06:50] Minutes: https://tools.wmflabs.org/meetbot/wikimedia-office/2017/wikimedia-office.2017-11-01-21.01.html [22:06:50] Minutes (text): https://tools.wmflabs.org/meetbot/wikimedia-office/2017/wikimedia-office.2017-11-01-21.01.txt [22:06:50] Minutes (wiki): https://tools.wmflabs.org/meetbot/wikimedia-office/2017/wikimedia-office.2017-11-01-21.01.wiki [22:06:50] Log: https://tools.wmflabs.org/meetbot/wikimedia-office/2017/wikimedia-office.2017-11-01-21.01.log.html [22:06:53] thank you all!