Page MenuHomePhabricator

ArticleRevisionViewCustom hook is unusable
Closed, ResolvedPublic

Description

Hi,

This is really two or three separate problems in one, but they all relate to the same hook, so I figured I'd make one bug report for all of them.

The ArticleRevisionViewCustom hook was added in MW 1.32:

https://www.mediawiki.org/wiki/Manual:Hooks/ArticleRevisionViewCustom

It is meant to replace two existing hooks, ArticleContentViewCustom and ArticleAfterFetchContentObject, both of which were deprecated when that hook was added:

https://www.mediawiki.org/wiki/Manual:Hooks/ArticleContentViewCustom
https://www.mediawiki.org/wiki/Manual:Hooks/ArticleAfterFetchContentObject

Here's the commit where it all happened, by the way:

https://phabricator.wikimedia.org/rMW4835a75ec56444c61c5d0cfbc9e98ceb420fc513

I have two extensions that use ArticleAfterFetchContentObject to modify the text displayed for particular pages: Approved Revs and an upcoming one, tentatively titled Diagrams. (These might be the only two extensions right now that use any of these three hooks, by the way, as far as I know.)

I want to replace the use of the deprecated ArticleAfterFetchContentObject with ArticleRevisionViewCustom in my extensions. However, there are some problems that prevent me from doing this. The first is that this hook is called in two different places in the code, with a different set of arguments in each:

https://phabricator.wikimedia.org/source/mediawiki/browse/master/includes/page/Article.php$756
https://phabricator.wikimedia.org/source/mediawiki/browse/master/includes/diff/DifferenceEngine.php$871

I believe that just adding $this->mOldid as a new third argument in the DifferenceEngine.php call will fix this problem.

The potentially more difficult problem is that it doesn't seem like ArticleRevisionViewCustom really allows for modifying the page display - in either the article or the diff page. In the article display (Article.php), this hook seems to be called *before* the body text variable (OutputPage::$mBodytext) is populated - which means that you can prepend text (ironically, both $output->prependHTML() and $output->addHTML() do this), but you can't append text, and you can't modify the original text. If this hook were called after $mBodytext is generated, I believe all three of these could be done.

In DifferenceEngine.php, the hook is called after the main diff display is generated, but before the "Revision as of ..." part is added - and, I would think, if anything, that the hook should be modifying that second part. (No?)

So, I think this hook needs to be fixed. Of course, a simpler solution, at least for now, is just to de-deprecate ArticleAfterFetchContentObject... but maybe there's some good reason not to do that.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 27 2019, 6:15 PM
Tgr added a subscriber: Tgr.Oct 28 2019, 3:33 AM

I believe that just adding $this->mOldid as a new third argument in the DifferenceEngine.php call will fix this problem.

Yeah, sorry, that's a bug.
Do you want to submit a patch?

The potentially more difficult problem is that it doesn't seem like ArticleRevisionViewCustom really allows for modifying the page display

ArticleRevisionViewCustom is the MCR version of ArticleContentViewCustom (and has probably inherited all problems that hook had). As far as I can tell they work identically wrt modifying output.

That said, I don't really understand the problem. These hooks allow you to render the output manually, instead of calling the parser to do it. What text would you want to prepend or postpone?

Of course, a simpler solution, at least for now, is just to de-deprecate ArticleAfterFetchContentObject... but maybe there's some good reason not to do that.

Hooks that allow a complex object to be replaced with another complex object are almost always liabilities, there are so many things that can go wrong if the hook author gets creative.

Alright, I'll submit a patch for that DifferenceEngine.php bug.

My two extensions, Approved Revs and "Diagrams" (that name might change), use that hook for two different things. Approved Revs uses it simply to display a blank page. AR has a setting so that, for pages that have no approved revision, what is displayed by default to users is a blank page.

As for Diagrams: it defines namespaces that hold diagram definitions. Right now it defines three such namespaces: "BPMN:", "Gantt:" and "Mermaid:". When a user goes to any page in any of these namespaces, they are shown the diagram itself at the top, followed by the definition below it, wrapped in a <pre> tag. You can see an example here:

http://discoursedb.org/wiki/BPMN:Test_diagram

It's somewhat similar to what Scribunto does with modules.

For Diagrams, is there a better way to do what I'm trying to do? Define a new ContentHandler class for each namespace, maybe? If so, is there any documentation on what exactly needs to be done?

And the same question applies to Approved Revs. What's the best way to blank a page?

Tgr added a comment.Oct 28 2019, 5:51 AM

For Diagrams, is there a better way to do what I'm trying to do? Define a new ContentHandler class for each namespace, maybe? If so, is there any documentation on what exactly needs to be done?

ContentHandler would be the way to go, yes. It has a wiki page and reasonably good phpdoc. (There is also docs/contenthandler.txt, no idea if that's up to date.) Basically you define a new Content and a new ContentHandler, and a new SlotDiffRenderer if you want to support some kind of visual diffing, register the content handler for the given content type, and implement Content::getParserOutput for HTML rendering (and then if any number of other methods if you want - custom search content, custom transclusion logic, secondary updates if your diagrams can import other diagrams, custom 3-way merge logic for edit conflicts etc. etc.).

And the same question applies to Approved Revs. What's the best way to blank a page?

ArticleRevisionViewCustom would be appropriate for that. It's basically a replacement for Content::getParserOutput for extensions which want to handle all content types, not just the ones defined by themselves. Just returning false from the hook and not doing anything else should be enough to blank the page, like it was with ArticleContentViewCustom. (There is not much difference between the old and new hook, other than a revision cannot necessarily be represented by a single content object now.)

@Tgr - thanks, that's helpful. It's great to know that everything I need can be accomplished with this new hook. I submitted a patch for the DifferenceEngine.php bug here:

https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/546473/

(I don't know why this bug report didn't get tagged, because I put the Phabricator ID there.)

Anyway, that bug unfortunately means that I can't use this new(-ish) hook any time soon. So I guess I'll stick with the ArticleAfterFetchContentObject hook for the time being - it's the least amount of work for me.

By the way, is there any way to delay the removal from MediaWiki of the two now-deprecated hooks? I don't know when these hooks would normally be removed, but given that their replacement is not yet usable, it would be great to delay it... maybe with a comment in the code or something?

Change 546473 had a related patch set uploaded (by Jforrester; owner: Yaron Koren):
[mediawiki/core@master] Fix for ArticleRevisionViewCustom hook in DifferenceEngine.php

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

Change 546473 merged by jenkins-bot:
[mediawiki/core@master] Fix for ArticleRevisionViewCustom hook in DifferenceEngine.php

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

Change 547697 had a related patch set uploaded (by Jforrester; owner: Yaron Koren):
[mediawiki/core@REL1_34] Fix for ArticleRevisionViewCustom hook in DifferenceEngine.php

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

Change 547698 had a related patch set uploaded (by Jforrester; owner: Yaron Koren):
[mediawiki/core@REL1_33] Fix for ArticleRevisionViewCustom hook in DifferenceEngine.php

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

Change 547699 had a related patch set uploaded (by Jforrester; owner: Yaron Koren):
[mediawiki/core@REL1_32] Fix for ArticleRevisionViewCustom hook in DifferenceEngine.php

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

Change 547698 merged by jenkins-bot:
[mediawiki/core@REL1_33] Fix for ArticleRevisionViewCustom hook in DifferenceEngine.php

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

Change 547699 merged by jenkins-bot:
[mediawiki/core@REL1_32] Fix for ArticleRevisionViewCustom hook in DifferenceEngine.php

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

Change 547697 merged by jenkins-bot:
[mediawiki/core@REL1_34] Fix for ArticleRevisionViewCustom hook in DifferenceEngine.php

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

Backported to REL1_32/3/4; will ship in 1.34.0 and the next releases of the 1.32.x and 1.33.x branches. Thanks for the fix.

I just updated Approved Revs to use the ArticleRevisionViewCustom hook for new-ish versions of MediaWiki. I plan to do the same thing for my (still-unreleased) diagrams extension.

So, I guess this task can be closed...

Jdforrester-WMF closed this task as Resolved.Dec 3 2019, 4:01 PM
Jdforrester-WMF assigned this task to Yaron_Koren.