Page MenuHomePhabricator

Request: Centralize code for "Data item" link in Wikibase codebase
Closed, ResolvedPublic

Description

Background

In [https://gerrit.wikimedia.org/r/c/mediawiki/skins/MinervaNeue/+/1071207 this patch] logic was added to Minerva to change the location of the "Data item" link prior to rendering.

While we have done this historically, it has led to confusion as it requires referencing multiple extensions to understand logic; it creates unclear who is accountable for the code when it breaks as the code operates in multiple boundaries; and it adds constraints to skin developers who may need to make rendering changes

It is proposed that this logic is moved into Wikibase at the time the link is inserted into the menu.

BDD

  • For QA engineer to fill out

Test Steps

  • For QA engineer to fill out

Acceptance criteria

  • There should be no logic in Minerva for relocating the wikibase link.
  • The wikibase link should appear in the toolbox on Minerva
  • The wikibase link should appear in "Other projects" on other skins

Why this is important

  • For skins we have been working towards migrating all menu manipulation towards the https://www.mediawiki.org/wiki/Manual:Hooks/SkinTemplateNavigation::Universal hook. In future we hope this would replace SidebarBeforeOutput hook for menu manipulation. We already moved categories there for example.
  • We have been slowly making Minerva compatible with all desktop menus and hooks. In future, as written it's possible that when Minerva supports sidebar, you'd have two links in 2 different places.

This task was created by Version 1.2.0 of the Web team task template using phabulous

Event Timeline

What I would recommend is doing all of this manipulation inside Wikibase:
Do the Minerva menu modification in hook(s) inside Wikibase instead of Minerva. It looks like currently you are adding the wikibase link in the SidebarBeforeOutput hook
https://gerrit.wikimedia.org/g/mediawiki/extensions/Wikibase/+/b8f640a0fa43835ff0f838706ea42fe075ba9dfc/client/includes/Hooks/SidebarHookHandler.php#132å
so here would be an appropriate place to check if the Minerva skin is present. This also means you can apply the logic to other skins if need be via your own configuration flag (UniversalLanguageSelector does this with its ULSPosition configuration flag as an example).

if ( $skin !== 'minerva' ) {
  addWikibaseLinkToProjects();
}
  1. For adding to the toolbox, use https://www.mediawiki.org/wiki/Manual:Hooks/SkinTemplateNavigation::Universal to inject your Wikibase link with a conditional check against skin.

e.g.

 if ( $skin === 'minerva' ) {
   addWikibaseLinkToToolbox();
}

Lower priority gotcha to watch out for: the caching for the sidebar is different to the toolbox (see https://www.mediawiki.org/wiki/Manual:Hooks/SkinBuildSidebar#Note_about_caching ) . I don't think this effects you but worth bearing in mind when you make use of both of these hooks

Change #1078921 had a related patch set uploaded (by Joely Rooke WMDE; author: Joely Rooke WMDE):

[mediawiki/extensions/Wikibase@master] Handle Minerva wikibase link in wikibase repo

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

Change #1078938 had a related patch set uploaded (by Joely Rooke WMDE; author: Joely Rooke WMDE):

[mediawiki/extensions/Wikibase@master] Handle Minerva wikibase link in wikibase repo

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

Change #1078921 abandoned by Joely Rooke WMDE:

[mediawiki/extensions/Wikibase@master] Handle Minerva wikibase link in wikibase repo

Reason:

Duplicate change made

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

Change #1079302 had a related patch set uploaded (by Joely Rooke WMDE; author: Joely Rooke WMDE):

[mediawiki/extensions/Wikibase@master] Handle Minerva wikibase link in wikibase repo

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

Change #1079302 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Handle Minerva wikibase link in wikibase repo

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

Thanks all for your understanding and speedy turnaround!

Change #1078938 abandoned by Joely Rooke WMDE:

[mediawiki/extensions/Wikibase@master] Handle Minerva wikibase link in wikibase repo

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