Page MenuHomePhabricator

Decouple wgValidSkinNames from underlying class
Closed, ResolvedPublic

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 );
			}

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
Reedy updated the task description. (Show Details)May 14 2020, 11:06 AM
Reedy updated the task description. (Show Details)
Reedy updated the task description. (Show Details)May 14 2020, 11:18 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

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?

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.

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

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

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

Is there anything remaining here?

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

Nope, I don't think so :)

Looks great, thanks!