Page MenuHomePhabricator

ResourceLoaderOOUIIconPackModule doesn't allow mixing of images and icons
Open, LowPublic

Description

When I create a module

		"skins.minerva.mainMenu.icons": {
			"class": "ResourceLoaderOOUIIconPackModule",
			"variants": [],
			"selectorWithoutVariant": ".mw-ui-icon-minerva-{name}:before",
			"defaultColor": "#4A4F53",
			"useDataURI": false,
			"icons": [ "userContributions" 	 ],
			"images": {
				"login": {
					"file": {
						"ltr": "resources/skins.minerva.mainMenu.icons/logIn-ltr.svg",
						"rtl": "resources/skins.minerva.mainMenu.icons/logIn-rtl.svg"
					}
				},
				"home": "resources/skins.minerva.mainMenu.icons/home.svg",
				"logout": {
					"file": {
						"ltr": "resources/skins.minerva.mainMenu.icons/logOut-ltr.svg",
						"rtl": "resources/skins.minerva.mainMenu.icons/logOut-rtl.svg"
					}
				},
				"nearby": "resources/skins.minerva.mainMenu.icons/nearby.svg",
				"random": "resources/skins.minerva.mainMenu.icons/random.svg",
				"settings": "resources/skins.minerva.mainMenu.icons/settings.svg",
				"watchlist": "resources/skins.minerva.mainMenu.icons/watchlist.svg"
			}
		},

I get no complaints that icons and images can be mixed, but the files in images cause the following issue as they are assumed to be in core:

[f17d98ebb5c9164e792b9a01] /w/load.php?modules=skins.minerva.mainMenu.icons&image=settings&format=rasterized&skin=fallback&version=1s0p7 MWException from line 294 of /Users/jrobson/git/core/includes/resourceloader/ResourceLoaderImage.php: File '/Users/jrobson/git/core/resources/skins.minerva.mainMenu.icons/settings.svg' does not exist
Backtrace:

#0 /Users/jrobson/git/core/includes/resourceloader/ResourceLoader.php(1068): ResourceLoaderImage->getImageData(ResourceLoaderContext)
#1 /Users/jrobson/git/core/includes/resourceloader/ResourceLoader.php(791): ResourceLoader->makeModuleResponse(ResourceLoaderContext, array, array)
#2 /Users/jrobson/git/core/load.php(46): ResourceLoader->respond(ResourceLoaderContext)
#3 {main}

In this situation we should either forbid the mixing of the values of update the path for the files (the latter would be very helpful for migrating ResourceLoaderImageModules which have icons that are not yet in OOUI!)

Event Timeline

Restricted Application added subscribers: Masumrezarock100, Aklapper. · View Herald Transcript

Have you worked around the issue? How important is this for you?

This is useful for T244444 which helps standardisation efforts as well as reducing our RL module burden.

We can work around it by adding new ResourceLoader modules but that adds caching concerns and bloats the startup module.

Having this capability would speed up the removal of several of these ResourceLoaderImage modules in our product which would lead to a smaller startup script so may be of interest to performance team from that perspective.

Gilles added a subscriber: matmarex.

@matmarex it seems like you built this specialised RL class originally, would you be able to take care of this?

@matmarex it seems like you built this specialised RL class originally, would you be able to take care of this?

I can't deny that, unfortunately, but I don't think I will have the time to work on this.

FWIW I never intended to allow using images in ResourceLoaderOOUIIconPackModule, it's supposed to only be used to create a "sub-pack" of existing OOUI icons, rather than define new ones. I should have made it throw an exception.

You can instead use ResourceLoaderOOUIImageModule to define new icons.

Alternatively, maybe you can use ResourceLoaderFilePath objects instead of strings for file paths (you'd have to define the module in PHP code rather than in JSON), to avoid the wrong base path being added, but if you do, don't ever ask me to look at that code :)

JTannerWMF added a subscriber: JTannerWMF.

It looks like @matmarex said he isn't able to work on this task and provided a workaround, so I am moving this to the freezer. Please ping me if this is a problem.