Page MenuHomePhabricator

Convert MobileFrontend extension to use extension registration
Closed, ResolvedPublic

Description

The MobileFrontend extension needs to be converted to use the new extension registration system. More details are available on T87875.

This is currently stalled whilst we decide how to document globals and keep them up to date and relevant.

Event Timeline

Legoktm created this task.Jan 30 2015, 2:10 AM
Legoktm updated the task description. (Show Details)
Legoktm raised the priority of this task from to Needs Triage.
Legoktm added a project: Readers-Web-Backlog.
Legoktm added a subscriber: Legoktm.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 30 2015, 2:10 AM
gerritbot added a subscriber: gerritbot.

Change 183906 had a related patch set uploaded (by Phuedx):
WIP: new extension registration

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

Patch-For-Review

As you can see it's messy.
I think for the sanity of developers we may want to do some cleanup before doing this to avoid having a huge json blob like this.

Florian added a subscriber: Florian.Feb 2 2015, 8:17 AM

Change 183906 abandoned by Jdlrobson:
WIP: new extension registration

Reason:
No activity in a while. Let's revisit this later. Patch still linked to from bug.

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

Change 183906 restored by Legoktm:
WIP: new extension registration

Reason:
Just because a patch hasn't been touched in a while doesn't mean it should be abandoned.

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

@Legoktm different teams have different practices. The word 'abandon' is unfortunate as the patch is still available. Someone can always restore it when they are ready to work on it again. Nothing is gained by having a patch lingering around in the review queue, it just distracts us from doing other code review.

I've removed it from your review queue since it was distracting, and will re-add you as a reviewer once it's no longer a WIP.

@Legoktm you make assumptions about my review queue. My review queue is every open patchset.
Most of the mobile team use the same tool.
I'm not sure why the issue. If you want to revive the patch be my guest, but it has sat in the review queue for 75 days and to my knowledge Max personally has no time to revive it right now. If it's abandoned this has more chance of getting done as it allows another team member to pick it up... it also gives the impression patches take 75 days to get reviewed in MobileFrontend which is simply not true..

Please allow me to manage code review expectations for the extension I manage accordingly.

Change 183906 abandoned by Jdlrobson:
WIP: new extension registration

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

(I should note this is not anything personal. I want the extension.json to happen but I'm more confident it will happen this way by making it clear Max's patch is not currently being worked on and can be picked up by someone else)

I'm not sure why the issue. If you want to revive the patch be my guest, but it has sat in the review queue for 75 days and to my knowledge Max personally has no time to revive it right now. If it's abandoned this has more chance of getting done as it allows another team member to pick it up... it also gives the impression patches take 75 days to get reviewed in MobileFrontend which is simply not true..

It's an issue because by abandoning it, you've removed it from my review queue. Umm, I just tried to revive it except before I could even rebase it you abandonded it again??? wtf?? This is ridiculous.

Okay, if you're reviving it that's different - assign yourself to the bug and feel free to restore it.

Paladox set Security to None.

Change 209882 had a related patch set uploaded (by Paladox):
Add extension.json, empty PHP entry point

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

Florian assigned this task to Paladox.May 10 2015, 12:29 AM

I -2ed the patch for now. We can't just throw away hundreds of lines of code comments. According to Legoktm, any key in the top structure or under "config" that starts with @ will be ignored, so we can include documentation that way. Another option is a README file, although I'm not sure if people would actually keep it up-to-date. I'll be happy to remove my -2 if we can come up with a good way to retain the comments and documentation.

kaldari added a subscriber: phuedx.May 21 2015, 9:47 PM

Hi best way would be @doc

I have added the options to a README file but haven't added things like Autoloadclasses and other ones on options that are added through this extension.

Hi I get this error now

Exception encountered, of type "ConfigException"
[f5cc5671] /index.php?title=Main_Page&mobileaction=toggle_view_mobile ConfigException from line 53 of /home/paladox1/public_html/includes/config/GlobalVarConfig.php: GlobalVarConfig::get: undefined option: 'MFBrowseTags'
Backtrace:
#0 /home/paladox1/public_html/extensions/MobileFrontend/includes/skins/SkinMinerva.php(1406): GlobalVarConfig->get(string)
#1 /home/paladox1/public_html/extensions/MobileFrontend/includes/skins/SkinMinerva.php(1394): SkinMinerva->getBrowseTagService()
#2 /home/paladox1/public_html/extensions/MobileFrontend/includes/skins/SkinMinerva.php(1089): SkinMinerva->getBrowseTags(Title)
#3 /home/paladox1/public_html/includes/OutputPage.php(2270): SkinMinerva->getDefaultModules()
#4 /home/paladox1/public_html/includes/MediaWiki.php(642): OutputPage->output()
#5 /home/paladox1/public_html/includes/MediaWiki.php(431): MediaWiki->main()
#6 /home/paladox1/public_html/index.php(41): MediaWiki->run()
#7 {main}

in the current patch. was working before last time I tested it was a month or two ago. Why is it causing this and how can it be fixed since it works if I download from master branch but if I download my patch it doesent. Must be a problem with extension.json.

MFIsBrowseEnabled and MFBrowseTags are defined in extension.json in that patch. GlobalVarConfig doesn't seem to be picking 'em up for some reason.

The latest patchset loses lots of very important comments in Resources.php and other fiels such as // FIXME: Remove when cache has cleared
This is simply unacceptable and we need to find a way to do this in the new way.

@Paladox since this patch seems to have been going on for a while (now 50+ patchsets) maybe we should talk some more on the task about moving this forward, as I'm keen for us to make some progress here and our issues to be addressed one at a time. I think the problem here is this is actually an epic task compared to switching to extension.json in a place like Gather.

If possible technically it might make sense to do this bit by bit - @Legoktm is it possible to have extension.json for say... config only and keep resources loading via a php file? If so could we move this forward that way, chunk by chunk?

This is quite frankly a terrifyingly large patch and I'd rather have more confidence we will be able to identify errors and rollback.

@phuedx fixed issue it was due to a space between the config name and " so I removed space and it fixed problem.

Yes I think I would need help to load resource loader.php file since doing it through callback isent working for me.

I have an idea on this since setting this $wgResourceModules = array_merge( $wgResourceModules, array( works even in the callback. But I can redo the resource file to support this since Doing it the way it is doesent work.

@Jdlrobson please see new patch at https://gerrit.wikimedia.org/r/#/c/209882/ which I have now resources in a resources file no longer in the extension.json. I have tested it and its works. haven't tested all parts.

Jdlrobson moved this task from Backlog to Epics on the MobileFrontend board.Aug 4 2015, 6:38 PM

Ping @bd808 do you have any good ideas on documentating globals inside these extension.json files? History has shown keeping separate files for documentation never works and they go out of date.

This patch has been sitting here a while and I'm keen to move things forward but I want to understand our options.

Jdlrobson changed the task status from Open to Stalled.
Paladox changed the task status from Stalled to Open.Aug 13 2015, 12:05 AM

@Legoktm recommends the README file but you can do it in extension.json if you want.

+1 to adding documentation to the README file. From recent experience, even adding a brief per-variable summary to extension.json makes it very noisy.

History has shown keeping separate files for documentation never works and they go out of date.

Here's my attempt at phrasing the problem: history has shown that the MobileFrontend maintainers – myself included – mostly fail to update documentation outside of the vicinity of a code change, i.e. the README file, the HISTORY file, and on-wiki documentation.

Jdlrobson triaged this task as Normal priority.Sep 16 2015, 6:46 PM

Blocking task has been closed. This task can now move along. it is now recommended to use a README file for adding comments.

Possibly as long as the patch work. I haven't tested my current patch but tested the previous couple and worked I haven't changed much asept from rebasing.

Change 241077 had a related patch set uploaded (by Paladox):
Add hooks that carnt be added in extension.json in MobileFrontend.hooks.php

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

Change 241075 had a related patch set uploaded (by Paladox):
Update Resouces.php to support extension.json

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

Jdlrobson changed the task status from Open to Stalled.Sep 29 2015, 10:37 PM

We should really fix T108271 first. That said https://gerrit.wikimedia.org/r/241075 looks like with a bit of additional work it should be mergeable. @Paladox can you help with that?

Hi what should I help with. I am not understanding do you mean that patch I uploaded about resource loader. If so what should I do.

That bug won't really affect this one since there is a workaround just load it through a php file using a callback. And then later on once that task is resolved then bring the changes to the extension.

Change 244958 had a related patch set uploaded (by Paladox):
Add extension.json, empty PHP entry point

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

Change 244958 abandoned by Paladox:
Add extension.json, empty PHP entry point

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

kaldari removed a subscriber: kaldari.Oct 13 2015, 4:26 PM

Change 257438 had a related patch set uploaded (by Florianschmidtwelzow):
Convert MobileFrontend to extension registration

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

Florian closed this task as Resolved.Jan 29 2016, 7:13 PM
Florian claimed this task.
Florian removed a project: Patch-For-Review.

finally! :D

Change 257438 merged by jenkins-bot:
Convert MobileFrontend to extension registration

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

Hearty congratulations to those involved!

Change 241077 abandoned by Jdlrobson:
Add hooks that can't be added in extension.json in MobileFrontend.hooks.php

Reason:
Superseded by https://gerrit.wikimedia.org/r/257438
Thanks Paladox for pushing us to get this done!

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

Change 209882 abandoned by Jdlrobson:
Add extension.json, empty PHP entry point

Reason:
Superseded by https://gerrit.wikimedia.org/r/257438
Thanks Paladox for pushing us to get this done!

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

Change 241075 abandoned by Jdlrobson:
Update Resources.php to support extension.json

Reason:
Superseded by https://gerrit.wikimedia.org/r/257438
Thanks Paladox for pushing us to get this done!

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

Change 267401 had a related patch set uploaded (by Jdlrobson):
Hygiene: Avoid confusion ASAP - remove Resources.php now unused

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

Change 267401 merged by jenkins-bot:
Hygiene: Avoid confusion ASAP - remove Resources.php now unused

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

Change 267810 had a related patch set uploaded (by Jdlrobson):
Avoid even more extension.json confusion

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

Change 267810 merged by jenkins-bot:
Avoid even more extension.json confusion

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