Page MenuHomePhabricator

Deprecate the SkinTemplateOutputPageBeforeExec hook
Closed, ResolvedPublic2 Estimated Story Points

Description

NOTE: For developers needing to update code using the SkinTemplateOutputPageBeforeExec hook please see subtasks and this guide

This hook is very generic and very dangerous adding a lot of risk to support for legacy and modern Vector - it allows any extension to redefine template variables in a skin and as a result change the behaviour of a skin. In the past we've run into various problems in the skin in MobileFrontend where language_urls in the context of our skin means a list of languages but that seems to differ from core.

I believe we should understand the existing uses of it and provide more generic hooks to guide usage.

e.g. in the case of languages a SkinAddLanguageUrl hook would be better as this would also change the return result of getLanguages.

https://codesearch.wmflabs.org/search/?q=SkinTemplateOutputPageBeforeExec&i=nope&files=&repos=

Acceptance criteria

https://www.mediawiki.org/wiki/Category:SkinTemplateOutputPageBeforeExec_extensions

Current users of SkinTemplateOutputPageBeforeExec hook per https://codesearch.wmflabs.org/ (as of 2018-01-30)

  • BreadCrumbs2
    • $tpl->set( 'subtitle', .. )
    • $template->data['sidebar'][ .. ] = ..
    • $wgLogo = ..
  • CookieWarning
    • $tpl->data['headelement'] .= ..
  • ExtraLanguageLink
    • $sk->getOutput()->getProperty( 'extralanguagelinks' )
    • $tpl->get( 'language_urls' )
    • $tpl->set( 'language_urls', .. )
  • StickToThatLanguage
    • $tpl->set( 'language_urls',.. )
  • Wikibase
    • $template->set( 'language_urls', [] )
    • $template->set( 'wbeditlanglinks, .. )
  • InterlanguageExtension
    • $template->data['language_urls'][] = ..
  • BlueSpiceFoundation, BlueSpiceReaders, BlueSpiceAuthors
    • $template->data['bs_..'][] = ..
  • HitCounters
    • $tpl->get( 'footerlinks' )
    • $tpl->set( 'footerlinks', .. ), $tpl->set( 'viewcount', .. )
  • MobileFrontend
    • $tpl->data['footerlinks']
    • $tpl->set( 'mobileview', .. )
    • $tpl->set( 'footerlinks', .. )
    • $tpl->set( 'desktop-toggle', .. ), $tpl->set( 'mobile-license', .. ), $tpl->set( 'privacy', .. ),$tpl->set( 'terms-use', .. )
  • SharedHelpPages
    • $tpl->set( 'copyright', .. )
  • UniversalLanguageSelector
    • $template->get( 'language_urls' ), $template->set( 'language_urls', .. )
  • WhoIsWatching
    • $tpl->set( 'numberofwatchingusers', .. )
  • Wikidata.org
    • $template->data['footerlinks'][] = ..,$template->set( .. )
  • WikidataPageBanner
    • $tpl->set( 'prebodyhtml', .. ), $tpl->get( 'prebodyhtml' )
  • WikimediaMessages
    • $template->data['footerlinks'][] = ..,$template->set( .. )

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I've reviewed the remaining usages of SkinTemplateOutputPageBeforeExec and documented them in T252725. To set some expecations, I'll be opening specific tasks and submitting patches for impacted extensions soon.

In the case of the AddFooterLink hook, I need to make some changes to skins first to support the transition. T251817
I am aiming to deprecate the hook inside 1.36 at latest.

Change 596482 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Deprecate SkinTemplateOutputPageBeforeExec hook

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

Change 596491 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Minerva should source mobile license without indirection

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

Change 596492 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Drop SkinTemplateOutputPageBeforeExec hook usage prior to deprecation

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

Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)

Change 597385 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/BlogPage@master] Don't try to suppress categories

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

Change 597385 merged by jenkins-bot:
[mediawiki/extensions/BlogPage@master] Don't try to suppress categories

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

Change 596491 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Minerva should source mobile license without indirection

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

Change 596492 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Drop SkinTemplateOutputPageBeforeExec hook usage prior to deprecation

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

Change 596482 had a related patch set uploaded (by Ammarpad; owner: Jdlrobson):
[mediawiki/core@master] Deprecate SkinTemplateOutputPageBeforeExec hook

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

Jdlrobson raised the priority of this task from Medium to High.Jun 16 2020, 6:40 PM

Change 596482 merged by jenkins-bot:
[mediawiki/core@master] Deprecate SkinTemplateOutputPageBeforeExec hook

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /596482

I noticed a strange #footer-places-terms-use { float: left; } style in the footer, which has been introduced in gerrit 596492.

It is an inelegant solution to make the 'Terms of Use" link display first (though, I agree with this ordering).

A better approach would be, in MobileFrontendHooks.php, to replace:

if ( $terms ) {
    $footerLinks['terms-use'] = $terms;
}

with:

if ( $terms ) {
    // We want the "terms-use" item to be iterated first
    $footerLinks = [ 'terms-use' => $terms ] + $footerLinks;
}

So that, in SkinComponentFooter.php, this item is iterated first.

I use the "+" operator rather than array_merge(), so the left array has precedence (if ever 'terms-use' already exists), which makes more sense, and matches the current code.

Mmmm, it wouldn't work this way, as $newItems is an empty array, that is populated by onSkinAddFooterLinks(), then back in getTemplateDataFooter() is merged to $data (using the foreach).

Still, I think prepending the item PHP-side is the proper approach.

Solutions I've been thinking of:

  • Do the prepend in getTemplateDataFooter(): if ( $index === 'terms-use' ) { /* prepend */ }. Very unclean, though.
  • In onSkinAddFooterLinks() definition, replace $newItems with $appendItems (reuse 3rd parameter) and $prependItems (new 4th parameter, optional).
    • Edit: but we would also have to update the interface, and it turn, it would break all the other existing implementations…

In order to be able to prepend the item, I was considering to pass $data to the onSkinAddFooterLinks hook, instead of generating a $newItems that is then merged into $data.

But the problem is that the onSkinAddFooterLinks hook returns (and receives) an array of "key => html string" items, whereas getTemplateDataFooter() and (the private methods it uses) provides arrays of "key => ['id' => ..., 'html' => ...]" or "key => ['id' => ..., 'text' => ..., 'href' => ...]" items.

Change #1105977 had a related patch set uploaded (by Gerrit Patch Uploader; author: Anne Haunime):

[mediawiki/core@master] Ensure the "Terms of Use" footer link added by MobileFrontend is iterated first

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

Change #1105978 had a related patch set uploaded (by Gerrit Patch Uploader; author: Anne Haunime):

[mediawiki/extensions/MobileFrontend@master] Remove hack to display "Terms of Use" first in footer links

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