Page MenuHomePhabricator

SkinMustache should extend Skin not SkinTemplate
Open, Needs TriagePublic

Description

The 3 important hooks run inside SkinTemplate should be run inside Skin on the long term so subclasses of Skin can use them.

When we introduce a SkinMustache class ideally it would subclass Skin rather than SkinTemplate but to make its introduction easier we made it extend SkinTemplate

precursors

Acceptance criteria

  • Skin::getFooterLinks is promoted from SkinTemplate.php to Skin.php. The hook SkinAddFooterLinks is not impacted by this move as it already uses the skin and currently only lives in that class for compatibility reasons.
  • buildContentNavigationUrl is promoted to SkinTemplate and The SkinTemplateNavigation::Universal hook is modified to run with a Skin instance instead of a SkinTemplate.
  • buildContentNavigationUrls is moved from SkinTemplate to Skin
  • PersonalUrls is moved from SkinTemplate to Skin and the PersonalUrls hook is modified to run with a Skin instance instead of a SkinTemplate.
  • SkinMustache extends Skin not SkinTemplate

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 17 2020, 11:17 PM
Ammarpad updated the task description. (Show Details)Sep 29 2020, 3:38 PM

Change 631681 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/core@master] Move SkinTemplate::getFooterLinks() to Skin.

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

Change 631681 merged by jenkins-bot:
[mediawiki/core@master] Move SkinTemplate::getFooterLinks() to Skin.

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

Jdlrobson updated the task description. (Show Details)Oct 2 2020, 7:49 PM

Change 631905 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/core@master] Move SkinTemplate::buildContentNavigationUrl to Skin

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

I think there's a need to discuss this more before attempting implementing

buildContentNavigationUrl is promoted to SkinTemplate
The SkinTemplateNavigation::Universal hook is modified to run with a Skin instance instead of a SkinTemplate.

As noted in review, over time this method has degenerated to almost uncontrollable logic. It's larger than many classes and even worse, it is also running four hooks, each with instance of SkinTemplate. (two are now fortunately soft-deprecated, but still supported and used by a lot of things).

  • SkinTemplatePreventOtherActiveTabs
  • SkinTemplateNavigation::SpecialPage
  • SkinTemplateNavigation
  • SkinTemplateNavigation::Universal

With the new hook system, each of them is an interface. So how will changing them to run with Skin instance work now with least disruption?

In T255319, SkinTemplateNavigation::SpecialPage and SkinTemplateNavigation are being deprecated in favor of SkinTemplateNavigation::Universal, but once all code has been migrated, SkinTemplateNavigation name will be free, and basically there's no need for the ::Universal suffix, which has its own inconveniences. For instance, apart from looking rather complicated than it's actually, it also has to differ from the actual interface name where colons are not allowed.

In my quick thought, to make this simpler. SkinTemplate::buildContentNavigationUrl(), and all that it's dragging (two private methods, plus 4 hooks ) should not be moved to Skin. Anyway it's done, it would be quite risky and prone to error.

Instead of moving, just leave it where it's now and completely recreate the method with more reasonable logic in Skin. Forget about all the SkinTemplate hooks and introduce single new hook 'SkinNavigation' to be used in the method. The method will run this hook and only it.

Two of the hooks are already deprecated, so there's no need to carry them in the new method. The other non-deprecated hook (SkinTemplatePreventOtherActiveTabs) is currently not being used by any client code. If it's still needed, reassess who need it and expose the possibility in 'SkinNavigation hook.

The only downsides of this plan as far as I can see, is that it will introduce mild inconvenience by requiring more migration after the one ongoing in T255319 especially for those who have already done so, but that is easily offset by the long-term benefit of fixing the mess of this method and those hooks. For those who have not yet migrated, there's nothing to worry about, the task can simply be stalled and let everyone know to wait to migrate to SkinNavigation completely.

After all reasonably maintained client extensions/skins have been migrated to use the new Skin method and SkinNavigation, then simply delete the SkinTemplate::buildContentNavigationUrl().

A new hook "SkinNavigation" a new skin method seems like an interesting idea but we'd want to make sure the older hooks never execute alongside the new SkinNavigation hook. One way to do that would be to use the same function name without inheritance e.g. Skin::buildContentNavigationUrl

I would suggest refactoring SkinTemplate::buildContentNavigationUrl where we can, to use any new Skin helper methods that come out of writing the new method

The only downsides of this plan as far as I can see, is that it will introduce mild inconvenience by requiring more migration after the one ongoing in T255319 espec

Perhaps but If T255319 is done we could make SkinTemplateNavigation::Universal alias to the new hook to which it will be compatible. A SkinTemplate is always a Skin, but a Skin is not always a SkinTemplate so SkinTemplateNavigation::Universal hook will be interchangeable with the SkinNavigation hook (just not the other way round).

Change 637889 had a related patch set uploaded (by Reedy; owner: Ammarpad):
[mediawiki/core@REL1_35] Move SkinTemplate::getFooterLinks() to Skin.

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

Change 637889 merged by jenkins-bot:
[mediawiki/core@REL1_35] Move SkinTemplate::getFooterLinks() to Skin.

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

Change 631905 abandoned by Ammarpad:
[mediawiki/core@master] Move SkinTemplate::buildContentNavigationUrl to Skin

Reason:
Needs new strategy

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