In preparation for skin refactoring, we should limit what skin methods can be extended. This will make it easier for us to perform refactors without breaking skins.
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | Jdlrobson | T299456 Make Skin::getPortletData private | |||
| Resolved | Jdlrobson | T305270 Menus: link-html does not work with personal tools menu |
Change 755030 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):
[mediawiki/skins/Vector@master] Overriding getPortletData will soon be deprecated
Change 751968 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):
[mediawiki/core@master] Overriding getPortletData is now deprecated
Change 755030 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Overriding getPortletData will soon be deprecated
Change 751968 merged by jenkins-bot:
[mediawiki/core@master] Overriding getPortletData is now deprecated
FemiwikiSkin and ContentTranslation skin are the remaining two callers of this method.
While investigating FemWiki I think I may have discovered a bug. The link-html key doesn't seem to be working as expected. Will continue to look into that and raise one if I get a minimum test case.
I am assuming ContentTranslation is not in production as otherwise we'd be seeing lots of errors in the logs... language team can you confirm? Patch provided to fix that.
Change 774954 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):
[mediawiki/extensions/ContentTranslation@master] Overriding getPortletData was deprecated
I am assuming ContentTranslation is not in production as otherwise we'd be seeing lots of errors in the logs... language team can you confirm?
ContentTranslation is deployed on all Wikipedias and the skin is used on Special:CX and Special:CXStats pages.
Thanks for the context @Nikerabbit. I'm not seeing any deprecation warnings in getPortletData for some reason and this function was deprecated in February. Perhaps there's a bug in the MWDebug::detectDeprecatedOverride call inside getPortletData in SkinTemplate and it only works for direct extending classes? SkinContentTranslation extends SkinMustache which extends SkinTemplate.
Change 774954 merged by jenkins-bot:
[mediawiki/extensions/ContentTranslation@master] Overriding getPortletData was deprecated in 1.38
SkinContentTranslation does not override the method, it only calls it; and the deprecation warning is only for overriding. So it seems to be working correctly to me.
(facepalm). Thanks for pointing that out. I obviously wasn't clearly thinking yesterday. We even touched on this already on https://gerrit.wikimedia.org/r/c/mediawiki/core/+/751968/8/includes/skins/SkinTemplate.php :)
Make Skin::getPortletData private and then hard deprecate it
If all usages are fixed... maybe just deprecate (and eventually remove)? I am not sure what the extra 'make private' stage will do
For now we just need to make it private. The method is still used but we've started componentizing skins. In future we will likely want to move all the menu code out into a Skins\MenuBuilder component (https://phabricator.wikimedia.org/project/view/5769/). Methods that are protected or public would need to be deprecated/removed before we build out that new class so we're trying to avoid too many of those. Does that make sense?
Change 777417 had a related patch set uploaded (by Ammarpad; author: Ammarpad):
[mediawiki/core@master] Make Skin::getPortletData private
Will review by end of week. Just waiting for feedback on https://github.com/femiwiki/FemiwikiSkin/pull/486
Change 777417 merged by jenkins-bot:
[mediawiki/core@master] Make Skin::getPortletData private