User:Daniel Kinzler (WMDE)/MCR-PO

While refactoring the MediaWiki storage layer for MCR, questions have arisen regarding the handling of multiple slots in ParserOutput.

This document is intended for discussing the requirements and constraints, and the possible solutions.

Please comment and discuss inline.

Proposal, March 9

While working on PageUpdater::getSecondaryDataUpdates(), I realized that we can't just add up DataUpdates returned for each slot, because they will override each other. In particular, we must not generate a LinkUpdate for each slot, otherwise we'll only have the links from the last slot in the DB. This problem exists not matter whether we call Content::getSecondaryDataUpdates() on per-slot ParserOutput, or on a combined ParserOutput.

This means we have to change the way getSecondaryDataUpdates is called, which I wanted to avoid since it's hard: we can't change the message signature without breaking extensions that override Content::getSecondaryDataUpdates. According to codesearch this is Gadgets, Wikidata, and Newsletter. We could fix them, but not in a compatible way - we would rely on always running the version of the extension that exactly matches core.

While I investigated getSecondaryDataUpdates(), I realized that the ParserOutput we pass in is only used by LinksUpdate, not by other kinds of DataUpdates so far. I still believe we need that option, though.

To move forward, I propose for the baseline, main-slot-only patch:

• remove PreparedEdit::getParserOutput( $slot ) for now.
• PreparedEdit::getCombinedParserOutput can be renamed to getStandardViewParserOutput if that helps us moving forward, but I think that name is less clear.
• use the main slot's getSecondaryDataUpdates() directly for now - since we only have one slot, that is sufficient.

I hope this will unblock the initial PageUpdater patch.

Then, in the follow-up patch(es) that make PageUpdater multi-slot capable:

• introduce PreparedEdit::getParserOutput( $slot ), but generate the PO on-demand (perhaps using a callback). The conceptual distinction between the main slot's output and the combined/effective output should be kept, even if we use the same PO for that (for now or forever).
• In PageUpdater::getSecondaryDataUpdates, construct LinksUpdate on the combined PO, and no longer construct a LinkskUpdate in AbstractContent::getSecondaryDataUpdates.
• Introduce a slot-aware version of Content::getSecondaryDataUpdates (by changing the signature or adding a new method). While I still think a Content object's behavior should not depend on the slot it is used in, I realized that DataUpdates will need to know what slot they refer to, so it can avoid overriding info from other slots.
• replace calls to Content::getSecondaryDataUpdates() with calls to PageUpdater::getSecondaryDataUpdates(). Content::getSecondaryDataUpdates() alone can no longer provide all necessary updates, since they may depend on other slots.

Maving on to higher le4vel functionality, the question of having a PageTypeHandler or having SlotRoleHandlers arises again. I can see use having both, but I'd like to start with the PageTypeHandler. I don't see how we can do what we need to do with just a set of SlotRoleHandlers. If Anomie wants to got that way, I suggest he writes some code we can discuss. The proposal should cover: which slots are allowed or required on a given page, deciding on the editing interface to use, doing conflict resolution (automatic and interactive), composing and serving the standard view, updating the cached standard view after an edit, doing data updates, and providing any necessary additions to actions like move, purge, rollback, etc. In my mind, SlotRoleHandlers may play are role in all of that, but I don't even see where the code would live that uses them, if not in some kind of PageTypeHandler. — Preceding unsigned comment added by Daniel Kinzler (WMDE) (talk • contribs) 10:06, 9 March 2018 (UTC)

I note that a subclass can add optional parameters. If whatever changes need to happen to getSecondaryDataUpdates() can be done by adding additional parameters with default values to the end of the argument list, the extension code using that can run with both new and older MediaWiki. But new MediaWiki couldn't use an old version of the extension.
PageUpdater::getSecondaryDataUpdates() is a reasonable idea. PreparedEdit::getParserOutput( $slot ) may be YAGNI, it could wait until something actually does need it.
No, I'm not going to implement code for all of MCR as a "proposal" for you. Yes, there will be code somewhere that handles calling all the SlotRoleHandlers, the difference from your PageTypeHandler is that it won't be different subclasses per page doing different things. Anomie (talk) 15:56, 9 March 2018 (UTC)

Note that codesearch only finds extensions on gerrit, and not all extensions are there (e.g. Semantic MediaWiki is not). Also people can have private extensions etc. It would be nice to not break those, and modifying interfaces usually breaks everything. As long as only new parameters are added, it would be possible to have a new interface (RoleAwareContent?) extend Content and add them. Alternatively, just move the role-dependent logic to some new Slot or Role class. (OTOH, it's maybe not worth the effort since B/C is impossible anyway when non-MCR-aware code assumes that it does not share the page with other content objects, and change links tables etc. in ways that might result in other changes getting lost.)
Anyway, what's the long term goal for data updates? Having per-slot dependencies where every slot generates its own dependencies, keyed by page + role, and have the code querying the link tables make sense of that; or combining slot-level dependencies into page-level dependencies and keeping most of MediaWiki unaware of the lower levels? In the second case, this is probably a good time to disentangle the concept of dependencies from arbitrary data updater callbacks controlled by the content, and define some new value-object-like interface for the former (where merging is a meaningful operation). --Tgr (WMF) (talk) 03:29, 13 March 2018 (UTC)

Adding a second interface would be an option, I currently think the new method should just live in ContentHandler (this is where this logic should have been in the first place). Note however that the logic in these methods should, in my mind, not be slot-specific in anyway - but it does need the slot, for tracking:

The long-term goal is to have per-slot tracking, at least for purging. The short-term goal is to have combined tracking for all info for a page, so we don't have to change all the db tables right away, and can perhaps wait for dependency tracking that doesn't depend on SQL.

You are correct about dis-entangling dependency tracking from other kinds of secondary data tracking. I have discussed plans for this with Marko, an (outdated) brain dump can be found at User:Daniel_Kinzler_(WMDE)/DependencyEngine. For now, I intend to base LinksUpdate on the combined output (this is neccessary so LinksUpdate for one slot doesn't override LinksUpdate for another slot), while keeping other content-model specific updates separate (works for now, as long as we don't have two slots emitting the same extra updates, one overriding the other on the DB). -- Daniel Kinzler (WMDE) (talk) 16:52, 13 March 2018 (UTC)

Subclasses can add parameters (though this behavior seems to have changed with recent PHP versions), but we'd need to add a parameter to the interface, without touching the classes that implement it. That doesn't work. We could only introduce a new interface, as Tgr suggested.

Note that I didn't ask you to "implement all of MCR". My proposal to introduce a PageTypeHandler is based on the realization that all the many things that I listed need to combine behavior from multiple slots, and what kind of combination is sensible really depends on the kind of page. I see no sensible way to leave that to per-slot logic. If you are opposed to introducing a PageTypeHandler, then make an alternative proposal that explains how all these things can be done without a central coordinating entity.

Luckily this doesn't have to be decided for a baseline implementation that would satisfy the minimal needs of SDoC. On the other hand, it would be nice to agree on the architecture we want to shoot for, to avoid introducing things that we'll later not need. I'm still thinking we may not need SlotHandler, though I'm open to the idea of having them. I just don't see how they can cover all we need. -- Daniel Kinzler (WMDE) (talk) 16:52, 13 March 2018 (UTC)

Why "without touching the classes that implement it"? Also, https://3v4l.org/Vfdiq and https://3v4l.org/clYji don't seem to show any relevant behavior changes.
I did make an alternative proposal, on this very page. I don't see where you've listed anything at all that requires knowledge of multiple slots in order to do merging. Certainly there isn't anything in SDC that needs it, since with only two slots the cases (if not the exact code) are basically equivalent.
I agree that knowing where we're going before we implement too much is a good idea. I don't see why you think SlotHandler won't cover all we need. Anomie (talk) 13:27, 14 March 2018 (UTC)

Discussion Reset, March 5

I think the long discussion below served well to bring out what points are contentious. I'd like to re-iterate them here, to avoid a fragmented discussion in nested threads.

• Is it a requirement to have full tracking in links tables (SecondaryDataUpdates) for all slots, regardless of what is shown in the standard view (i.e. on /wiki/Foo without extra parameters)? That is, should e.g. an a search for external links turn up external links in a non-main slot, even if that slot isn't visible in the default view?
  • Daniel thinks: yes, this is a requirement, since such reverse searches are important maintenance tools to find usages of templates and images and to combat link spam. Therefore, this should at least be the default behavior, and should not require any effort for every new slot. It should perhaps even be hard-wired. Note however that this currently causes the ParserOutput for the default view to be purged and re-rendered for edits that may not actually affect it. This can be solved with more fine grained dependency tracking for rendered content, which is in planning.
  • Roan thinks: yes, these things should be tracked, but it'd be good to present them in a less confusing way in cases where the default view doesn't show them.
  • Gergő: it should be possible for non-main, non-visible slots to interact with link tables. I'm not sure we have a use case for this right now, but there is no reason to prevent extensions from figuring out uses for it.
  • Anomie: It should possible, as it is in either of the proposals below. It shouldn't necessarily be forced as Daniel wants.
  • Daniel replies: I'm not saying we MUST make sure that this is enforced, but I do believe that writing all secondary data exposed by some content should be the default. The Content object exposes this data with the expectation that it is actually stored. I don't believe we should break that assumption for non-main slots, even if the non-main slot is invisible on the standard view (which IMHO will be rare, but no matter). When an admin checks the usage of an image before deleting it, they expect to see all usages. When a spam fighter searches for bad external links, they expect to find all occurrences. When a Content object exposes a geo-coordinate as a page-property, it's expected to be able to find the respective page via GeoSearch. Requiring an extra effort for this if the content is not in the main slot breaks assumptions.
    • If content is visible, the data can probably be added to the combined ParserOutput easily enough when the content gets added. If content isn't visible at all, is the image really used etc.? I note we already don't record uses of images, external links, and so on when it's in the not-taken branch of an {{#if:}}, or inside a <!-- comment -->, or in an unused template parameter, or in <includeonly> or <noinclude> in cases where those hide comments. Anomie (talk) 15:32, 8 March 2018 (UTC)
      • It's true that we are currently inconsistent about tracking usage. I consider this mostly a bug. It's true that for some usages (namely, purging), it's useful to only track usages that have a visible effect. For other use cases, such as cleaning up after a rename or a deletion, "invisible" usages should also be tracked.
      • I believe this we should track all usages, plus ideally the information whether the usage is "visible", for purging - even better, track which rendering depends on that data. This is what the new dependency tracking infrastructure is intended to allow us to do.
      • So, I suppose we disagree on whether it should be left to content or slot to make an extra effort to always expose info to secondary tables, or whether it should happen in the same way always, regardless of whether the content is in the main slot or another slot. -- Daniel Kinzler (WMDE) (talk) 14:28, 9 March 2018 (UTC)
        • I note that tracking all usages is impossible when the actual thing used can wind up depending on template parameters or other externalities. Anomie (talk) 15:56, 9 March 2018 (UTC)
          • True. Let's do the best we can. -- Daniel Kinzler (WMDE) (talk) 17:16, 9 March 2018 (UTC)
            • OTOH, setting an impossible goal and, since it's impossible, failing to do it very well may be worse than setting an attainable goal and doing it well. Anomie (talk) 13:41, 12 March 2018 (UTC)
• Is it a requirement to be able to show the content of non-main views in the standard view, without the need to explicitly request this in the main slot's content?
  • Daniel thinks: yes, this is a requirement. SDC is a prime example for this need.
  • Roan thinks: yes, we should default to not hiding content, and only hide it when there's a reason to. The content model/handler should also be able to have specific behaviors for displaying non-main content, e.g. templates should present TemplateData in a human-readable way in the main view.
  • Gergő: yes, extensions need to be able to add non-main slots and show their content in the standard view, even when they don't control the content type of the main slot (and so it has no knowledge of the extension). I guess it would also be possible to require that the main slot explicitly request it, but allow other slots to cause the main slot to make such a request via some hook mechanism. But that seems pointlessly cumbersome.
  • Anomie: This is a silly question. No one disagrees on this being possible, the discussion is all about how it happens.
  • Daniel replies: this seems to be consensus now, but that was not always the case, so I wanted to make sure.
    • I don't recall it not being the case at any point. But whatever. Anomie (talk) 15:32, 8 March 2018 (UTC)
• Is subclassing ContentHandler a good mechanism for pulling in content from non-main slots, and attaching it to the main slot?
  • Daniel thinks: no, this is a bad idea. This means we'd be recording a different model ID in the content table, making e.g. wikitext on file description pages incompatible with other wikitext, and forcing us to convert all existing wikitext content on file description pages to the new pseudo-model. Also, it's a conflation of unrelated concepts: the different roles content may play has nothing to do with the content model. Wikitext especially may be used for a variety of purposes. Conversely, the main slot may have a variety of models.
  • Gergő: Agreed, but I imagine if we went this way we had some kind of RoleHandler instead / on top of the ContentHandler. We might want to have that anyway.
    • DK: Yes, we'll need a SlotRoleHandler, or a PageTypeHandler, or both.
  • Anomie: Meh.
• Content::getSecondaryDataUpdates takes a ParserOutput object - can we assume that a Content object of one kind can deal with ParserOutput generated from Content of another kind, or combined from several sources? Is that requirement a breaking change?
  • Daniel thinks: this is dubious at best - FooContent::getSecondaryDataUpdates() currently can assume to get a ParserOutput returned by FooContent::getParserOutput(), and not from XyzzyContent::getParserOutput(). And there is another problem: if we call getSecondaryDataUpdates() on the content of each slot, re-using the same combined ParserOutput, we will end up with one LinksUpdate object for each slot, but all of them containing the same ParserOutput, rendering them completely redundant. We'd have to take away control over the updates from the Content object (breaking Wikibase and others), or somehow only generate a LinksUpdate for the main slot. That seems odd - the DataUpdates geturned by getSecondaryDataUpdates() should represent whatever secondary information is to be recorded about the content. It shouldn't depend on the slot role. ContentHandlers generally shouldn't know about specific roles.
  • Anomie: It is possible to define the semantics for non-main slots when we add them, instead of assuming everything rigidly has to match the existing non-MCR semantics without any modification. BTW, at some point we're going to have to figure out how code can determine which slots are valid for a page and which content models are valid for each slot. Both of those provide a better place to avoid having an unready content type in a non-main slot.
  • Daniel replies: it's possible to treat content of non-main slots as "not really on the page" per default, requiring extra steps for it to behave as if it's "really" on the page, but I believe it's a bad idea to break assumptions and workflows in this way, see above. And yes, we will indeed need some thing that decides which slots are supported or required on a certain kind of page, and how they relate to the main slot. This defines the page type, which in turn is generally determined by the namespace. This is one reason we need some kind of PageTypeHandler.
    • How can there be assumptions and workflows to break when non-main slots don't even exist yet? As for your "This defines the page type" and so on, none of that actually follows as a requirement. We could as well (and IMO, better) leave it up to the SlotHandler (or whatever) to decide, much like how ContentHandler currently decides whether the model is allowed on a page rather than having a "PageTypeHandler" to do that. Anomie (talk) 15:32, 8 March 2018 (UTC)
      • Ah - you assume that new slots are generally for new kinds of content, and new functionality. This is the case for SDoC, but this is not the case for most other use cases we target. Mostly, we either integrate stuff that is now on subpages, or we extract stuff that is now embedded. If tracking behavior changed when moving existing kinds of content to a slot, that would break workflows. -- Daniel Kinzler (WMDE) (talk) 14:28, 9 March 2018 (UTC)
        • If we're duplicating existing functionality, it's probably naturally going to follow that the links tables track the same uses as the existing functionality. And if not and we want to preserve the old behavior, the SlotHandler can still do that. We don't need it forced by a PageTypeHandler. Anomie (talk) 15:56, 9 March 2018 (UTC)
          • It doesn't need to be forced. It should just be the default, and should not need extra effort. In particular, tracking should (per default) behave the same regardless of whether content is in the main slot of a different slot. -- Daniel Kinzler (WMDE) (talk) 17:16, 9 March 2018 (UTC)
            • That's a lot of "shoulds" that are begging their questions. I note it tends to be more difficult to override "default is to do everything" than to have the default be minimal and adding things on to it. Anomie (talk) 13:41, 12 March 2018 (UTC)
• Is it a requirement for code that accesses PreparedEdit to be able to explicitly access "output as shown in the standard view" and "contentOutput of slot x only" [edited to replace "content" with "output"]? Is it alternatively acceptable to have the main slot's output to serve as the combined output, and no "main slot only" output to be available to hook handlers?
  • Daniel thinks: yes, extensions like SpamBlacklist and AbuseFilter should have this option. It should be up to the filter logic whether it works on per-slot output or combined output, or both.
  • Roan thinks: yes, you need to be able to write an AbuseFilter that looks specifically at edits to TemplateStyles for example. Also note that "content of slot x only" is pre-parse (wikitext, XML, CSS or something) while "output as shown in the standard view" is HTML. Right now AbuseFilter gives you the old and new wikitext, as well as the new HTML.
    • DK: A, sorry, my mistake. I meant ask about the output for each slot individually.
  • Gergő: content definitely needs to be available on a per-slot level. I can't think of a use case for per-slot output, but this is the interface new extensions will use for a long time so it's better to be flexible.
    • DK: I meant the output per slot, sorry. Note that output isn't just HTML, it's also the list of external links, etc - AbuseFilter uses this a lot.
  • Anomie: No. If some code actually needs a ParserOutput for a non-main slot, it can obtain that from the Content object. Otherwise there's no point in wasting time generating all these ParserOutput objects that are unlikely to be used. Regarding AbuseFilter, it will need changes to handle MCR anyway, so the fact that it currently has "HTML output" doesn't mean it needs HTML output for every slot.
  • Daniel replies: Hook handlers that need ParserOutput for non-main slots can *not* just get that from the Content object, because that would mean re-parsign and re-rendering. Caching inside the Content object isn't feasible because of the presence of different ParserOptions. Caching ParserOutput per ParserOuptions is exactly why we have prepareContentForEdit() and PreparedEdit(). And yes, AbuseFilter will still need a PO - not for the HTML, but for the list of external links. This need, together with the way getSecondaryDataUpdate() works, is the reason for adding per-slot ParserOutputs to PreparedEdit. WE need them there, no matter how output composition works. PreparedEdit could create these on-the fly, when needed. But with the need to call getSecondaryDataUpdate() for each, we always need all of them anyway.
    • "Caching inside the Content object isn't feasible because of the presence of different ParserOptions" is bogus, ParserCache handles that sort of thing fine.
      "Caching ParserOutput per ParserOuptions is exactly why we have prepareContentForEdit() and PreparedEdit()" is also wrong, since neither of those actually writes the parser cache. We have prepareContentForEdit() to do pre-save transforms and parsing.
      "And yes, AbuseFilter will still need a PO" I never said it wouldn't need a PO, I said it wouldn't need one for every individual slot. Which your claim about external links does nothing to refute, while I've refuted your getSecondaryDataUpdate() claims above already. Anomie (talk) 15:32, 8 March 2018 (UTC)
      • Ok, caching of ParserOutput inside Content objects is possible, it's just horribly complicated and brittle. The convoluted way that PreparedEdit signatures and ParserCache keys work is evidence of that.
        Yes, PreparedEdit exists only to avoid re-parsing during the process of saving - that is, caching of a ParserOutput, depending on ParserOptions and related info (the signature). Doing aching via the ParserCache would be an alternative to that. I considered switching to that approach, but that would need structural changes to all code currently using PreparedEdit, especially hook handlers.
        You think that it's enough to give extensions that use PreparedEdit access to a combined PO. I disagree, and if I understand the above comments correctly, I'm not alone with that. But we can defer constructing the per-slot POs until actually needed. -- Daniel Kinzler (WMDE) (talk) 14:28, 9 March 2018 (UTC)
        • No, it's not horribly complicated and brittle. Extend get() and save() to take an optional $role and pass that down into getParserOutputKey() and getOptionsKey() and you're pretty much done. If you don't want to pass $role into Content the caching could be done in the SlotRoleHandler instead, that's an irrelevant detail.
          Regarding PreparedEdit, you seem to be confused as to the different types of caching. PreparedEdit is the object held in an in-process cache during saving, storing a ParserOutput and some other data useful during the saving process. It doesn't do anything about different ParserOptions as you claimed.
          Extensions that use PreparedEdit have access to PreparedEdit. That includes Content objects for each slot, from which a PO for the slot can be gotten if necessary. Having a utility method in PreparedEdit to do that (and cache it inside PreparedEdit) seems reasonable, if an extension turns out to actually need a per-slot PO. Anomie (talk) 15:56, 9 March 2018 (UTC)
          • I was referring to the logic in prepareContentForEdit() that decides whether mPreparedEdit can be re-used or not. But you are right, that doesn't depend on ParserOptions but on other options. You seem to imply that Content::getParserOuptput shoudl use the ParserCache. That would introduce a circular dependency. Also, I'd rather like to avoid moving more business logic into Content, it was a mistake to put it there in the first place. What I was referring to as brittle and convoluted is the logic in getParserOutputKey and getOptionsKey. It'd rather not add to that.
          • Anyway, I think we have a way forward now: Have a utility method in PreparedEdit for getting per-slot POs on demand. I'd say it's already there, it's called getParserOutput( $slot ). I guess you'd want that to generated the PO on the fly, instead of pre-generating it - is that correct? -- Daniel Kinzler (WMDE) (talk) 17:16, 9 March 2018 (UTC)
            • It wouldn't introduce a circular dependency any more than other things that do internal caching introduce circular dependencies.
              I note a lot of bad design decisions over the years seem to have been made because someone thought existing code was to "brittle and convoluted" to work with.
              Yes, if the slot POs aren't generated if they're not needed then I think we're good. Anomie (talk) 13:41, 12 March 2018 (UTC)
• Does code that fetches ParserOutput from the ParserCache assume that to be the complete output that is shown in a default view?
  • Daniel thinks: yes, code that gets a ParserOutput from the ParserCache generally assumes that this is the HTML as shown in a default view.
  • Gergő: the alternative are to change how the ParserCache works (so it can return multiple ParserOutputs). I don't think I've strong arguments for one or the other.
  • Anomie: Another silly question. No one disagrees that the ParserOutput you get from ParserCache is what is needed to produce the default view. But that doesn't mean there can't be post-cache transformations applied by ParserOutput::getText(), and it doesn't mean the ParserOutput has to contain sub-ParserOutputs for every slot.
  • Daniel replies: Just making sure we actually need this. Also, doing post-cache transformations in ParserOutput::getText() is what drove the "nested ParserOutput" idea in the first place. How else would the main PO have access to other POs in getText()? Please don't say "global state".
    • "How else would the main PO have access to other POs in getText()" is begging the question that the "main" PO needs access to "other POs" at all. Anomie (talk) 15:32, 8 March 2018 (UTC)
      • You suggested post-cache transformations applied by ParserOutput::getText(). That transform is what would attach output from other slots to the PO used for the standard view. So it needs access to access to the POs of pother slots. ParserOutput is a serializable value object. How do you suggest it gets access to these other POs, so it can apply a post-cache transformation in getText()?
      • I suspect the answer is "from the parser cache". But that means 1) we need to put them into the ParserCache before we know whether we need them and 2) ParserOutput::getText() has to depend on global state, which I would definitely want to avoid. -- Daniel Kinzler (WMDE) (talk) 14:28, 9 March 2018 (UTC)
        • I don't know why you keep assuming that the only way to get data from a slot is to generate a PO for it. I also note that doing the composition as a post-cache transform is only one possibility, which may not actually make sense if it turns out getting the needed data there is problematic.
          But if we did do composition as a post-cache transform and a SlotRoleHandler did need a PO to do its work, it could indeed fetch it from the ParserCache for that slot as I mentioned somewhere above. But there's no need to preemptively cache it, on miss we could easily generate it (and put it into the cache). Nor would ParserOutput::getText() have to depend on global state, although it would have to have the ParserCache service and maybe other data injected at creation. Anomie (talk) 15:56, 9 March 2018 (UTC)
          • Injecting stuff doesn't work for serializable objects. That was my point. -- Daniel Kinzler (WMDE) (talk) 17:16, 9 March 2018 (UTC)
            • It does if the injected stuff is not included in the serialized representation and the deserializer knows how to re-inject it on re-creation. I had thought that was obvious. Anomie (talk) 13:41, 12 March 2018 (UTC)
              • So how does that re-injecting work without access to global state? If it does depend on global state, it's either inconsistent with the original injection, or the original injection is pointless, because it cannot be different from global state. -- Daniel Kinzler (WMDE) (talk) 16:54, 13 March 2018 (UTC)
                • When ParserCache does unserialize(), it then calls methods on the newly-deserialized PO object to inject the necessary state (i.e. DI services) to complete the deserialization process. Again, I had thought that would be obvious.
                  I can't make sense of your other objection, what does it matter that the deserialized object has a different handle to the ParserCache service than the original object did when it's still a ParserCache? Anomie (talk) 13:27, 14 March 2018 (UTC)

A few points I'd like to clarify:

• I'd like to postpone the decision on how exactly output composition happens for now. I just want to assert that it does need to happen, without the need to put anything into wikitext.
  • Unless you've removed a bunch of code from your patches since I last looked, though, you're building in code that's only going to be needed for your method of composition. Anomie (talk) 14:36, 6 March 2018 (UTC)
    • I can only assume that you mean the per-slot ParserOutputs in PreparedEdit. They exist for two reasons: for use with getSecondaryDataUpdates so we actually save all the info from all the slots in the links tables, and to allow hooks handlers for AbuseFilter and friends to check them individually. The latter is not needed until the relevant extensions become MCR-aware, but per-slot rules seem likely, just like we have per-namespace rules.
    • Calling getSecondaryDataUpdates() only for the main slot, or calling it for each slot on a combined PO, do not seem acceptable to me for the reasons laid out above. The need to expose per-slot POs to hook handlers seems obvious. On-demand creation of the POs in PreparedEdit is feasible, but seems pointless because of the need for DataUpdates.
    • This discussion however made me realize a need for further investigation: we can't have a LinksUpdate for each slot, because these will undo each other's effect when executed. The way getSecondaryDataUpdates() works will have to be modified in some way. I'm still convinced we need the PO of each slot in order to generate all DataUpdates - the need for tracking usages does not depend on what slot Content resides in. Usage is usage, and backlink is backlink. -- Daniel Kinzler (WMDE) (talk) 16:23, 7 March 2018 (UTC)
      • I already replied to all those assertions above. Anomie (talk) 15:32, 8 March 2018 (UTC)
• While I prefer the PageOutputComposer approach ("one composer per page"), there is no need to commit to it at this point, and I'm quite open to discuss per-slot-type handlers for composition. I'm am opposed to binding the composition behavior to content models. Also, I want to avoid relying on hooks, in favor of defining proper interfaces and type safe registries.
• The introduction of some kind of PageTypeHandler (which could be or provide a PageOutputComposer) seems the most sensible approach when refactoring Article, FilePage, FileWikiPage. This refactoring will be probably be neccessary for full MCR support.
• I'd like us to have a plan for avoiding re-rendering all slots when one slot changes. One idea is nested ParserOutput, as outlined below. But we don't need to decide on that here and now, and I'm quite open to alternatives.
  • Chances are in many cases we'll have to anyway. SDC seems likely to be an exception, not the rule, and that only if you omit the possibility for editors to make any use of the structured data within the wikitext. Anomie (talk) 14:36, 6 March 2018 (UTC)
• I'd like to avoid touching the way ParserCache works
• I'd like to avoid changing the signature of getSecondaryDataUpdates().
• I'd like to change the behavior of doEditUpdates as little as possible for now
  • I think that these three points are likely to make the project harder to do well, not easier. Anomie (talk) 14:36, 6 March 2018 (UTC)
    • I guess we'll just have to disagree on that, then. I have tried messing with all that. This is the forth iteration of this refactoring. I have become really careful of falling down rabbit holes, trying to fix all the things. I want to touch as few classes as possible, change as few signatures as possible. This first refactoring step is already way bigger than I'd like. -- Daniel Kinzler (WMDE) (talk) 16:13, 7 March 2018 (UTC)
      • When you're doing a big refactor, sometimes you have to take big steps if you want to get anywhere useful. If you're afraid to change all the things that need changing, you may lock yourself into a bad design. Note I'm not saying a big step has to be one big patch. Anomie (talk) 15:32, 8 March 2018 (UTC)
        • Indeed - but you also have to make sure you cut off at certain points. This is the forth iteration of this refactor, and I have quite intentionally reduced it in scope every time. I'm not afraid to change all the things that need changing - I want to find way to do it iteratively, and at the same time avoid adding technical debt by adding hacks to old code. It's of course a question of striking a balance, and of personal taste and experience.
        • Avoiding lock-in to bad design is exactly why I oppose anything that relies on global state, or on putting more concerns into Content (using subclassing, no less). It's exactly why I want to define explicit interfaces for the concepts we need, e.g. by explicitly modeling page types. -- Daniel Kinzler (WMDE) (talk) 14:28, 9 March 2018 (UTC)
          • One of the things we keep arguing about is whether we actually need page types as a concept, rather than slot types. Anomie (talk) 15:56, 9 March 2018 (UTC)
            • We already have both concepts in the code base. We don't have handlers/controllers for slot types (aka roles). And the page type handlers are currently hard-coded, but can be extended via hooks. -- Daniel Kinzler (WMDE) (talk) 17:16, 9 March 2018 (UTC)
              • Just because we have file pages already doesn't mean that's something we want to keep in MCR, it may be (and IMO probably is) the case that we can get rid of that hack in favor of a more general mechanism. And of course we don't have handlers for slot types, because we don't have slot types yet, so that's another bogus argument. Anomie (talk) 13:41, 12 March 2018 (UTC)

From my perspective, these requirements look correct as I understand them. --MarkTraceur (WMF) (talk) 19:01, 5 March 2018 (UTC)

Requirements

Baseline

Goal: provide an MCR-capable interface for updating pages, with a minimum of refactoring of adjecant code. The implementation doesn't need to fully support MCR yet.

• Use ContentHandler::getParserOutput to generate output for each slot
  • A true "baseline" doesn't require this. You only list it here because of the extra requirements you include below. Anomie (talk) 20:32, 26 February 2018 (UTC)
• Use Content::getSecondaryDataUpdates to get data updates for each slot
  • A true "baseline" doesn't require this. This seems aimed at "SDC wants two slots blindly concatenated". Anomie (talk) 20:32, 26 February 2018 (UTC)
• Update links tables with information from all slots. In the future, links tables may also record which slot references what resource.
  • A true "baseline" doesn't require this. This seems aimed at "SDC wants two slots blindly concatenated". Anomie (talk) 20:32, 26 February 2018 (UTC)
    • It's aimed at "SDC wants to be able to place the output of two slots freely on a page", along with "SDC wants to re-use the Wikidata representation and editing interface for the structured output", which I just confirmed as requirements with Mark and Ramsey. Plus "it seems really useful to be able to use output for different slots independently of each other for various use cases", which I confirmed with James. Do you disagree with these things even being requirements? -- Daniel Kinzler (WMDE) (talk) 18:32, 28 February 2018 (UTC)
      • Which doesn't change the fact that a true baseline doesn't require it. Just that SDC's requirements can't be satisfied by a true baseline that doesn't blindly concatenate slots. Anomie (talk) 17:25, 1 March 2018 (UTC)
        • There is no intention for the baseline patch to concatenate HTML. The patch [1] generates POs for all slots, uses them to register secondary data, and then throws the POs away: the code for generating the "combined" PO currently just uses the main slot's PO.
        • One could argue that secondary data (e.g. pagelink rows) should only be added for content in the ParserCache, since we use pagelinks to purge the parser cache. But we also use it for whatlinkshere, to find (and change) references to pages. You'd want to find e.g. template documentation linking to a page in whatlinkshere, even if we end up not showing the documentation in the default view, right?
        • But we can push back the decision on what secondary data gets tracked when, and only do it for the main slot in the initial patch. I'd still want the POs for all slots to be in the PreparedEdit - we will need them in some shape or form anyway. -- Daniel Kinzler (WMDE) (talk) 11:12, 2 March 2018 (UTC)
          • Why wouldn't we show the template documentation in the default view? We do currently, at least on enwiki. I also note that we already get complaints when parser functions cause unexpected links on WhatLinksHere, we'd want to consider that before potentially making it worse by blindly adding links from undisplayed slots.
            "we will need them in some shape or form anyway"^{[citation needed]} Anomie (talk) 14:34, 2 March 2018 (UTC)
• Store a single ParserOutput for the revision in the ParserCache. Can be just the ParserOutput for the main slot for now, but will have to somehow contain all information from all ParserOutputs of all slots eventually. The code composing that combined output may be different for different "kinds" of pages, and may be defined by an extension. The mechanism for that doesn't need to be decided at this stage, though.
  • It won't necessarily have to contain all information from all slots. Even if it does contain some information from a slot, that need not be all the information. Trivial example: if we add a "metadata" slot to hold categories and such, there's no need for the main ParserOutput to contain whatever HTML might be generated to view that metadata. Anomie (talk) 20:32, 26 February 2018 (UTC)
    • If we want to have it in the ParserCache, it has to be in the PO. Or we re-design the ParserCache. -- Daniel Kinzler (WMDE) (talk) 18:32, 28 February 2018 (UTC)
      • The key word there is "if". Do we actually want random unused HTML and other content in the main PO, though? Anomie (talk) 17:25, 1 March 2018 (UTC)
        • It's not random, nor unused, not blindly concatenated. The ParserCache would have the HTML as shown in the standard view. And the HTML will in the future be explicitly composed from the POs of the individual slots, not blindly concatenated.
        • We may want to have the per-slot HTML in the ParserCache, using separate keys or in a nested object, for optimized re-rendering. But that's not part of the initial patch. -- Daniel Kinzler (WMDE) (talk) 11:12, 2 March 2018 (UTC)
• The use of ApiStashEdit can be restricted to the main-slot-only case for now.
  • Only if EditPage is restricted to the main slot only. Anomie (talk) 20:32, 26 February 2018 (UTC)
    • No, it could just not use stashing if it's being used for multiple slots. This will be rare anyway, at least in the beginning. -- Daniel Kinzler (WMDE) (talk) 18:32, 28 February 2018 (UTC)
      • I suppose, although that would be a performance regression for the cases ApiStashEdit is trying to speed up. Anomie (talk) 17:25, 1 March 2018 (UTC)
        • Sure, but it would still work for 99.9% of the edits. Anyway, nothing keeps us from changing ApiStashEdit when refactoring EditPage to allow multiple slots to be edited at once. I'm just saying that it's not a hard requirement. -- Daniel Kinzler (WMDE) (talk) 11:12, 2 March 2018 (UTC)

This implies that we need access to POs for individual slots and a combined PO during saving. This does not imply anything about the structure of the PO that gets written to the ParserCache or shown in a preview.

Anomie, Daniel Kinzler: just making sure that I understand what is the disagreement here: Brad would prefer an initial implementation in the spirit of T174039 (use the MCR interface, but completely ignore and/or error out on non-main slots), while Daniel would like to pull in enough functionality to make the SDC use-case work, right? The former seems like a more obvious initial goal, so is some time constraint the motivation to do more at once? Or are you worried that doing the change in small steps might lead into some architectural cul-de-sac? --Tgr (WMF) (talk) 10:20, 1 March 2018 (UTC)

No, that's not the issue. The first patch will fail on non-main slots, but we'll have to support them for SDC, so support will be added over the next few weeks. The disagreement is instead:

Anomie would like to keep the main slot "the page content", and have other slot content as ancillary that can be used by the main slot. If need be, hooks could be used to provide some kind of "implicit transclusion" that attaches information from the other slots to the main slot's PO. That implicit transclusion would be controlled by hooks. It's unclear to me how multiple hook handlers would interact to compose the final HTML for the main slot's PO.

My idea is to have a dedicated facility for composing a "combined" ParserOutput for all slots. That "PageOutputComposer" would be bound to page type (e.g. File pages) and could be overwritten by extensions. But the only thing that is blocking the initial patch[2] is the question whether WikiPage (or rather, the new PageUpdater) should know about a "combined" ParserOutput that is separate from the stand-alone output of the main slot.

Below, Anomie talks about "pre-implicit-transclusion output" vs the "post-implicit-transclusion output". This sounds very similar to that I have in mind. The discussion how output is composed, and what component owns the composition, still needs to be had, but it should not block the initial patch, I think. As far as I understand the issue, blocker for this patch is really that I found it useful, for practical reasons, to have the distinction between "main only" and "combined" PO explicit the code. -- Daniel Kinzler (WMDE) (talk) 14:00, 1 March 2018 (UTC)

My problem with this section is that it's neither a baseline that ignores or throws errors for non-main slots nor a step towards a functional implementation. Instead it's a hacky implementation that would minimally work for SDC but makes questionable choices that may leave us with tech debt to clean up when the actual implementation is done, depending on what that actual implementation looks like. My problem with Daniel's desire to have a merged PO that contains every slot's individual PO is that it wastes work generating (and space saving) the PO for every non-main slot even if it's not actually required. And, as I said elsewhere, my problem with Daniel's idea of having a combining class per page is that you can't have more than one of them, meaning only one extension can reasonably define extra slots for any particular page. Anomie (talk) 17:25, 1 March 2018 (UTC)

Let's differentiate between two things: the current baseline patch, and the minimum requirements for SDC.

The current patch doesn't do any combining of POs. It does generate all POs, to register secondary data. That could be considered wasteful if the only need for registering things in the links tables was purging the parser cache. But that is not the only use of that data. But even if it was - we can assume that typically, information from all slots makes it into the default view somehow - that means we need to purge & re-render whenever anything that any slot depends on changes. So we need to run the DataUpdates for all slots.

For SDC, we need some mechanism for composing the HTML, and putting the composed HTML into the ParserCache. That does not require nested POs, though we may want them for optimized re-rendering. We don't have to decide that now, and we don't have to decide on the composition mechanism now. We don't even *have* to have the secondary data tracking (links table updates) for all slots in the baseline patch, but we'll need it for SDC.

I don't see what is hacky, and what needs to be cleaned up. I find a clear separation of composed output and main slot output helpful, both conceptually and in the code. And I see no way around generating POs for all slots, at least whenever the respective slot changes. We could optimize by generating POs with no HTML, getParserOutput supports this in theory, though I don't many implementations honor that flag. -- Daniel Kinzler (WMDE) (talk) 11:12, 2 March 2018 (UTC)

If you still don't see any way around generating POs for all slots, then I may as well give up. I've already said how to do it multiple times on this page. Anomie (talk) 14:34, 2 March 2018 (UTC)

Multi-Slot View

Model use case: Structured Data on Commons (SDoC). Single-slot viewing should also be possible.

• Allow the rendering of one slots to depend on content of all slots. (Possible restriction: only allow the main slot to depend on other slots. This would allow the main slot to depend on the rendering of other slots, which could otherwise lead to circular dependencies). This needs ContentHandler::getParserOutput to have access to all slots.
• Allow Article::view() (or a replacement of that method) to show the content of each slot, rendered individually, in separate sections of the page. In particular, allow a rendering of the structured data to be shown that is independent of the wikitext, rendered in the user's interface language. That rendering also serves as the editing interface for structured data, just like on a Wikidata page.
  • This, IMO, should be a non-standard view. And it may not need to display all slots at once, versus showing individual slots. Anomie (talk) 20:32, 26 February 2018 (UTC)
    • I asked that question to the MultiMedia team. The result was that they want to at least have the option to have the standard view serve the full HTML of both slots. And it seems likely that this will be the case at least for the initial roll-out. -- Daniel Kinzler (WMDE) (talk) 18:39, 28 February 2018 (UTC)
      • What is meant by "standard" here? That no extension is customizing the rendering, or that the user is not customizing the request (so e.g. the request is for /wiki/File:Foo.png and not /w/index.php?title=File:Foo.png&slots=all)? If it's used in the first sense, why is it important for the MM team what the standard behavior is? They are, after all, working on an extension to customize the rendering of file pages. --Tgr (WMF) (talk) 10:19, 1 March 2018 (UTC)
        • The "standard" view is /wiki/File:Foo.png, as opposed to ...&slots=mediainfo or ...&slots=subtitles. ...&slots=main would be the same as the default view with Brad's approach, but could provide a different view with my approach. ...&slots=all isn't really a thing in either model: with my approach, all slots would typically (but not necessarily) be visible in the standard view, while in Brad's approach, the standard view is the same as the main slot view, which may have content from other slots transcluded, implicitly or explicitly. -- Daniel Kinzler (WMDE) (talk) 14:14, 1 March 2018 (UTC)
      • Let's not play Telephone. Why can't you ask them these questions here and have them respond here? I have a suspicion that your question and their answer may have confused "the standard view for SDC should include both the wikitext and wikibase output" with the more general question of whether we actually need the ability to have a view that directly shows the HTML of all slots, including slots with HTML that isn't included in the standard view. OTOH, even if we don't need it, it may be simple enough to include it anyway (with ...&slots=*, or perhaps only via enumerating all slots like ...&slots=main|foo|bar). Anomie (talk) 17:25, 1 March 2018 (UTC)
        • Indeed, I did send them an email asking them to comment here.
        • The question of what goes into the standard view is mainly what goes into the ParserCache, what goes into the web cache, and how much coding is needed to make that happen.
        • You are asking the more general question of whether we actually need the ability to have a view that directly shows the HTML of all slots, including slots with HTML that isn't included in the standard view. I don't think we need such a view. I think the standard view should be composed of whatever bits of the content of all slots "the composer" (whatever that may be) sees fit. And I believe we need to track secondary data for all slots, even if not shown in the standard view, for stuff like whatlinkshere (ideally, we could differentiate in the links tables - this is where we get into the discussion of having a separate mechanism for dependency tracking for purging; I'm all for it, and I'm working on a plan with Marko). I do not think we need a view that always shows all slots. I think that would be sensible to have that as the default behavior of the standard view, but I don't feel strongly about that bit.
        • So, again: we need all separate PO for tracking references while processing an edit. We want HTML for some slots combined somehow in the ParserCache and CDN for the standard view. We don't necessarily need a view that gives HTML of all slots. We do need separate views showing the HTML for each slot. -- Daniel Kinzler (WMDE) (talk) 11:23, 2 March 2018 (UTC)
• The code composing that combined output, when constructing and/or when using the PO, will be different for different "kinds" of pages, and may be defined by an extension. In the SDoC case, it will be specific to the File namespace, and will be supplied by the WikibaseMediaInfo extension.
  • This is an assumption that need not hold, and either limits the combining to a single extension per page or requires different extensions to hook into or extend each other. What if, for example, TimedMediaHandler wants a slot of its own for subtitles, with its own "combined output" code? Anomie (talk) 20:32, 26 February 2018 (UTC)
    • Since a completely generic combining mechanism seems insufficiently flexible (or would have to be extremely complex), and it's unclear how multiple extensions would cooperate on this, the present idea is to have a singe extension control the rendering of a page. I could also imagine us allowing a single extension to do complex composition, while allowing other extensions to add stuff at the top or bottom.
    • Anyway, what's the alternative? Am I understanding correctly that you want to use wikitext in the main slot as the only composition mechanism? -- Daniel Kinzler (WMDE) (talk) 18:39, 28 February 2018 (UTC)
      • Specifically in the case of TimedMediaHandler, it'll probably want to customize the MediaTransformOutput rather than the ParserOutput as the subtitles relate to the image and not the description wikitext. But in general, yeah, the ownership of composition seems tricky. One approach is that content handlers have access to what role the content is in (seems like a good idea in general) and can request to be included in the view. (So by default only the main content is visible but other slots can opt in.) Each of the visible slots could have access to all others and do its own composition then. --Tgr (WMF) (talk) 10:19, 1 March 2018 (UTC)
        • My initial thinking was also that this should be controlled by the ContentHandler. But then I realized that this is not a good separation of concerns, and gets in the way of extensibility. E.g. for template documentation, both the main and the doc slot are wikitext. Why should the handler for a content model know about the different purposes that kind of content can be used for? And how could extensions add to that? If it's the main slot's ContentHandler that does the composing (as I originally envisioned), how will the wikitext handler on File pages know how to deal with MediaInfo?
        • It seems to make much more sense to me to bind this behavior (a PageOutputComposer) to a "page type" - which, for now, is given by the namespace. But yes, if multiple extensions want to inject things into the File page's layout, some kind of generic handling needs to be possible. Perhaps the PageOutputComposer could have a list of "other slots" to add at the bottom, to which extensions can add.
        • Note btw that all slots need to be accessible via a single-slot view, to ensure no content becomes inaccessible when configuration changes or strange things happen due to undeletion or import. -- Daniel Kinzler (WMDE) (talk) 14:09, 1 March 2018 (UTC)
• Note that for output in the user language, the ParserCache needs to be split (which is already the case for File pages on Commons)

Multi-Slot View (Anomie's alternative)

• The default view will show the content of the main slot, as it does now for the non-MCR case.
• The main slot can explicitly transclude content from other slots, via parser functions or tags, as can happen now. This probably won't generate a ParserOutput for that slot, although that's a possibility if the implementation of the function/tag finds that useful.
• Content from other slots might also be implicitly "transcluded", much like how Extension:Cite does now when no ‎<references /> is present in the wikitext. This need not generate a ParserOutput either, e.g. a "metadata" slot might just call addCategory(), addDisplayTitle(), and so on to add to the ParserOutput from the main slot. We could locate the code for implicit transclusion in hooks (as Extension:Cite's implicit ‎<references /> does now), or we could add it to Content or ContentHandler to avoid having to use hooks.
  • So, the main implication is that the default view will always be the main slot only. The only way to show things from other slots in the default view would be to explicitly access it from the main slot's wikitext (or other content), or by using hooks to do something very similar to what I am proposing with the combined PO.
  • Since pretty much all the use cases that have been discussed for MCR (Requests_for_comment/Multi-Content_Revisions#Use_Cases) assume that the content of all slots is visible per default, it like a good idea to establish a standard mechanism for achieving this easily. I don't see how this can be done in Content or ContentHandler, I'd do it in a separate "PageOutputComposer" or "PageTypeHandler", since the composition logic doesn't have anything to do with the content model. Point in case, for e.g. template documentation, all slots would be wikitext. -- Daniel Kinzler (WMDE) (talk) 22:36, 28 February 2018 (UTC)
    • Of the use cases described there, structured media info, gadget styles and file history should be visible by default; infobox data, (probably) the page metadata, templatedata, templatestyles, (probably) video subtitles, assessments, focus area, CentralNotice bits, ProofreadPage and blame maps shouldn't. I could see categories and template/module docs go either way - get their own "view slot" or just get pulled in by the main content. (Arguably they should work with arbitrary types of main content so maybe getting their own slot makes more sense.) Some use cases discussed elsewhere were Wikidata description overrides and page annotations, which also should not be visible. So only displaying the main content seems like a reasonable default.
      Having something like ContentHandler::shouldBeVisible() seems like an OK way to allow easy opt-in, although the content handler would have to know the role for that (and maybe at that point it makes more sense to have some kind of role handler?).
      As for PageTypeHandler, how would it differ from the content handler of the main slot? We have no such thing as page type. --Tgr (WMF) (talk) 11:13, 1 March 2018 (UTC)
      • I actualyl disagree on may of the things you say should be invisible (why should infoboxes be invisible? or templatedata? or assessments? I believe they should all be visible in some way somewhere on the page - they just shouldn't have their own section). But that's not what needs to be decided here.
      • The question is whether code should assume that "the page content" is the content of the main slot, and other stuff should only be shown if the main slot's content, or the main slot's ContentHandler, or something hooking into that, wants to show it. That means that in code, there is no distinction between "main slot content" and "all slot's content". Even ignoring the HTML composition, this poses problems for tracking secondary data of non-main slots: it would only be tracked if explicitly looped through by the main slot.
      • I would much prefer a mechanism that allows us to always track all secondary info from all slots, as defined by their ContentHandlers. I would also prefer to not bind the decision what is shown to the content model of the main slot. I also don't see how it belongs into the content handler of the "extra" slot. As I said, both can be wikitext, and why should the wikitext handler know what loa module documentation is, and whether it should be visible per default? That's a decision that belongs to "module pages", in my mind.
      • But again - besides when and where and how to compose HTML, the major issue is how to track secondary info, and when and where to determin what DataUpdates need to be run to track it. If we don't want to change much about how DataUpdates work, we'll have to call Content::getSecondaryDataUpdates() for each slot, for the ParserOutput of that slot. I'd really like to avoid having to fundamentally change that. -- Daniel Kinzler (WMDE) (talk) 14:25, 1 March 2018 (UTC)
        • On the other hand, if the "page's content", aka the "standard view", is the same as the "all slots view", then there's no way for a slot to show more helpful HTML output when viewed non-standardly. For example, when viewing a template that has a TemplateStyles slot you'd either never be able to see the pretty-printed CSS or you'd have to always see it on the page. Or for article assessments, there'd be no way short of opening the editor to get a more explicit list of the assessments than whatever icons the assessments might add to the default view of the page (and, at least on enwiki, assessments other than "good article" and "featured article" only belong on the talk page with no indication on the article itself).
          I find it odd that you want to add a whole inflexible "page controller" infrastructure and a model that doesn't allow a slot to not be displayed by default to avoid any changes to how Content::getSecondaryDataUpdates() might work for non-main slots. Anomie (talk) 17:25, 1 March 2018 (UTC)
          • I think we are getting to the core of the misunderstanding here.
          • The idea is not that the standard view has to show the HTML of all slots. And the composition logic for the standard view may use HTML from each slot's own PO, but it could also render the content of other slots in some other way, or ask the slot's Content object to render in some other way for use in the standard view. The composition logic can just as well ignore some slots.
          • I think whatever the standard view is should be in the ParserCache. This is the easiest thing to do, and it matches the expectations of code that reads from the PC. However, having the POs of each slot, or rather, the slots used to generate the HTML of the standard view, "nested" in a combined PO is just an idea for optimized re-rendering. Nothing forces us to do that.
          • But we do need to generate the POs for all slots when saving an edit, so we know what to put into the links tables. But they don't have to be used to generate the main view. This is actually the reason I put the list of per-slot POs and also the combined PO into the PreparedEdit. The need and opportunity for this split occurred to me while working on this patch. Previously, I thought it would be necessary to feed the secondary tables from the combined PO. This is not the case - in fact, it wouldn't work without fundamental changes to getSecondaryDataUpdates(). Perhaps this is what caused the confusion.
          • We could try to only generate "blind" POs, with no HTML, but it's tricky to know in advance for which slots we won't later need to HTML. So I'd leave that optimization for later. -- Daniel Kinzler (WMDE) (talk) 11:48, 2 March 2018 (UTC)
            • No, we don't need POs for every slot to handle links tables. All we need is that any necessary links wind up in a PO that gets passed into LinksUpdate. That can be the "combined" PO, which as I've already said need not actually be generated by combining per-slot POs. Anomie (talk) 14:34, 2 March 2018 (UTC)
    • I don't see how this can be done in Content or ContentHandler When MediaWiki is doing the implicit transclusion step, instead of (or in addition to) calling a hook it could do something like foreach ( $nonMainSlots as $slot ) { $slot->getContent()->amendParserOutput( $parserOutput, $role ); }. The actual implementation may, of course, be more complicated (e.g. consideration of ordering), but that's the basic idea for letting the Content or ContentHandler do implicit transclusion in my model. Anomie (talk) 17:25, 1 March 2018 (UTC)
      • It seems odd, logically, to bing this to the ContentHandler. The ContentHandler knows about a specific kind of content, it does not know about the roles this content may have on a page.
      • I initially also though this would be the easiest approach. But now it seems clear to me that injecting handling for concepts like template documentation or structured media info into the WikitextContentHandler is backwards and unnecessary.
        • • It seems to me that attaching the composition logic to page types gives us flexibility, it doesn't take any ways. -- Daniel Kinzler (WMDE) (talk) 11:48, 2 March 2018 (UTC)
            • As soon as you say "into the WikitextContentHandler", you should know that you've missed the point. A documentation slot could be a "DocWikitextContentHandler" subclass of WikitextContentHandler. Or it could be done with hooks. Or perhaps we could have slots register handlers that aren't ContentHandlers, which would still be better than a one-per-page handler. Anomie (talk) 14:34, 2 March 2018 (UTC)
• It should also be possible to view an HTML rendering of a non-main slot. This would generate a ParserOutput for the slot.
• The parser cache will likely need to be extended to be able to cache non-main slot ParserOutput objects, both for the non-main viewing and for use by potential implicit transclusion. Whether we cache only the main slot's pre-implicit-transclusion ParserOutput (and run implicit transclusion as a post-cache transformation), only the post-implicit-transclusion ParserOutput (and thus re-parse the main slot whenever any slot changes), or both is yet to be determined.
  • I was trying to avoid that. Partially just to avoid additional work, but also because code reading from the ParserCache expects the PO in there to be "the page output". Which it wouldn't be if we only put the main slot there. Basically, my thinking is that all slots contribute to the page's output. Your approach seems to be that the main slot is the page output, but there may be other page output, which may or may not be tagged onto the main output. -- Daniel Kinzler (WMDE) (talk) 22:36, 28 February 2018 (UTC)
    • Is it really so much extra work to add an optional $slot parameter or saveForSlot/getForSlot methods if a slot wants to put its own ParserOutput into the cache? That seems like a lot less work than adding a new "page controller" concept. Anomie (talk) 17:25, 1 March 2018 (UTC)
  • Anomie, I just realized: your pre-implicit-transclusion output is my "main slot" output. Your post-implicit-transclusion output is my "combined" output. We are not disagreeing here. It's the same thing. We need both somewhere - and working on the initial WikiPage refactoring[3], I found that things become a lot easier if both are available from the PreparedEdit (I was trying to avoid that, but kept running into trouble). We can keep talking about how the code that does the "implicit transclusion" aka "output composition" lives - but that discussion does not need to block the current patch. For that patch, the only thing that matters is that we need a way to a) get a PO for each slot and b) get a PO with implicit transclusion (aka output composition) applied. And I did not find a way to avoid having both in PreparedEdit -- Daniel Kinzler (WMDE) (talk) 13:49, 1 March 2018 (UTC)
    • We're disagreeing on some of the details. You want to parse every slot and have a one-per-page class to combine the slots' ParserOutputs, which means that you're designing everything around actually creating and passing around all those POs. I want to begin with the one "main" ParserOutput and let code for non-main slots add to it using a hook model, without creating additional POs if it's not actually needed for the slot's use case and without a one-per-page combiner. Anomie (talk) 17:25, 1 March 2018 (UTC)
      • As I pointed out above, the other POs are always needed, at least temporarily. The baseline patch passes them around only as much as needed to feed the links tables.
      • I'd like to reduce the usage of hooks, but some kind off extension point / injection will be needed for whatever code composes the HTML for the standard view. I see no reason for this code to live in a ContentHandler, though. -- Daniel Kinzler (WMDE) (talk) 11:48, 2 March 2018 (UTC)
        • As I pointed out above, the other POs are not always needed. We're having this discussion because I disagree with how you implemented your "baseline" patch, so appealing to it is a bogus argument. Anomie (talk) 14:34, 2 March 2018 (UTC)
• Access to other slots of the revision will proceed by loading the revision, normally by using Parser::fetchCurrentRevisionOfTitle(). If a Parser instance isn't available, ParserOptions::getCurrentRevisionCallback() could be used more directly or via a similar helper.
  • This could work, but we'll want to be able to use this before a Revision has been saved. In my mind it makes more sense to have a way to pas the ParserOptions for other slots - which may or may not come from the revision. -- Daniel Kinzler (WMDE) (talk) 13:49, 1 March 2018 (UTC)
    • Even if the RevisionRecord isn't saved yet, we should be able to construct one. It may be more complicated for MCR than what ParserOptions::setupFakeRevision() does currently, but that's the general idea. That also has the benefit of working well with existing code like TemplateSandbox. Anomie (talk) 17:25, 1 March 2018 (UTC)
      • If that'S the easiest way, then let's do that. -- Daniel Kinzler (WMDE) (talk) 11:48, 2 March 2018 (UTC)

The SDoC case could work like Extension:Cite's ‎<references />: if the wikitext includes a tag to say "insert the structured data here" it will do so, and if not it'll append the structured output to the main slot's wikitextoutput.

• Appending to the wikitext will not work, it would need to append full HTML, plus registering the necessary JS modules, CSS modules, tracking info for the links table, etc. This sounds a lot like the "combined" PO I was suggesting, but it's unclear to me how the Wikibase content can register its DataUpdates. Probably by hooking in again at some later point. Basically, this sounds like an emulation of my proposal using hooks instead of defining a mechanism for composing a PO from multiple slots.
• Overall, this seems to be more work: we have to modify the parser cache, introduce new hooks, provide ways to access all kinds of content from withing wikitext, or use several hooks for every slot so it can attach itself to the main PO. I'd rather have a mechanism that just defines how the combine PO is composed, with a per-page-type handler for combining the HTML in the desired way. -- Daniel Kinzler (WMDE) (talk) 22:22, 28 February 2018 (UTC)
  • "to the main slot's wikitext" was not an accurate statement, I meant "to the main slot's output". A ParserOutput doesn't even have wikitext to append to. And yes, in the case of SDC, it would be a lot like the "combined" PO you're suggesting. Your DataUpdates can probably be registered in the same way they are now, by having MediaWiki call Content::getSecondaryDataUpdates() on the content of each slot.
    But it's not "an emulation of [your] proposal". Your proposal wants to generate individual PO objects then combine them using a one-per-page combiner class. My proposal wants to allow multiple Content objects to contribute to a single PO without a one-per-page combiner, which need not proceed by generating a PO for each one.
    I don't think my proposal is more work, but it's not "I get to write entirely new code instead of refactoring old code" like your proposal is. We may have to make a minor extension to the parser cache, "ways to access all kinds of content from within wikitext" would have to happen anyway unless you want to be even more inflexible and forbid explicit transclusion, and it'll much more likely be one hook than several. Anomie (talk) 17:25, 1 March 2018 (UTC)

• I think there are orthogonal issues here:
  • Pull model (only the main content is parsed; it can initiate the parsing of other slots if it desires) vs. push model (all slots are are parsed and then the ParserOutput objects merged somehow). The latter seems easier to coordinate.
  • PO object hierarchy: should the non-main content POs be merged into / provided to the main content PO, or should all the content POs combined into a new revision-level PO? I guess that depends on whether we expect that typically the main content type determines how combining should work, or it's something that would typically be controlled by an extension that registers itself in some content-type-unrelated way. I can't really think of any counterexamples to the first option.
• --Tgr (WMF) (talk) 11:13, 1 March 2018 (UTC)
  • SDC is a counter-example. The main slot is wikitext. How or why should it know or care about MediaInfo? The wikitext will have access to structured data via Lua modules, but the wikitext content model doesn't know about that. And this has nothing to do with exposing the full structured data as HTML along with the rendered wikitext description. -- Daniel Kinzler (WMDE) (talk) 14:28, 1 March 2018 (UTC)
    • Neither of our proposals are having the wikitext ContentHandler know or care about MediaInfo, but IMO yours is closer to that model. You want to add a "page model" that forces the use of the SDC content-combiner. I want combining to proceed based on the slots that are present: the SDC code is called when the SDC slot exists to add that slot's content to a ParserOutput that may or may not have been generated by a wikitext main slot, and other extensions' code is called for their slots if those also exist. Anomie (talk) 17:25, 1 March 2018 (UTC)
  • "Pull model" versus "push model" is not an accurate characterization here. My proposal has both pull (explicit transclusion) and push (implicit transclusion). Daniel's does too unless he's forbidding explicit transclusion. The differences are in how exactly the pushing works: generate POs for each Content and have a one-per-page combiner operate on those POs, or have each Content push output into a single PO without a one-per-page combiner to coordinate it. The former (Daniel's) is simpler but less flexible. Anomie (talk) 17:25, 1 March 2018 (UTC)
    • In your model, how does WikitextContent used as template documentation know where and how to "push" itself into the main PO? -- Daniel Kinzler (WMDE) (talk) 11:48, 2 March 2018 (UTC)
      • To avoid duplication, I'll just mention here that I answered that above. Anomie (talk) 14:34, 2 March 2018 (UTC)

Multi-Slot Editing

Model use case: Atomic editing of a Lua module and its documentation. Single-slot editing should also be possible.

• Allow EditPage to show one editing area (for text based content: one text area) for each slot. Also provide a single-slot editing mode.
• Allow previews to be rendered for all slots or individual slots.
• When parsing/rendering after save, re-use the cached rendering of unchanged slots, instead of regenerating it needlessly. However, if the rendering of the unchanged slot depends on a slot that was touched, it needs to be re-rendered anyway.

This implies that ApiStashEdit, ApiParse, and ApiEditPage all accept input for multiple slots at once.

IMO trying to figure out the edit view based solely on architecture requirements and only incuding designers when it's fully implemented will end badly. Just throwing out a bunch of text boxes under each other is probably not acceptable UX. (It will be needed as a non-JS fallback as there is little else to do int that case, but usually some mechanism will be required to decide which component gets to control the rendering. Maybe some kind of tabbed view would make sense for handling multiple large edit boxes. TimedMediaHandler might have 200 slots for subtitle translations that it wants to handle in a single view block, without interfering how the rest of the content is rendered. VisualEditor takes over the whole content area, it might want to force the non-main components into dialog boxes. What would be a reasonable interface on a mobile screen? etc.) It would be good to ask for a set of mock designs based on the use cases collected so far, just to have some ideas to base the architecture planning on. --Tgr (WMF) (talk) 07:25, 1 March 2018 (UTC)

You are right that we shouldn't discuss UI specific here. The point is that we need to support the case of the updated content of multiple slots coming in via a single POST request, and that we may want to optimize for the case where one slot changed, and another did not. If we don't need this optimization, we don't need a "hierarchical" ParserOutput. Then it is sufficient to cache the composed HTML, and regenerate the whole thing by re-parsing all slots on edit. -- Daniel Kinzler (WMDE) (talk) 14:33, 1 March 2018 (UTC)

We might avoid the need for that optimization by not actually parsing every slot unconditionally, instead letting the individual slots decide whether and what kind of caching is needed. Anomie (talk) 17:25, 1 March 2018 (UTC)

On the other hand, getting designers involved too early is also going to end badly, and having Daniel and/or I (rather than developers more familiar with front-end coding) being the ones to implement the designers' designs might also be a bad end. If you want to take on the responsibility for redoing the UI of EditPage, as far as I'm concerned you're welcome to it. Otherwise my plan is to put together a MVP (which might not even get merged) for the textarea-based editor and let others who're better qualified handle VE and improvements to the textarea-based editor. I might make a tabbed interface, or I might simply show one slot at a time and force the user to submit to change slots. And I hope TimedMediaHandler doesn't actually add a slot per language. Anomie (talk) 17:25, 1 March 2018 (UTC)

Proposed Changes to ParserOutput

• Each ParserOutput can record which slots it depends on
• The combined canonical PO has to return aggregated information of all slots from all getters
• The combined canonical PO should not contain duplicate information, since it will be serialized and stored in the parser cache, so size matters.

Option 1, compose output just in time:

• The combined ParserOutput for all slots would have to
• know the ParserOutput objects for each slot
• getters aggregate info from all the POs on the fly.
• know how to combine the HTML from all slot's ParserOutputs into a single output (or return null from getText()), by substituting placeholders in an HTML "template". Note that this template may be defined or modified by an extension, and may depend on page type or namespace or some other property of the page.

Options 2, compose output right away:

• The combined parser output gets fed with the aggregated content of the POs.
• The code that constructs the combined PO also constructs the combined HTML right away.
• This means that during saving, the POs of unchanged slots cannot be re-used.

Note that option 1 and 2 offer largely the same interface to calling code. The only code that needs to be aware of this distinction is the PO itself, and the code that constructs it.

This means that we can start with option 2, and change to option 1 later when need arises, for optimizing the rendering during save, or for other purposes. E.g. AbuseFilter may want to apply different rules to different slots, so it would have to process the POs of the individual slots separately.

• Since all this depends on your model of how things work that I disagree with, I find all this unnecessary as well. Anomie (talk) 20:32, 26 February 2018 (UTC)
  • If I understand correctly, you want to "somehow" merge all info from the extra slots into the ParserOutput of the main slot. That seems very close to my proposal, the only difference being whether we conceptually have a "combined" PO that is different from the main slot's PO or not. I think it would be useful for this discussion if you could state your proposal below, and explain how it meets the requirements above. If you disagree with the requirements, please explain how and why. -- Daniel Kinzler (WMDE) (talk) 18:27, 28 February 2018 (UTC)
    • That's largely correct, although we could still have a "combined" PO (post-implicit-transclusion) that is different from the main slot's PO (pre-implicit-transclusion) in my proposal. The real differences are whether we force every slot to generate a PO to make that "combined" PO and how exactly the combining happens. Anomie (talk) 17:25, 1 March 2018 (UTC)
  • Actually, for the "combine output using hooks" version of your proposal, the need/opportunity for optimization is the same. The difference is then just whether we have separate keys for each slot in the ParserOutput. I like separate keys in principle, but see little practical advantage, and I expect sticking with a single key is going to be a lot simpler. -- Daniel Kinzler (WMDE) (talk) 23:05, 28 February 2018 (UTC)
    • You're still hung up on the idea of always generating a PO for every slot. That need not happen to generate the "combined" PO, even if it is needed for a non-standard view of that slot. And even if a PO is created for a slot, we don't necessarily have to cache it if we know it's always going to be cheap to regenerate. Anomie (talk) 17:25, 1 March 2018 (UTC)
      • I'm hung up on the idea of tracking page links and image links and template usage etc for all slots, even if they are not visible in the standard view, yes. It seems to be that this is what we disagree on. Let's get input on this question from other developers and product folks. -- Daniel Kinzler (WMDE) (talk) 11:53, 2 March 2018 (UTC)

Page Type Considerations

I have said above that I think the "composition logic" for combining HTML from different slots should be bound to the concept of "page type", e.g. "file page" or "article page" or "template page", etc.

This isn't absolutely necessary, though. My main point is that this logic should not be bound to a content model. What could be an option is to have "combining logic" for each slot role. However, I like the "page type" approach better, for two reasons:

1) If we leave the combining to the respective slots, all they can do it append their HTML above or below what's already there. More complex composition isn't possible.

2) We will need "page type handlers" of sorts anyway - in fact, we already have them, in the form of FilePage/WikiFilePage and CategoryPage/WikiCategoryPage. If we want to refactor WikiPage and Article, these need to be replaced by some kind of "file type handler", with extension being able to add their own (just like they now do via the WikiPageFactory and ArticleFromTitle hooks). These handlers would have the responsibility to provide the appropriate action handers, diff view, edit page, redirection logic, purging logic, etc. All these functions will have to involve all the slots. It seems natural to also place the content composition there.

In any case, for factoring PageUpdater out of WikiPage, none of this needs to be decided. The first iteration can just use the main slot for the "composed" standard view. -- Daniel Kinzler (WMDE) (talk) 13:37, 2 March 2018 (UTC)

• Having "combining logic" for each slot role is one of the two main features of my proposal.
  Your #1 is entirely false. There's nothing that can be done in a per-page combiner that can't be done by a per-slot combiner unless you're doing crazy things with multiple unrelated slots that the slots themselves know nothing about or you're "combining" the one main slot with no non-main slots in some weird way.
  Your #2 is also false. Yes, we have FilePage, but we could store reference to the file as a slot and get almost everything there that way (the missing bits are, potentially, some details of the show-Commons-locally functionality). CategoryPage goes beyond what can sanely be done with a per-page MCR combiner anyway, it's closer to being an implicit special page.
  The problem with PageUpdater is that you're basing decisions like "we need a PO for every slot" on things that need deciding here. Anomie (talk) 14:34, 2 March 2018 (UTC)
  • "doing crazy things with multiple unrelated slots that the slots themselves know nothing about" is exactly the point - the ContentHandlers should not have to know anything about how or where the content is used. I'm not opposed to per-slot logic though. I'm just opposed to binding it to the content model.
  • "we could store reference to the file as a slot and get almost everything there that way" yes, exactly - but the overall layout of media content, description wikitext, etc, should not be baked into the WikitextContentHandler. That seems to be the entirely wrong place for that.
  • "basing decisions like 'we need a PO for every slot' on things that need deciding here" - yes, I'm assuming that I'm not allowed to break secondary data tracking for Content in non-main slots, until I have a very clear vote from all affected stakeholders that yes, I'm allowed to break that. -- Daniel Kinzler (WMDE) (talk) 17:42, 5 March 2018 (UTC)
    • You seem to be a bit hung up on the idea of putting the combining logic in ContentHandlers, I've pretty much dropped that beyond noting that it could work if we took an approach of subclassing the "wikitext" handler to make "doc-wikitext" for the doc slot. As for the secondary updates, that's already being discussed elsewhere so I'll not repeat it here beyond noting, again, that there's nothing to "break" yet since non-main slots don't yet exist. Anomie (talk) 15:41, 8 March 2018 (UTC)

Product-level requirements and illustrative wireframes

Hi @Anomie: and @Daniel Kinzler (WMDE): . This is quite a spirited debate you've got going on here :). I am not familiar enough with the arcane ways of the inner workings of page rendering to provide specific technical feedback to the above proposals. What I can do is share with you the work we've done so far to define and illustrate how Commons pages will need to look like in the near future (this summer) and by project end (Jan 1, 2020).

I will leave it up to you guys to decide the best implementation that enables the capabilities shown in the designs. Hopefully visuals will help guide you guys to a happy ending where we all hold hands and frolic in a meadow as the sun shines on our grand works.

First, let's talk about requirements for the near future:

Wireframes for this can be found here (13 screens total). Keep in mind these are still works in progress.

These screens represent the following basic needs:

1. We need to render items from Wikibase and Wikitext at the same time
2. We'll need an editing interface, probably the Wikidata representation and editing interface (which we should get for "free")

That's all we need for this summer. We're just showing Multilingual captions (and maaaaybe multilingual descriptions too) from MediaInfo. That's all we're focused on. The thinking here is that we want to make the initial implementation as basic and non-scary as possible for the Community.

Now, let's look at the preliminary designs for the long-term UI plan:

As you can see, these designs are quite different than what we have on Commons today. We are attempting to address a number of UI/UX problems that make Commons a less-than-ideal place for viewing media. That means:

• Making Commons less like old MySpace - right now the pages are a little too flexible. You go to a File page and you really don't know what to expect. We are considering a more rigid structure for the UI layout.
• That new UI structure would have more in common with Wikidata UI than it does with Commons
• Current plan is to not allow markup in our structured data fields; just plain text.
• But we do want to keep the ability to style specific types of statements with HTML (to turn some items into links, for instance). We imagine this could work very similarly to the work done at Wikidata for creating formatters for Lexemes (ex: https://phabricator.wikimedia.org/T184997)
• However, although there doesn't seem to be much opposition to a more defined page structure, community members have expressed a clear desire to have the ability to control the presentation of statements (visually and picking and choosing the data displayed). We do not yet know how this could work, but we're trying to avoid falling back on Wikitext templates for this (although that may ultimately end up as the solution).

Additionally, we've explored the possibility that we may have to offer a "legacy" version of Commons pages for a while. This would just allow the user to switch to the usual full Wikitext, template-based rendering and editing of the File page instead of the new hotness we all just poured sweat and blood into. We've talked to Adam and Lydia about this briefly at All Hands and it seemed doable.

That's the rundown of what we need. If you require additional details, I'm at your disposal to answer all questions (feel free to bug me as much as you need to). Ultimately though, when it comes to implementation, we're leaning on you guys as the subject matter experts to make the best technical decisions that enable our use cases.

-- RIsler (WMF) (talk) 22:03, 2 March 2018 (UTC)