Page MenuHomePhabricator
Create Task
Maniphest T252760

Decouple wgValidSkinNames from underlying class
Closed, ResolvedPublic
Actions

Description

It would be useful if ValidSkinNames accepted ObjectFactory notation. Currently it only accepts "displayname => Class suffix" such as

	"ValidSkinNames": {
		"monobook": "MonoBook"
	},

Which means no namespacing or other "complex" setup.

	/** @suppress PhanTypeInvalidCallableArraySize - https://github.com/phan/phan/issues/1648 */
	'SkinFactory' => function ( MediaWikiServices $services ) : SkinFactory {
		$factory = new SkinFactory( $services->getObjectFactory() );

		$names = $services->getMainConfig()->get( 'ValidSkinNames' );

		foreach ( $names as $name => $skin ) {
			$factory->register( $name, $skin, [
				'class' => "Skin$skin"
			] );
		}
		// Register a hidden "fallback" skin
		$factory->register( 'fallback', 'Fallback', [
			'class' => SkinFallback::class
		] );
		// Register a hidden skin for api output
		$factory->register( 'apioutput', 'ApiOutput', [
			'class' => SkinApi::class
		] );

		return $factory;
	},

Simplest way forward seems to be replacing

		foreach ( $names as $name => $skin ) {
			$factory->register( $name, $skin, [
				'class' => "Skin$skin"
			] );
		}

with something like (noting this won't work due to the display name needing to come from somewhere for is_array( $skin )

			foreach ( $names as $name => $skin ) {
				if ( is_array( $skin ) ) {
					$spec = $skin;
					//$displayName = ??;
				} else {
					$displayName = $skin;
					$spec = [
						'class' => "Skin$skin"
					];
				}
				$factory->register( $name, $displayName, $spec );
			}

Can we just add a display name to the objectfactory spec... And leave it? Or unset it when setting up here?

			foreach ( $names as $name => $skin ) {
				if ( is_array( $skin ) ) {
					$spec = $skin;
					$displayName = $skin['DisplayName'] ?? $name;
				} else {
					$displayName = $skin;
					$spec = [
						'class' => "Skin$skin"
					];
				}
				$factory->register( $name, $displayName, $spec );
			}

Details

SubjectRepoBranchLines +/-
skins: Allow $wgValidSkinNames to take ObjectFactory specmediawiki/coremaster+35 -9
Customize query in gerrit

Related Objects

Event Timeline

Reedy created this task.May 14 2020, 11:03 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 14 2020, 11:03 AM
Reedy claimed this task.May 14 2020, 11:03 AM
Mainframe98 subscribed.May 14 2020, 11:05 AM
Reedy updated the task description. (Show Details)May 14 2020, 11:06 AM
Reedy added a project: Platform Engineering.May 14 2020, 11:14 AM
Reedy updated the task description. (Show Details)
Reedy updated the task description. (Show Details)May 14 2020, 11:18 AM
gerritbot added a comment.May 14 2020, 11:20 AM

Change 596416 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@master] Allow $wgValidSkinNames to take ObjectFactory spec

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

gerritbot added a project: Patch-For-Review.May 14 2020, 11:20 AM
Mainframe98 added a comment.May 14 2020, 11:23 AM

The documentation for SkinFactory::register says that $displayName is For backwards-compatibility with old skin loading system. I don't know exactly what it is referring to (the old autoloading skin system?) , but given that all new skins use extension registration (and really should be providing skinname-NAME), perhaps we can deprecate it?

Reedy added a comment.May 14 2020, 11:48 AM
In T252760#6136550, @Mainframe98 wrote:

The documentation for SkinFactory::register says that $displayName is For backwards-compatibility with old skin loading system. I don't know exactly what it is referring to (the old autoloading skin system?) , but given that all new skins use extension registration (and really should be providing skinname-NAME), perhaps we can deprecate it?

I'm presuming something to do with this change in 1.18

* Skin names are no longer created based on a ucfirst version of the key in
  $wgValidSkinNames but now the value. This means for
  $wgValidSkinNames["monobook"] = "MonoBook"; the skin loader will no longer try
  loading SkinMonobook and will instead load SkinMonoBook.
Reedy added a comment.May 14 2020, 12:22 PM

Quoting the full doc for reference

	 * @param string $name Internal skin name. Should be all-lowercase (technically doesn't have
	 *     to be, but doing so would change the case of i18n message keys).
	 * @param string $displayName For backwards-compatibility with old skin loading system. This is
	 *     the text used as skin's human-readable name when the 'skinname-<skin>' message is not
	 *     available. It should be the same as the skin name provided in $wgExtensionCredits.
	 * @param array|callable $spec Callback that takes the skin name as an argument, or
	 *     object factory spec specifying how to create the skin

I'm honestly not sure if that is even correct these days

Method
    getSkinNames
Found usages  (5 usages found)
    Method call  (4 usages found)
        mediawiki  (4 usages found)
            core/includes/skins  (2 usages found)
                Skin.php  (1 usage found)
                    Skin  (1 usage found)
                        getSkinNames  (1 usage found)
                            60 return $skinFactory->getSkinNames();
                SkinFallbackTemplate.php  (1 usage found)
                    SkinFallbackTemplate  (1 usage found)
                        buildHelpfulInformationMessage  (1 usage found)
                            47 $enabledSkins = $skinFactory->getSkinNames();
            core/tests/phpunit/unit/includes/skins  (1 usage found)
                SkinFactoryTest.php  (1 usage found)
                    SkinFactoryTest  (1 usage found)
                        testGetSkinNames  (1 usage found)
                            133 $names = $factory->getSkinNames();
            extensions/VisualEditor/includes  (1 usage found)
                VisualEditorDesktopArticleTargetInitModule.php  (1 usage found)
                    VisualEditorDesktopArticleTargetInitModule  (1 usage found)
                        getMessages  (1 usage found)
                            34 foreach ( $services->getSkinFactory()->getSkinNames() as $skname => $unused ) {
    Usage in comments  (1 usage found)
        mediawiki  (1 usage found)
            core/tests/phpunit/unit/includes/skins  (1 usage found)
                SkinFactoryTest.php  (1 usage found)
                    SkinFactoryTest  (1 usage found)
                        126 * @covers SkinFactory::getSkinNames

There is SkinFallbackTemplate::buildHelpfulInformationMessage() (which feels related to T178118: SkinFallback guess skin keys from skin name), but that doesn't use the value part of the displayname

	/**
	 * Returns an associative array of:
	 *  skin name => human readable name
	 *
	 * @return array
	 */
	public function getSkinNames() {
		return $this->displayNames;
	}

While the displayname part has small impact on this patch, it doesn't depend on it...

Looking around, it does seem the code (eventually, from a Skin::getSkinNames() call) does some sanity checking, like in the API:

		foreach ( Skin::getSkinNames() as $name => $displayName ) {
			$msg = $this->msg( "skinname-{$name}" );
			$code = $this->getParameter( 'inlanguagecode' );
			if ( $code && $languageNameUtils->isValidCode( $code ) ) {
				$msg->inLanguage( $code );
			} else {
				$msg->inContentLanguage();
			}
			if ( $msg->exists() ) {
				$displayName = $msg->text();
			}

But AFAIK, MW doesn't enforce the existence of the skin-skinname message, so that would probably need doing first

Reedy mentioned this in T252765: Enforce the existence of `skin-$skinName` message.May 14 2020, 12:26 PM
DannyS712 subscribed.May 14 2020, 3:04 PM
CCicalese_WMF edited projects, added Platform Team Workboards (External Code Reviews); removed Platform Engineering.May 14 2020, 3:35 PM
Krinkle added projects: Core Platform Team Initiatives (Decoupling (CDP2)), MediaWiki-User-Interface.May 16 2020, 1:53 AM
Krinkle moved this task from Backlog to Skinning system on the MediaWiki-User-Interface board.
gerritbot added a comment.May 16 2020, 2:17 AM

Change 596416 merged by jenkins-bot:
[mediawiki/core@master] skins: Allow $wgValidSkinNames to take ObjectFactory spec

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

ReleaseTaggerBot added a project: MW-1.35-notes (1.35.0-wmf.34; 2020-05-26).May 16 2020, 3:00 AM
Jdlrobson added a project: MediaWiki-Core-Skin-Architecture.May 27 2020, 10:30 PM
Jdlrobson moved this task from Backlog to Phase 1 - simplify hook contracts on the MediaWiki-Core-Skin-Architecture board.May 28 2020, 11:32 PM
Jdlrobson moved this task from Phase 1 - simplify hook contracts to Backlog on the MediaWiki-Core-Skin-Architecture board.May 29 2020, 10:11 PM
Jdlrobson moved this task from Backlog to Phase 3 - standardise template data on the MediaWiki-Core-Skin-Architecture board.Jun 5 2020, 8:38 PM
Jdlrobson subscribed.Jun 15 2020, 9:56 PM

Is there anything remaining here?

Reedy closed this task as Resolved.Jun 15 2020, 9:56 PM

Nope, I don't think so :)

Samwilson subscribed.Aug 25 2020, 2:00 AM

Does the documentation at https://www.mediawiki.org/wiki/Manual:$wgValidSkinNames need to be updated?

Reedy added a comment.Aug 25 2020, 12:21 PM
In T252760#6408075, @Samwilson wrote:

Does the documentation at https://www.mediawiki.org/wiki/Manual:$wgValidSkinNames need to be updated?

Yup! Looks like I missed it

Does https://www.mediawiki.org/w/index.php?title=Manual%3A%24wgValidSkinNames&type=revision&diff=4054992&oldid=4017654 cover it now?

Samwilson added a comment.Aug 25 2020, 9:43 PM

Looks great, thanks!

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits