Page MenuHomePhabricator

WikidataPageBanner uses a blacklist of skin names to decide 'prebodyhtml' support instead of sane feature detection
Open, Needs TriagePublic

Description

It appears that wikidata page banner uses an (essentially) undocumented* skin api ('prebodyhtml') that is only implemented in Vector and Minerva.

*It is technically documented, but there's the strong implication it is a skin-specific hack to make VectorBeta work and not meant for general usage nor expected to be implemented by skins in general.

It uses a blacklist to use an alternative approach in all other skins that (I assume) the extension author could think of.

This is a bad approach. If nothing else, given this api basically only exists in Vector and is not documented as something skins are expected to implement, it should be using a whitelist of good skins, since basically no skins implement it.

Better would be to have some sort of class constant if the api is supported, so skins can do feature detection.

Even better would be, if the feature is actually useful and a good idea, would be to document/standardize it so that all skins are expected to implement it (What this entails in our context is debatable. At a bare minimum it should be in the official "example" skin and perhaps have a more explicit code comment in skins/SkinTemplate.php.

The original context that this came up in is that WikidataPageBanner does not seem to work in Timeless and some other skins. Arguably one could band-aid this by just adding Timeless to the blacklist, but that seems like just pushing the tech-debt down the road.

Related Objects

Event Timeline

Bawolff created this task.May 7 2019, 4:52 AM
Restricted Application added a project: Wikidata. · View Herald TranscriptMay 7 2019, 4:52 AM
Isarra added a subscriber: Isarra.EditedMay 7 2019, 2:23 PM

So @ashley looked into it, and apparently the only public skins he found that support this prebodyhtml thing are Vector, Minerva, and Metrolook.

Also, this may be a stupid question, but do we not register such apis in some way? Shouldn't it be possible to just check if one by that particular name exists?

Isarra added a subscriber: ashley.May 7 2019, 2:37 PM
phuedx added subscribers: Jdlrobson, phuedx.

Ping @Jdlrobson (the extension's author IIRC).

Patches welcome. Extension was a GSoc project and right now I'm the only one who reviews patches and patches I submit to the repo don't get code reviewed so I don't submit them myself (and I prefer to avoid self merging if I can). I've been trying to find a maintainer to mentor/code review for, but sadly nothing has come about in that area yet.

The crux of the problem here is skins do not have well defined APIs and the best we do have is hard to discover template variables (something I know that Krinkle is working towards with introducing mustache and trying to define this contract).

WikidataPageBanner by design was defined such that skins would need to make themselves compatible if they wanted to. We didn't want to burden skin designers unnecessarily with a feature that may not work and/or would need skin level support (which is why we have $wgWPBSkinBlacklist configuration option). Supporting all the skins we have in the Wikimedia universe is hard, so I wanted to share that load.

Per the code:

GetSkinTemplateOutputPageBeforeExec
Modifies the template to add the banner html for rendering by the skin. Note not
	 * all skins render the prebodyhtml template variable so in some skins this will have no impact
	 * whatsoever.

So up to you. You can add that prebodyhtml and support banners or not. Longer term, I'd hope to see some kind of documentation of all the possible values that a skin can render and their types (array, html string or text string) as a result of some sort of skin API. On hindsight it would have been better to use "wikidatapagebanner" as a key rather than use the "prebodyhtml" that already existed to prevent overloading it.

e.g.

$skin->registerTemplateValue( 'wikidatapagebanner', $value, 'string', 'An HTML string providing WikidataPageBanner')

rather than

$skin->wikidatapagebanner = $value;

so we can document how these template values are for and how they are used. T217158 should get us closer to something like that.

Isarra moved this task from Backlog to Compatibility on the Timeless board.May 14 2019, 7:43 PM

I meant to bring this up before, but I guess I forgot - I'm thinking a whitelist probably would make more sense in the meantime. We know what skins have this thing, it's not particularly likely new ones will add it unless something major changes with the development/usage of this or related extensions, and it appears there is a fallback behaviour for those that don't have the better thing, so we should be falling back to that by default, instead of unlisted unsupporting skins just winding up with nothing at all.

Basically it's a quick fix until we get something, you know, proper.