Page MenuHomePhabricator

Enforce the existence of `skin-$skinName` message
Open, Needs TriagePublic

Description

Forking from T252760: Decouple wgValidSkinNames from underlying class

There doesn't seem to be any obvious enforcement of skinname-$skinName message existing, and various bits of code seem to check for its existence, before falling back to the "display name" part of $wgValidSkinNames (the value)

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

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

The "display name" part is also used in the instantiation of the skin instance (in a very strict way), so T252760: Decouple wgValidSkinNames from underlying class will add more flexibility there

Event Timeline

Reedy created this task.May 14 2020, 12:26 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 14 2020, 12:26 PM

Is this still possible? If so could you give me a sample skin.json to understand it? I can't find the code this bug is pointing to and I know we've recently made some changes to SkinFactory.

Reedy updated the task description. (Show Details)Tue, Jan 5, 11:09 PM
Reedy added a comment.Tue, Jan 5, 11:16 PM

Is this still possible? If so could you give me a sample skin.json to understand it? I can't find the code this bug is pointing to and I know we've recently made some changes to SkinFactory.

Is what still possible?

I'm not sure what use a sample skin.json file will tell you.

This is basically a request for structure type test.

If we look at DefaultPreferencesFactory:

		// Display the installed skin the user has specifically requested via useskin=….
		$useSkin = $context->getRequest()->getRawVal( 'useskin' );
		if ( isset( $allInstalledSkins[$useSkin] )
			&& $context->msg( "skinname-$useSkin" )->exists()
		) {
			$validSkinNames[$useSkin] = $useSkin;
		}

it's looking for skinname- prefixed message.

SkinFactory

	/**
	 * Map of name => fallback human-readable name, used when the 'skinname-<skin>' message is not
	 * available
	 *
	 * @var array
	 */
	private $displayNames = [];

and from register()

	 * @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.

There's various code paths that do this sort of existence check, but we should just enforce skins to have the message defined in the first place

The code referenced in the description has changed a bit, but is functionally still the same in ServiceWiring.php; it just changed for the change attached to T252760: Decouple wgValidSkinNames from underlying class.

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

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

Change 654545 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Skins using factory must define name.

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

Change 654545 abandoned by Jdlrobson:
[mediawiki/core@master] Skins using factory must define name.

Reason:
A bit more complicated then I first realized. It seems the message parser creates an instance of skin which causes a cyclic dependency eek...

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