Page MenuHomePhabricator

FlaggedRevs is not compatible with TemplateStyles
Closed, ResolvedPublic

Description

Steps to reproduce:

  • visit beta dewiki as a user who can make non-pending edits (autoreviewer, editor, admin etc)
  • create a template and associated CSS file; make the last revision for both shows up in the history as "checked" or "automatically checked"
  • visit as anonymous, change the CSS
  • view the template as anonymous

Expected: the CSS from the last checked revision is used
Actual: the CSS from the latest revision is used

Details

Related Gerrit Patches:

Event Timeline

Tgr created this task.Mar 28 2018, 4:13 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 28 2018, 4:13 PM
Anomie added a subscriber: Anomie.EditedMar 28 2018, 4:47 PM

TemplateStyles uses the standard $parser->fetchCurrentRevisionOfTitle( $title ) to get the Revision of the stylesheet page.

Apparently FlaggedRevs uses only the 'BeforeParserFetchTemplateAndtitle' hook to override the logic used by the default fetcher for $parser->fetchTemplateAndTitle(), rather than using ParserOptions::setCurrentRevisionCallback() (added in MW 1.24) and ParserOptions::setTemplateCallback() (added in MW 1.12!) to override the fetchers. It should be updated.

Anomie renamed this task from TemplateStyles is not comaptible with FlaggedRevs to FlaggedRevs is not comaptible with TemplateStyles.Mar 28 2018, 4:52 PM
Tgr added a comment.Mar 28 2018, 4:57 PM

What would happen if multiple things tried to set the callback (e.g. FlaggedRevs and TemplateSandbox, or FlaggedRevs and EditPage)?

There's only the one callback. Usually code setting the callback would chain it something like

$oldCurrentRevisionCallback = $popt->setCurrentRevisionCallback(
    function ( $title, $parser = false ) use ( &$oldCurrentRevisionCallback ) {
        /* Do whatever specialness needs to happen for certain titles */

        // Nothing special, use the default handling
        return call_user_func( $oldCurrentRevisionCallback, $title, $parser );
    }
);

FlaggedRevs might want to do the opposite:

$oldCurrentRevisionCallback = $popt->setCurrentRevisionCallback(
    function ( $title, $parser = false ) use ( &$oldCurrentRevisionCallback ) {
        $rev = call_user_func( $oldCurrentRevisionCallback, $title, $parser );
        if ( $rev ) {
            // Check which revision was returned, and maybe replace it with a
            // stable revision of the same page (without reference to $title)
        }
        return $rev;
    }
);

BTW, FlaggedRevs probably has the same problem with Scribunto's mw.title:getContent(), and core parser functions such as {{PAGESIZE:Title}}, {{REVISIONID:Title}}, and so on.

XanonymusX renamed this task from FlaggedRevs is not comaptible with TemplateStyles to FlaggedRevs is not compatible with TemplateStyles.Mar 30 2018, 2:13 PM
ggellerman triaged this task as Normal priority.Apr 5 2018, 5:48 PM
MBH added a subscriber: MBH.Apr 30 2018, 3:37 PM
matmarex claimed this task.May 8 2018, 5:51 PM
matmarex added a subscriber: matmarex.

Dan says I should look into this, so let's see…

matmarex added a comment.EditedJun 19 2018, 10:27 AM

I didn't have time to do it earlier. However, it looks like an easy fix…

TemplateStyles uses the standard $parser->fetchCurrentRevisionOfTitle( $title ) to get the Revision of the stylesheet page.
Apparently FlaggedRevs uses only the 'BeforeParserFetchTemplateAndtitle' hook to override the logic used by the default fetcher for $parser->fetchTemplateAndTitle(), rather than using ParserOptions::setCurrentRevisionCallback() (added in MW 1.24) and ParserOptions::setTemplateCallback() (added in MW 1.12!) to override the fetchers. It should be updated.

Looks like we can easily replace 'BeforeParserFetchTemplateAndtitle' with setCurrentRevisionCallback(). They are called by the same function Parser::statelessFetchTemplate() (hook first, then the callback), and the only other users of getCurrentRevisionCallback() are CoreParserFunctions::getCachedRevisionObject() (used by functions like {{REVISIONID:Foo}}, so they will now use the flagged revision ID rather than the latest), Parser::getRevisionObject() (used by magic words like {{REVISIONID}}, likewise) and TemplateStyles (which should fix this task!).

It seems entirely superfluous to override setTemplateCallback(). The default handler actually calls the function Parser::statelessFetchTemplate() mentioned above, which runs the other hook/callback. I am not sure why would you ever override it at all these days, maybe it's more convenient if you want to return arbitrary text rather than coming from a revision, but getCurrentRevisionCallback() is already allowed to return a "fake" Revision object.

On the other hand though, why does TemplateStyles use $parser->fetchCurrentRevisionOfTitle( $title ) instead of $parser->fetchTemplate( $title )? Could it use that instead? It seems that some other extensions use the 'BeforeParserFetchTemplateAndtitle' hook (ApprovedRevs, Memento) and using the other function in TemplateStyles should fix compatibility with them both.

Change 441020 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/FlaggedRevs@master] Replace old and busted hook with the new hotness of a callback

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

On the other hand though, why does TemplateStyles use $parser->fetchCurrentRevisionOfTitle( $title ) instead of $parser->fetchTemplate( $title )? Could it use that instead? It seems that some other extensions use the 'BeforeParserFetchTemplateAndtitle' hook (ApprovedRevs, Memento) and using the other function in TemplateStyles should fix compatibility with them both.

$parser->fetchTemplate( $title ) returns a string. TemplateStyles needs the Content object from the revision. In the future it might use the Content object from a non-main MCR slot of the revision.

TemplateStyles also uses the revision ID in warnings, the in-process cache key, and the key for deduplication.

Deskana moved this task from Up next to Doing on the TemplateStyles board.Jun 25 2018, 2:04 PM

Should we deprecate the BeforeParserFetchTemplateAndtitle hook if it's usage is incompatible with extensions like Scribunto/TemplateStyles?

Change 441020 merged by jenkins-bot:
[mediawiki/extensions/FlaggedRevs@master] Replace old and busted hook with the new hotness of a callback

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

matmarex closed this task as Resolved.Aug 16 2018, 8:41 PM
matmarex removed a project: Patch-For-Review.