Page MenuHomePhabricator

Move Content methods that don't belong in Content to ContentHandler
Closed, ResolvedPublic

Description

The following methods in Content interface don't actually belong there. Content should be closer to a value object, and these methods are impossible in a value object:

  • Content::getParserOutput
  • Content::preSaveTransform
  • Content::preLoadTransform
  • Content::prepareSave

These methods need to move to new corresponding methods in ContentHandler. Here's a patch that does the first part of the work for Content::preSaveTransform

The signatures of the ContentHandler methods should be:

  • ContentHandler::getParserOutput( Content, PageReference, ?int, ParserOptions, bool ) // TODO: discuss if generateHtml should be a new parser option
  • ContentHandler::preSaveTransform( Content, PageReference, UserIdentity, ParserOptions )
  • ContentHandler::preLoadTransform( Content, PageReference, ParserOptions, array )
  • ContentHandler::prepareSave( Content, PageRecord, int, int, UserIdentity ) // TODO: discuss if PageRecord is needed or we can just do PageIdentity/Reference

You can create subtasks for each method to work on them individually, or just do all in here, up to you.

The process:

  1. Do the same as in the referenced patch for all the methods.
  2. In all extensions where Content overriding classes override the method - override the ContentHandler method instead
  3. Replace all callers. Use ContentHandlerFactory to obtain a ContentHandler, not the Content::getContentHandler method
  4. Once all callers and overrides in extensions is done, do some hard deprecations.

Event Timeline

First thoughts, without thorough investigation:

ContentHandler::getParserOutput( Content, PageReference, ?int, ParserOptions, bool ) // TODO: discuss if generateHtml should be a new parser option

  • Perhaps rename to generateParserOutput, for clarity.
  • generateHtml should either be a parser option, or passed via an additional options array
  • the revision Id could also be part of such an array. Maybe we want a ParserContext class?

ContentHandler::preSaveTransform( Content, PageReference, UserIdentity, ParserOptions )

  • Similar to the ideas above, UserIdentity could be part of some kind of ParserContext or options array.

ContentHandler::preLoadTransform( Content, PageReference, ParserOptions, array )

  • "preload", not "pre-load". This is applied when preloading the edit form with page text. It's not applied before/while loading content in general.
  • It's very specialized, I'm tempted to remove it from the general interface and make it wikitext-only. Or have an extra interface, like PreloadableContentHandler or something. TextContentHandler or even just WikitextContentHandler could implement that,

ContentHandler::prepareSave( Content, PageRecord, int, int, UserIdentity ) // TODO: discuss if PageRecord is needed or we can just do PageIdentity/Reference

  • Intended for last-ditch validation, should be renamed to something like confirmSave or checkValid or something like that.
  • Can't take a PageRecord, because PageRecord should only be used when we know we have an existing page. But we may just be creating the page.
  • The $flags bitfield could be replaced by an options array. More flexible, more readable. And that array could
  • $parentRevId doesn't seem to be used anywhere. I can't think of a good reason to have it here, unless we want to load that revision. If that is the use case, we should just pass in a RevisionRecord -- I guess the caller (PageUpdater?) would already have that.

ContentHandler::preLoadTransform

It's very specialized, I'm tempted to remove it from the general interface and make it wikitext-only.

It's overridden in a few Content classes which are mostly an aggregation of WikitextContent, but I can also imagine easily that you'd want to preload for other content types? It's just not very implemented. Like, we can preload JSON and make it look better for editing or something... I think we should keep it.

generateHtml should either be a parser option, or passed via an additional options array

This would be a really weird ParserOption. Wikitext parser can't really do anything with it, it needs to generate HTML no matter what, but other content handlers can. If it's a parser option, we need to split ParserCache on it - right now we don't break ParserCache cause when we store into the parser cache we don't set generateHtml to false. It seems like it would be too low-level for being a ParserOption.

options array

I think we should part ways with options arrays. With PHP 8 named method parameters we will replace them back to a ton of optional parameters anyway? Although in this case there could be content-model-specific options... Yeah, array is ok.

ParserContext... I donno, introducing it would make this ticket much much more broad, we'd probably need to bring along Parsing team, etc

ParserContext... I donno, introducing it would make this ticket much much more broad, we'd probably need to bring along Parsing team, etc

If we don't do it now, we will have to change this interface again once we create that.

The interface of ContentHandler is hard to change. The interface of ParserContext should be easy to change.

Aklapper added a subscriber: roman-stolar.

@roman-stolar: Per emails from Sep18 and Oct20 and https://www.mediawiki.org/wiki/Bug_management/Assignee_cleanup , I am resetting the assignee of this task because there has not been progress lately (please correct me if I am wrong!). Resetting the assignee avoids the impression that somebody is already working on this task. It also allows others to potentially work towards fixing this task. Please claim this task again when you plan to work on it (via Add Action...Assign / Claim in the dropdown menu) - it would be welcome. Thanks for your understanding!

Pppery assigned this task to roman-stolar.
Pppery subscribed.

Closing as this appears to have been a meta-task for its four subtasks (despite not being tagged as such), all of which are now resolved.