[21:00:52] #startmeeting RFC meeting [21:00:52] Meeting started Wed Jun 27 21:00:52 2018 UTC and is due to finish in 60 minutes. The chair is kchapman. Information about MeetBot at http://wiki.debian.org/MeetBot. [21:00:52] Useful Commands: #action #agreed #help #info #idea #link #topic #startvote. [21:00:52] The meeting name has been set to 'rfc_meeting' [21:01:07] #topic Factoring page update logic out of WikiPage https://phabricator.wikimedia.org/T198075 [21:01:18] hi! who is here for the RFC meeting today? [21:01:48] This is about the overall architecture of the code we use to process page updates. It’s a major part of a comprehensive refactoring of the MediaWiki storage layer. [21:03:56] I'm here, anomie is presumably not [21:04:17] #link https://phabricator.wikimedia.org/T198075 [21:04:54] yea, anomie usually isn't around at this time. he already gave a lot of feedback on this, as you can see on the talk page [21:05:17] he's on vacation [21:05:56] oh right, silly me [21:06:16] anyone else around for the discussion of the page update refactoring? [21:07:23] TimStarling since nobody else seems to be around, i'll just pick yoour brain for a bit, if you don't mind [21:07:34] sure, I'm reading the wiki page at the moment [21:07:50] o/ [21:07:55] ok, let me know if there is anything that stands out to you [21:08:02] hey tgr! [21:11:10] tgr: what bit of the proposed architecture are you least happy with? [21:15:56] one of the things that I'm still not quite sure on is whether we need some kind of abstraction for a pair of revisions (old and new) that provides access to before-and-after states for stuff like isCountable()) [21:17:04] I don't think there is anything I'm unhappy with [21:17:12] are you aware of the code review I did for the task description of T157658? it may be obsolete now [21:17:13] T157658: Factor out a backend from EditPage - https://phabricator.wikimedia.org/T157658 [21:17:50] more of a hook survey [21:18:49] TimStarling: yes, I'm aware, though I didn't look at it recently. [21:18:51] there are some small details that are not mentioned on the RfC page like what the SlotRoleHandler depends on or how content types for slots are handled but the content of the MCR-PageUpdater wiki page seems as good as it gets to me [21:19:29] you haven't done any hook planning for this RFC, right? [21:19:48] This is the part that the spec refers to as EditController. I wasn't thinking abotu that part in detail in this context, except for deciding on what should *not* be in the PageUpdater, because it belongs to the edit contrroller [21:20:13] Namely, permission checks, rater limits, and edit filter hooks [21:20:39] anything related to the process of "editing" as opposed to the things that need to happen when a new revision is created [21:21:43] TimStarling: i think your survey is still mostly accurate and applicable. [21:21:57] the architecture proposal is for stuff one level below that [21:23:07] so I said "I propose introducing a new backend containing edit authorisation, automatic conflict merging, and the call through to WikiPage::doEditContent(), separated from the web UI" [21:23:11] that is your EditController [21:23:21] yep [21:23:45] In the flow chart on that wiki page, a preview makes a new revision, is that how it works now too? [21:24:19] Oh I see it doesn't get saveed [21:24:30] RoanKattouw: yes, and yes :) [21:25:23] TimStarling: I have looked at hook usage of the hooks that got moved out of WikiPage, but didn't do a comprehensive survey yet. Just ensured backwards compatibility for now. There is a ticket for replacing them: https://phabricator.wikimedia.org/T192307 [21:26:57] OK, great. This looks good to me at a high level, and I'm happy to hear that EditController is finally gonna happen after my false start in 2008 [21:27:06] I have to run now though, will check the logs later [21:28:10] thanks RoanKattouw! doesn't look like there is going to be a lot of hot discussion :) [21:28:59] at first it looks like a big wiki page [21:29:20] but it is a big project and there's not many details about each bit [21:30:12] broadly, you're introducing EditController and PageUpdater, this is fine [21:30:13] yes, it's a very high level view. For some bits, there are more detailsd plans or experiments. [21:30:41] the idea was to have a borad map of what architecture we are aiming for, and then tackle the individual bits [21:31:31] PageUpdater is in core now. But mostly contains codde that was just moved out of WikiPage, and still needs further refactoring [21:31:57] is this all blocking MCR? [21:32:47] Not all of it. And most importantly, most of it doesn't block the parts of MCR needed for SDC. [21:32:53] I'm looking through a the moment to try and see if each proposed class has a clear purpose to me. I'm somewhat concerned in general about class growth to a point where a class' responsible is too small to quantify when getting on-boarded with the code. [21:33:01] However, what I see so far makes sense and seems sensible. [21:33:13] I do wonder what 'PageStatusUpdate' is. Couldn't find a definition for it on the page only a mention. [21:34:23] mmm [21:34:57] Krinkle: PageStatusUpdate is just the idea that it meay be handy to have an object that has the old and the new "status" of the page, where "status" is stuff like "isRedirect" and "isCountable". [21:35:18] DanielK_WMDE_: the 'Further refactoring' section reads very easily. Quite well done :) Seems like these one-liners could go straight to each @class block later :) [21:35:39] will there be namespaces? you know we have PSR-4 now [21:35:41] TimStarling: here's a half finished code experiment / spike that I used to help me thing while I wrote this: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/440882 [21:36:11] ok that answers that question [21:36:20] TimStarling: I put all new classes into namespaces. How well these are thought out is another question... [21:36:35] Krinkle: thank you, that's encouraging! [21:37:25] TimStarling: one of the parts needed for SDC, which has been speced out already, is SlotRoleHandler/SlotRoleRegistry: https://www.mediawiki.org/wiki/User:Daniel_Kinzler_(WMDE)/MCR-SlotRoleHandler [21:37:59] That's basically a zoom-in on one component. [21:38:01] Can you elaborate a bit about "unsaved RevisionRecord", that seems somewhat surprising. I'd expect such object to never exist.. But maybe it needs to? E.g. passing a Revision to RevisionStore can get back something that can help get a RevisionRecord object, right? [21:38:37] Krinkle: it exists even in the old code, though it'sa bit hidden. When rendering a preview, the parser gets a "fake revision". [21:38:52] Right, but does it need a RevisionRecord? [21:39:26] Krinkle: RevisionRecord replaces revision. Also, RevisionStore talkes a RevisionRecord for saving. That needs to be constructed at some point, from PST content [21:39:50] so it's basically: user input -> pst -> new revision -> save revision -> update page table [21:39:56] Right. [21:40:10] I was expecting Record to represent the row existing, not the draft value to submit. [21:40:26] But if it can represent the draft, that means is has no getId()? [21:40:30] It would be possible to use an incompatible interface for unsaved revisions. that would be cleaner, but inconvenient. [21:41:07] Hm.. getId() is nullable in master. [21:41:08] okay. [21:41:16] Krinkle: there are distinct sublcasses of RevisionRecord, if yoou want to make the distinction: MutableRevisionRecord (typically unsaved), RevisionStorerecord (saved) and RevisionArchiveRecord (archived) [21:41:24] I can see how that might cause a problem at some point. [21:41:44] per the substitution principle. [21:41:51] Yes, I'm not very happy about that aspect. It's a compromize. [21:43:04] Ah, these exist already, cool. [21:43:12] Krinkle: yes, the base class gives fairly weak guarantees to satisfy LSP. RevisionStoreRecord gives stronger guarantees. [21:43:33] Yeah, would it be possible to have the base class not get a getId() method? [21:43:41] as in, not part of its definition at all? [21:43:50] Or make MutableRevisionRecord could potentially be made not-a-RevisionRecord. [21:43:56] Righit [21:44:07] but that'S really... the spepc we discussed, implemented and merged 9 months ago :) [21:44:09] Anyway, I understand now. That's fine for now. [21:45:35] Krinkle: you are a meetbot maintainer now, btw. but i never figured out how to controll it from the cli [21:46:49] thx [21:46:56] I couldn't figure out how to be a maintainer, but root is a maintainer so "sudo become meetbot" works [21:47:18] oh, i got in there, but i had no idea what to do then [21:47:42] did you read https://wikitech.wikimedia.org/wiki/Tool:Meetbot ? [21:48:30] my need was to give myself "admin" privileges so i could close a meetingg that was created and left open by a troll. [21:48:43] just 13 minutes left! Remember the info tag is your friend :) [21:49:37] TimStarling: had a brief look, but doesn't seem to conver the kind of config i was looking for. [21:49:56] can I say we agreed that EditController is a fine concept and you can go ahead with making it? [21:50:06] sure :) [21:50:25] i think that's the least controversial part of the proposal ;) [21:50:47] #agreed EditController concept is approved [21:50:57] According to a breakdown we did a while ago, it'll probably be brad who will take that on [21:51:03] \o/ [21:55:19] end meeting? [21:56:42] am i a chair? [21:56:43] #endmeeting [21:56:43] Meeting ended Wed Jun 27 21:56:43 2018 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4) [21:56:43] Minutes: https://tools.wmflabs.org/meetbot/wikimedia-office/2018/wikimedia-office.2018-06-27-21.00.html [21:56:43] Minutes (text): https://tools.wmflabs.org/meetbot/wikimedia-office/2018/wikimedia-office.2018-06-27-21.00.txt [21:56:43] Minutes (wiki): https://tools.wmflabs.org/meetbot/wikimedia-office/2018/wikimedia-office.2018-06-27-21.00.wiki [21:56:44] Log: https://tools.wmflabs.org/meetbot/wikimedia-office/2018/wikimedia-office.2018-06-27-21.00.log.html [21:56:47] ah :) [21:56:49] you are not :) [21:56:50] thanks kate :) [21:57:06] ok, thanks for the feedback!