Page MenuHomePhabricator

[Dev] Drop VectorTemplate usage in Vector
Open, MediumPublic

Description

Minerva and Vector are duplicating code in various places. If left unchecked that's going to be a lot of additional code we need to maintain after the project has completed. Change now will help us reduce time wasted maintaining and updating skins in future. For example right now adding a link to the footer will require changes in two places rather than one!

Now we are using Mustache, VectorTemplate has become a bit of redundant. There are many problems with the existing skin architecture, in particular the relationship between VectorTemplate and SkinVector. This adds unnecessary confusion and indirection.

Among the problems:

  • there is lots of indirection (and about 900 lines of code inherited),
  • The naming! VectorTemplate extends BaseTemplate (which then extends QuickTemplate) and SkinVector extends SkinTemplate. VectorTemplate DOES NOT extend SkinTemplate!
  • There is no longer a benefit to set and get when you are using Mustache
  • BaseTemplate and QuickTemplate come with extra public APIs that the subclass (VectorTemplate) must not break. Notably, /** @var array */ public $data;, This makes many refactors dangerous and challenging.

Move Vector away from VectorTemplate with some minimal changes in MediaWiki core.

Changes needed:

  • Several functions in includes/skins/BaseTemplate.php would be moved to static functions so they can be used directly in SkinVector.
  • SkinTemplate::setupTemplateForOutput currently does a lot of initialisation of private class variables. I'd like to move those to the constructor
  • An html method will be pulled out and used by outputPage to allow a Skin to not use a BaseTemplate
  • (Optional) a few helper classes will be added to SkinTemplate to refactor the very long prepareQuickTemplate function
  • Slowly methods and properties will be migrated safely to the new SkinVectorTemplate class to mitigate risk. e.g. https://gerrit.wikimedia.org/r/597133
  • VectorTemplate will be renamed SkinTemplateVector and made to extend SkinVector. Some deprecated hooks may need to be run with a FallbackTemplate for compatibility with existing known extensions while we continue to deprecate them. https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/Vector/+/593070/
  • A single PHP class SkinTemplateVector. This will be an intentional breaking change breaking compatibility with any 3rd party skins that are extending SkinVector.

A proof of concept is available for core and Vector

Event Timeline

Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptApr 28 2020, 12:06 AM

Change 592791 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Refactor skin code to allow us to move away from SkinTemplate

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

Several functions in includes/skins/BaseTemplate.php would be moved to static functions so they can be used directly in SkinVector.

If at all possible, I think these would be better moved to Skin, rather than a static methods on a not-so-meaningful helper class.

Niedzielski updated the task description. (Show Details)Apr 28 2020, 1:11 AM
Niedzielski updated the task description. (Show Details)Apr 28 2020, 1:14 AM
ovasileva triaged this task as Medium priority.Apr 28 2020, 2:08 PM
Jdlrobson renamed this task from Drop VectorTemplate usage in Vector to [Dev] Drop VectorTemplate usage in Vector.Apr 28 2020, 5:18 PM
Jdlrobson updated the task description. (Show Details)

Change 593013 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Add buildSidebar method

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

Change 593020 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Refactor: Remove indirection

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

matmarex removed a subscriber: matmarex.Apr 28 2020, 6:44 PM

Change 593039 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] [skins] Separate rendering from setup

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

Change 593040 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] [skin] Move static functions from BaseTemplate to Skin

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

Change 593013 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Add buildSidebar method

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

Change 593069 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Use consistent function names to SkinVector

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

Change 593070 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Begin merge of SkinVector and VectorTemplate

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

Change 592790 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Big switchermovearoo

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

Change 593069 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Use consistent function names to SkinVector

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

Change 593039 merged by jenkins-bot:
[mediawiki/core@master] SkinTemplate: Separate rendering from setup

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

Change 593326 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] [skin] Refactor prepareQuickTemplate

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

Change 593326 merged by jenkins-bot:
[mediawiki/core@master] skins: Refactor and split up SkinTemplate::prepareQuickTemplate

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

Change 593931 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Tests: Always set Skin

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

Change 593931 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Tests: Always set Skin

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

Change 594285 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Merge BaseTemplate:getFooterIcons into SkinTemplate:getFooterIcons

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

Change 593020 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Refactor: Remove indirection (where alternatives exist)

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

Change 593040 merged by jenkins-bot:
[mediawiki/core@master] skins: Move some BaseTemplate functions to Skin

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

Change 595662 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] BaseTemplate:makeListItem is deprecated

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

Change 594285 merged by jenkins-bot:
[mediawiki/core@master] BaseTemplate: Deprecate getFooterIcons

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

Jdlrobson moved this task from Inbox to OKR Backlog on the User-Jdlrobson board.May 14 2020, 4:57 PM

Change 597133 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Avoid using get indirection in VectorTemplate

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

Jdlrobson updated the task description. (Show Details)May 18 2020, 7:19 PM

Change 595662 merged by jenkins-bot:
[mediawiki/skins/Vector@master] BaseTemplate:makeListItem is deprecated

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

Change 598843 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Model indicators as their own template rather than a block of HTML

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

I know this hasn't been estimated but this is a case of needing to reflect reality - I am working on this one.

Change 597133 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Avoid using get indirection in VectorTemplate

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

Change 598843 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Model indicators as their own template rather than a block of HTML

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

Change 602146 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Model indicators as their own template rather than a block of HTML

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

Change 602184 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Move html-printtail template variable to SkinVector

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

Volker_E updated the task description. (Show Details)Jun 4 2020, 4:04 AM
Volker_E updated the task description. (Show Details)Jun 4 2020, 4:11 AM
Jdlrobson updated the task description. (Show Details)Jun 4 2020, 3:09 PM

Change 602184 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Move html-printtail template variable to SkinVector

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

Change 602146 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Model indicators as their own template rather than a block of HTML

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

Jdlrobson moved this task from OKR Backlog to Doing on the User-Jdlrobson board.Jun 9 2020, 11:43 PM

Progress where is blocked on language engineering and Wikimedia Germany right now. We have a meeting with the former next week.

Change 610868 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Merge SkinVector and SkinTemplateVector (1/2)

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

Change 610869 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Merge SkinVector and SkinTemplateVector (2/2)

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

Change 592790 abandoned by Jdlrobson:
[mediawiki/skins/Vector@master] Move SkinVector methods to SkinTemplateVector

Reason:
See https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/ /610868

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

Change 610868 abandoned by Jdlrobson:
[mediawiki/skins/Vector@master] Merge SkinVector and SkinTemplateVector (1/2)

Reason:
See https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/ /610869

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