Page MenuHomePhabricator
Create Task
Maniphest T262233

MobileFrontend doesn't work with Vector
Closed, ResolvedPublic
Actions

Description

According to https://www.mediawiki.org/wiki/Extension:MobileFrontend#Setup_a_skin one could set

$wgMFDefaultSkinClass = 'SkinVector';

to have MobileFrontend use Vector.

However, setting that configuration (mediawiki 1:1.35.0~rc.3-1) with multiple MobileFrontent versions, fail with:

PHP Fatal error:  Uncaught Error: Cannot use object of type RequestContext as array in /usr/share/mediawiki/includes/skins/Skin.php:157
tack trace:
#0 /var/lib/mediawiki/extensions/MobileFrontend/includes/MobileFrontendHooks.php(60): Skin->__construct(Object(RequestContext))
#1 /var/lib/mediawiki/extensions/MobileFrontend/includes/MobileFrontendHooks.php(127): MobileFrontendHooks::getDefaultMobileSkin(Object(RequestContext), Object(GlobalVarConfig))
#2 /usr/share/mediawiki/includes/HookContainer/HookContainer.php(320): MobileFrontendHooks::onRequestContextCreateSkin(Object(RequestContext), NULL)
#3 /usr/share/mediawiki/includes/HookContainer/HookContainer.php(131): MediaWiki\\HookContainer\\HookContainer->callLegacyHook('RequestContextC...', Array, Array, Array)
#4 /usr/share/mediawiki/includes/HookContainer/HookRunner.php(3221): MediaWiki\\HookContainer\\HookContainer->run('RequestContextC...', Array)
#5 /usr/share/mediawiki/includes/context/RequestContext.php(402): MediaWiki\\HookContainer\\HookRunner->onRequestContextCreateSkin( in /usr/share/mediawiki/includes/skins/Skin.php on line 157

Details

SubjectRepoBranchLines +/-
Don't fatal when creating new Mobile Skinmediawiki/extensions/MobileFrontendREL1_35+38 -13
Don't fatal when creating new Mobile Skinmediawiki/extensions/MobileFrontendmaster+38 -13
Clarify documentation of SkinVector::__construct()mediawiki/skins/Vectormaster+5 -0
Remove problematic fallback skinname propertymediawiki/coreREL1_35+0 -6
Require Skin name when passing options to Skin constructormediawiki/coremaster+26 -14
Remove problematic fallback skinname propertymediawiki/coremaster+0 -6
Customize query in gerrit

Event Timeline

Platonides created this task.Sep 7 2020, 10:48 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 7 2020, 10:48 PM
Reedy renamed this task from MobileFrontent doesn't work with Vector to MobileFrontend doesn't work with Vector.Sep 8 2020, 12:31 AM
Reedy added a project: MW-1.35-release.
Reedy updated the task description. (Show Details)
Reedy moved this task from Blocker to Not a blocker on the MW-1.35-release board.
Reedy added a project: Web-Team-Backlog.Sep 8 2020, 1:16 AM
Reedy added a project: MediaWiki-Core-Hooks.Sep 8 2020, 1:20 AM
gerritbot added a comment.Sep 8 2020, 6:03 AM

Change 625775 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/extensions/MobileFrontend@master] Don't fatal when creating new Mobile Skin

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

gerritbot added a project: Patch-For-Review.Sep 8 2020, 6:03 AM
Ammarpad triaged this task as Medium priority.Sep 8 2020, 8:16 AM
Jdlrobson edited projects, added Vector (legacy skin); removed Patch-For-Review, MediaWiki-Core-Hooks, Web-Team-Backlog.EditedSep 8 2020, 3:48 PM
Jdlrobson added subscribers: Ammarpad, Jdlrobson.

I can replicate it. Looks like this got broken in the SkinMustache work. I'm not sure why Context is passed to skin as a parameter. It should be an array or a string (name).

As @Ammarpad has hinted on gerrit, we need to use SkinFactory here.

I think this is the right time to switch this config to support skin key from MFDefaultSkinClass to wgDefaultMobileSkin or wgMFDefaultSkin to mirror core. This will also be more intuitive to 3rd parties.

For compatibility even thought it's inefficient we could run a compatibility mode:

$compatSkinClass = $config->get( 'MFDefaultSkinClass' );
 $mfDefaultSkin = $config->get( 'MFDefaultSkin' );
// compatiiblity mode
if ( $compatSkinName ) {
  // log warning to sysops to change value
  $skin = new $compatSkinClass();
  // https://github.com/wikimedia/mediawiki/blob/master/includes/skins/Skin.php#L168
  $mfDefaultSkin = $skin->getSkinName();
}

SkinFactory::makeSkin(  $mfDefaultSkin  )

Thoughts?

Jdlrobson edited projects, added Vector (legacy skin) (Tracking), Web-Team-Backlog; removed Vector (legacy skin).Sep 8 2020, 6:02 PM
Mainframe98 subscribed.Sep 8 2020, 8:14 PM
Ammarpad added a comment.EditedSep 9 2020, 12:21 AM

I think wgDefaultMobileSkin would be more intuitive and mirrors the core main skin variable. I am not fan of the MF acronym (and it seems not all MobileFontend config variables are using it.)

Ammarpad claimed this task.Sep 9 2020, 9:45 AM
Jdlrobson added a comment.Sep 9 2020, 8:34 PM
In T262233#6445154, @Ammarpad wrote:

I think wgDefaultMobileSkin would be more intuitive and mirrors the core main skin variable. I am not fan of the MF acronym (and it seems not all MobileFontend config variables are using it.)

WFM!

gerritbot added a comment.Sep 14 2020, 11:02 AM

Change 627252 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/core@master] Remove problematic fallback skinname property

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

gerritbot added a project: Patch-For-Review.Sep 14 2020, 11:02 AM
gerritbot added a comment.Sep 14 2020, 8:34 PM

Change 627252 merged by jenkins-bot:
[mediawiki/core@master] Remove problematic fallback skinname property

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

ReleaseTaggerBot added a project: MW-1.36-notes (1.36.0-wmf.9; 2020-09-15).Sep 14 2020, 9:01 PM
gerritbot added a comment.Sep 15 2020, 1:15 PM

Change 627451 had a related patch set uploaded (by Reedy; owner: Ammarpad):
[mediawiki/core@REL1_35] Remove problematic fallback skinname property

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

Jdlrobson edited projects, added Web-Team-Backlog (Kanbanana-FY-2020-21); removed Web-Team-Backlog.Sep 15 2020, 11:09 PM
gerritbot added a comment.Sep 16 2020, 11:02 AM

Change 627785 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/core@master] Require Skin name or default to $wgDefaultSkin when constructing skin

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

Jdlrobson moved this task from Needs Analysis to Doing on the Web-Team-Backlog (Kanbanana-FY-2020-21) board.Sep 17 2020, 1:14 AM
gerritbot added a comment.Sep 17 2020, 11:06 AM

Change 628069 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/skins/Vector@master] Add skin name before calling parent constructor

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

gerritbot added a comment.Sep 18 2020, 4:41 PM

Change 627785 merged by jenkins-bot:
[mediawiki/core@master] Require Skin name when passing options to Skin constructor

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

ReleaseTaggerBot edited projects, added MW-1.36-notes (1.36.0-wmf.10; 2020-09-22); removed MW-1.36-notes (1.36.0-wmf.9; 2020-09-15).Sep 18 2020, 5:00 PM
Reedy subscribed.Sep 18 2020, 9:56 PM

Which other patches from this task actually need backporting?

Jdlrobson moved this task from Doing to Code Review on the Web-Team-Backlog (Kanbanana-FY-2020-21) board.Sep 21 2020, 5:11 PM
gerritbot added a comment.Sep 21 2020, 8:42 PM

Change 628822 had a related patch set uploaded (by Jdlrobson; owner: Ammarpad):
[mediawiki/extensions/MobileFrontend@REL1_35] Don't fatal when creating new Mobile Skin

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

Jdlrobson moved this task from Code Review to Ready for Signoff on the Web-Team-Backlog (Kanbanana-FY-2020-21) board.Sep 21 2020, 8:46 PM
gerritbot added a comment.Sep 21 2020, 9:02 PM

Change 627451 abandoned by Jdlrobson:
[mediawiki/core@REL1_35] Remove problematic fallback skinname property

Reason:
Doesn't need to go into branch. Allows more time for skin developers to update to support the new enforced skin name.

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

gerritbot added a comment.Sep 21 2020, 9:04 PM

Change 628069 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Clarify documentation of SkinVector::__construct()

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

Reedy added a comment.Sep 21 2020, 9:09 PM
In T262233#6475719, @Reedy wrote:

Which other patches from this task actually need backporting?

Is it just https://gerrit.wikimedia.org/r/c/628822 that needs merging into REL1_35?

Jdlrobson added a comment.Sep 21 2020, 9:14 PM

Yes.

gerritbot added a comment.Sep 21 2020, 9:34 PM

Change 625775 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Don't fatal when creating new Mobile Skin

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

gerritbot added a comment.Sep 21 2020, 10:19 PM

Change 628822 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@REL1_35] Don't fatal when creating new Mobile Skin

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

Reedy moved this task from Not a blocker to Done on the MW-1.35-release board.Sep 21 2020, 11:11 PM
Jdlrobson closed this task as Resolved.Sep 30 2020, 5:08 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 · Credits