Page MenuHomePhabricator
Create Task
Maniphest T300284

Mark Skin::buildContentNavigationUrls as @internal
Closed, ResolvedPublicBUG REPORT
Actions

Description

In preparation for a future refactor I'd like to make Skin::buildContentNavigationUrls @internal, with the eventual plan to deprecate and replace the method.

Mirage, Tempo, WMAU, Nimbus, Citizen, Minerva

TODO

  • Understand what limitations Skin::getTemplateData has that leads skins to use this method.
  • Provide alternatives (where needed) for all the associated skins via hooks/updates to template data.
  • Mark Skin::buildContentNavigationUrls as an @internal method making it clear the method shouldn't be extended or used as precursor to making it private
  • Make the method as deprecated (We can do this by making buildContentNavigationUrls call a private method buildContentNavigationUrlsInternal and send deprecation warnings when buildContentNavigationUrls is called)
  • submit patch for Tempo https://gerrit.wikimedia.org/r/c/mediawiki/skins/Tempo/+/761405/1
  • submit patch for Citizen https://github.com/StarCitizenTools/mediawiki-skins-Citizen/issues/422

Details

SubjectRepoBranchLines +/-
Use of Skin::buildContentNavigationUrls is deprecatedmediawiki/skins/Tempomaster+22 -22
Deprecate buildContentNavigationUrlsmediawiki/coremaster+17 -4
buildContentNavigationUrls will be made internal and privatemediawiki/skins/MinervaNeuemaster+25 -1
Avoid buildContentNavigationUrls() and rearrange side tool drawermediawiki/skins/WMAUmaster+83 -67
Create a separate portal for the edit button and its dropdownmediawiki/skins/Miragemaster+191 -182
Don't use reflection to access content_actionsmediawiki/skins/Nimbusmaster+7 -17
Skins: Supply is-empty for portletsmediawiki/coremaster+2 -2
Customize query in gerrit

Related Objects

Event Timeline

Jdlrobson created this task.Jan 27 2022, 6:44 PM
Restricted Application added a subscriber: Masumrezarock100. · View Herald TranscriptJan 27 2022, 6:44 PM
Mainframe98 subscribed.Jan 27 2022, 7:23 PM

I've checked for MediaWiki-skins-Mirage. Mirage uses buildContentNavigationUrls in one place, twice: Once to determine if it needs to provide its own notification bell, and once to create an edit button dropdown for the views and actions portlet. The first issue can be addressed with an alternate approach. It would be helpful to have the html-items property of portlets be null when there are none instead of an empty string so that the conditional logic in mustache templates can be used, but I've worked around that in Mirage itself for now. The second issue is more difficult to address as the portlets provided to mustache are already turned into HTML, preventing modification.

I'm sure I'm able to come up with a convoluted approach for that given time, but right now I can't think of anything that would solve the dependency on buildContentNavigationUrls without re-engineering half of MediaWiki, Mirage, or both.

For the first usage, I've uploaded https://gerrit.wikimedia.org/r/c/mediawiki/skins/Mirage/+/757728.

Mainframe98 moved this task from Backlog to Modernizations and deprecations on the MediaWiki-skins-Mirage board.Jan 27 2022, 7:24 PM
gerritbot added a comment.Jan 28 2022, 8:51 AM

Change 757841 had a related patch set uploaded (by Mainframe98; author: Mainframe98):

[mediawiki/core@master] Skins: Supply is-empty for portlets

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

gerritbot added a project: Patch-For-Review.Jan 28 2022, 8:51 AM
Jdlrobson added a subscriber: Samwilson.Jan 28 2022, 11:56 PM
  • Minerva appears to be using buildContentNavigationUrls to get at the raw data to inform the building of its own menu items. I'm pretty sure it could do this via hooks but this would be a non-trivial (but achievable) refactor to get the same desired result.
  • @alistair3149 from what I can see Citizen skin is using similar code to Vector and using it inside Skin::buildPageTools to create data for the template. I don't think it's transforming the data in any way. Could the existing data-portlets be used to the same effect or am I missing something?
  • WMAU skin seems to be creating its own menu from existing menu items. I'm pretty sure the same result can be achieved using SkinTemplateNavigationUniversal to register a custom menu. See CologneBlue for reference (https://github.com/wikimedia/mediawiki-skins-CologneBlue/blob/master/includes/CologneBlueHooks.php#L231) @Samwilson could you take a look and see if that's correct? Happy to help guide you if there's any problems.
  • Tempo is a PHP SkinTemplate skin. It can use $tpl->set( 'content_navigation' ) to unset items.
  • Nimbus is doing something pretty hacky using ReflectMethod to call buildContentActionUrls. It should just be using $tpl->get( 'content_actions' ) so also easy to fix.
gerritbot added a comment.Jan 31 2022, 8:57 PM

Change 757841 merged by jenkins-bot:

[mediawiki/core@master] Skins: Supply is-empty for portlets

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

ReleaseTaggerBot added a project: MW-1.38-notes (1.38.0-wmf.20; 2022-01-31).Jan 31 2022, 9:00 PM
Mainframe98 mentioned this in rSMIR6d317949f5ba: Remove a dependency on SkinTemplate::buildContentNavigationUrls.Jan 31 2022, 9:00 PM
Maintenance_bot removed a project: Patch-For-Review.Jan 31 2022, 9:10 PM
Jdlrobson moved this task from Backlog to Milestone: Separate BaseTemplate and SkinMustache code, simplify Skin class on the MediaWiki-Core-Skin-Architecture board.Feb 2 2022, 12:46 AM
Jdlrobson moved this task from Milestone: Separate BaseTemplate and SkinMustache code, simplify Skin class to Unused 3 on the MediaWiki-Core-Skin-Architecture board.
gerritbot added a comment.Feb 3 2022, 1:33 PM

Change 759501 had a related patch set uploaded (by Mainframe98; author: Mainframe98):

[mediawiki/skins/Mirage@master] Create a separate portal for the edit button and its dropdown

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

gerritbot added a project: Patch-For-Review.Feb 3 2022, 1:33 PM
Mainframe98 added a comment.Feb 3 2022, 2:53 PM

https://gerrit.wikimedia.org/r/759501 fixes the second usage of Skin::buildContentNavigationUrls, the note about Minerva in T300284#7660986 gave me the idea to use a custom portlet through the SkinTemplateNavigation::Universal hook, but it feels like a bit of a hack.

Before I merge that change, I'd like to know if you, @Jdlrobson, think this might complicate matters in the future when overhauling buildContentNavigationUrls.

gerritbot added a comment.Feb 3 2022, 3:18 PM

Change 759513 had a related patch set uploaded (by Mainframe98; author: Mainframe98):

[mediawiki/skins/Nimbus@master] Don't use reflection to access content_actions

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

gerritbot added a comment.Feb 3 2022, 5:02 PM

Change 759513 merged by jenkins-bot:

[mediawiki/skins/Nimbus@master] Don't use reflection to access content_actions

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

Jdlrobson removed a project: Nimbus.Feb 3 2022, 9:55 PM
gerritbot added a comment.Feb 4 2022, 9:18 AM

Change 759679 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/skins/WMAU@master] Avoid buildContentNavigationUrls() and rearrange side tool drawer

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

Samwilson added a comment.Feb 4 2022, 9:27 AM

@Jdlrobson Thanks for the ping. I've had a go at fixing this up, and I think the only remaining bit to sort out is how properly to remove the mainpage and specialpages links from the side drawer.

gerritbot added a comment.Feb 4 2022, 10:48 AM

Change 759501 merged by jenkins-bot:

[mediawiki/skins/Mirage@master] Create a separate portal for the edit button and its dropdown

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

Mainframe98 mentioned this in rSMIR54dcf8981060: Create a separate portal for the edit button and its dropdown.Feb 4 2022, 10:48 AM
Jdlrobson added a project: MW-1.38-release.Feb 4 2022, 5:22 PM
Jdlrobson moved this task from Unused 3 to Milestone: 1.38 release on the MediaWiki-Core-Skin-Architecture board.

Thanks everyone. Getting this into the 1.38 release now seems very achieveable.

gerritbot added a comment.Feb 4 2022, 5:23 PM

Change 759744 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/MinervaNeue@master] buildContentNavigationUrls will be made internal and private

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

Jdlrobson triaged this task as Medium priority.Feb 4 2022, 5:39 PM
gerritbot added a comment.Feb 7 2022, 9:45 PM

Change 760669 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Deprecate buildContentNavigationUrls

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

Jdlrobson updated the task description. (Show Details)Feb 7 2022, 10:15 PM
gerritbot added a comment.Feb 8 2022, 1:26 AM

Change 759679 merged by jenkins-bot:

[mediawiki/skins/WMAU@master] Avoid buildContentNavigationUrls() and rearrange side tool drawer

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

Samwilson mentioned this in rSWAU544d4e824764: Avoid buildContentNavigationUrls() and rearrange side tool drawer.Feb 8 2022, 1:27 AM
Jdlrobson added a project: Web-Team-Backlog (Needs Prioritization (Tech)).Feb 8 2022, 5:32 PM
gerritbot added a comment.Feb 9 2022, 12:06 AM

Change 759744 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] buildContentNavigationUrls will be made internal and private

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

ReleaseTaggerBot edited projects, added MW-1.38-notes (1.38.0-wmf.22; 2022-02-14); removed MW-1.38-notes (1.38.0-wmf.20; 2022-01-31).Feb 9 2022, 1:00 AM
gerritbot added a comment.Feb 9 2022, 2:31 AM

Change 760669 merged by jenkins-bot:

[mediawiki/core@master] Deprecate buildContentNavigationUrls

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

Jdlrobson updated the task description. (Show Details)Feb 9 2022, 2:43 AM
Jdlrobson updated the task description. (Show Details)Feb 9 2022, 3:47 PM
gerritbot added a comment.Feb 9 2022, 5:38 PM

Change 761405 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Tempo@master] Use of Skin::buildContentNavigationUrls is deprecated

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

Jdlrobson closed this task as Resolved.Feb 9 2022, 5:39 PM
Jdlrobson updated the task description. (Show Details)

Thanks everyone for the help with this one!

gerritbot added a comment.Feb 11 2022, 2:37 PM

Change 761405 merged by jenkins-bot:

[mediawiki/skins/Tempo@master] Use of Skin::buildContentNavigationUrls is deprecated

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

Mainframe98 mentioned this in rSMIRbda7037a6567: Use SkinTemplate::runOnSkinTemplateNavigationHooks.Feb 27 2022, 2:10 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL