Page MenuHomePhabricator

Suggest renaming "MobileFrontendSkinHooks.php" to "MobileFrontendSkin.php" or something better
Open, MediumPublic

Description

I've had a look at MobileFrontendSkinHooks.php and extension.json, I see that, there are really no hook handlers in MobileFrontendSkinHooks.php. From our extension repos, anything that end with Hooks.php should have hook handlers right? Or are my missing something?

I'm suggestion we rename MobileFrontendSkinHooks.php and also, from usage: https://codesearch.wmcloud.org/search/?q=MobileFrontendSkinHooks&i=nope&files=&repos= of this file, I see the only external repo affect would be MinervaNeue skin (per CS).

Not sure if this rename makes sense so I decided to bring it on Phab for discussion before I work on this.

Event Timeline

@Jdlrobson, any reason why the file was named like this? Did it have MF skin related hooks in the past?

All hook handlers should be inside includes/MobileFrontendHooks.php
I think the original intention was to keep MobileFrontendHooks.php as a class containing the hooks and no private methods but over time that seems to have failed - MobileFrontendHooks::enableMediaWikiUI for example.

MobileFrontendSkinHooks.php contains helper methods that hooks inside MobileFrontendHooks.php can use that operate on skins.

It would be nice to clean up both of these files and tie them to more meaningful classes. For example MobileFrontendSkinHooks::interimTogglingSupport should probably be a public method on includes/Transforms/MakeSectionsTransform.php since it relates to that code and gradeCImageSupport should be on includes/Transforms/LazyImageTransform.php

The methods getTermsLink, getPluralLicenseInfo, getLicense, getDesktopViewLink, getMobileViewLink , getLicenseText all relate to the footer and is only used inside MobileFrontendHooks::onSkinAddFooterLinks so having a new class with a name like Hooks\MobileSkinAddFooterLinks and then running execute/runHook inside MobileFrontendHooks::onSkinAddFooterLinks would make more sense there like so:

public static function onSkinAddFooterLinks( Skin $skin, string $key, array &$footerLinks ) {
    $hook = new Hooks\MobileSkinAddFooterLinks();
    $hook->runHook( $skin, $key, &$footerLinks );
}

Is that helpful guidance? Let me know if anything doesn't make sense

That's makes sense. I'll break this into 2 sub-tasks then:

  1. Move Transforms related code from MobileFrontendSkinHooks;
  2. Rename & refactor MobileFrontendSkinHooks.

Thanks for the guidance :)

Jdlrobson triaged this task as Medium priority.Oct 2 2020, 8:42 PM
Jdlrobson moved this task from Incoming to Triaged but Future on the Web-Team-Backlog-Archived board.