Allow the MobileContext class to be broken apart by creating the MobileContext service
Closed, ResolvedPublic5 Estimated Story Points
Actions

Assigned To

Authored By

	phuedx
	Aug 25 2016, 10:11 AM

Description

There are many services within the MobileContext god object that will be extracted on a case-by-case basis. As there are many clients of the MobileContext class, it will have to act as a proxy for these new services until those clients are migrated. Since these new dependencies will have to be injected into MobileContext, we'll need to create the MobileFrontend.MobileContext service.

Note well that while MobileContext is a singleton, [its singleton instance can be overridden via MobileContext::setInstance](https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/includes/MobileContext.php#L121-L128).

TOREVIEW

TODO

Audit MobileContext::setInstance class, documenting:
- where and why the call is made
- how the call might be removed
Create the MobileFrontend.Context service
For backwards compatibility, make MobileContext::singleton defer to MediaWikiServices
Remove calls to MobileContext::setInstance that can be safely removed

TODONE

Introduce MediaWikiServices into the MobileFrontend codebase (rEMFR819064d68aba)

Details

Subject	Repo	Branch	Lines +/-
Remove MFPageActions/MFEnableSiteNotice vars	mediawiki/extensions/MobileFrontend	master	+9 -30
Hygiene: Remove BogusMobileContext	mediawiki/extensions/MobileFrontend	master	+0 -20
Name singleton overriding methods more clearly	mediawiki/extensions/MobileFrontend	master	+23 -15
Revert "Revert "Hygiene: Remove BogusMobileContext""	mediawiki/extensions/MobileFrontend	master	+0 -22
Revert "Revert "Add MobileFrontend.Context service""	mediawiki/extensions/MobileFrontend	master	+85 -24
Add MobileFrontend.Context service	mediawiki/extensions/MobileFrontend	master	+85 -24
Hygiene: Remove BogusMobileContext	mediawiki/extensions/MobileFrontend	master	+0 -22

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Declined	None	T208761 [Epic] Improve PHPUnit coverage in MobileFrontend to 50%
Open	None	T143189 [EPIC] Split MobileContext god object apart
Resolved	phuedx	T143875 Allow the MobileContext class to be broken apart by creating the MobileContext service

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

ovasileva added a project: Reading-Web-Sprint-81-We-suck-at-coming-up-with-sprint-names.Sep 7 2016, 3:15 PM

phuedx updated the task description. (Show Details)Sep 8 2016, 9:06 AM

Change 305212 had a related patch set uploaded (by Phuedx):
Add MobileFrontend.Context service

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

gerritbot added a project: Patch-For-Review.Sep 8 2016, 9:58 AM

phuedx updated the task description. (Show Details)Sep 8 2016, 10:02 AM

Change 309281 had a related patch set uploaded (by Phuedx):
Hygiene: Remove BogusMobileContext

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

phuedx updated the task description. (Show Details)Sep 12 2016, 9:37 AM

MBinder_WMF set the point value for this task to 5.Sep 12 2016, 4:59 PM

MBinder_WMF moved this task from Needs Analysis to Code Review on the Reading-Web-Sprint-81-We-suck-at-coming-up-with-sprint-names board.

• Jhernandez assigned this task to phuedx.Sep 12 2016, 4:59 PM

• Jhernandez removed the point value for this task.

• Jhernandez set the point value for this task to 5.

Jdlrobson moved this task from Code Review to Blocked Externally on the Reading-Web-Sprint-81-We-suck-at-coming-up-with-sprint-names board.Sep 13 2016, 12:07 AM

Jdlrobson moved this task from Blocked Externally to Code Review on the Reading-Web-Sprint-81-We-suck-at-coming-up-with-sprint-names board.Sep 13 2016, 8:18 PM

Change 305212 merged by jenkins-bot:
Add MobileFrontend.Context service

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

ReleaseTaggerBot added a project: MW-1.28-release (WMF-deploy-2016-09-20_(1.28.0-wmf.20)).Sep 13 2016, 10:00 PM

Change 309281 merged by jenkins-bot:
Hygiene: Remove BogusMobileContext

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

Jdlrobson moved this task from Code Review to Ready for Signoff on the Reading-Web-Sprint-81-We-suck-at-coming-up-with-sprint-names board.Sep 13 2016, 10:23 PM

@Jhernandez: I guess it's over to you (!?)

See the last 3 comments on https://gerrit.wikimedia.org/r/#/c/305212/

Seems like the patch was reverted because of beta issues not caught on CI again?

phuedx claimed this task.Sep 16 2016, 8:35 AM

Thanks again to @Legoktm for the quick reverts (rEMFR725abdcce59c: Revert "Add MobileFrontend.Context service" and rEMFR8b5fc331bc83: Revert "Hygiene: Remove BogusMobileContext").

I'm worried that I didn't see exceptions during local testing or during CI. What's different about the Beta Cluster?

Change 311102 had a related patch set uploaded (by Phuedx):
Remove MFPageActions/MFEnableSiteNotice vars

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

Change 311103 had a related patch set uploaded (by Phuedx):
Revert "Revert "Add MobileFrontend.Context service""

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

Change 311104 had a related patch set uploaded (by Phuedx):
Revert "Revert "Hygiene: Remove BogusMobileContext""

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

I've C-2'd rEMFR42515c05456b: Revert "Revert "Add MobileFrontend.Context service"" and will try to replicate the failure locally and on our staging server so that I can provide instructions for folk testing the changes.

phuedx moved this task from -1 (Needs More Work) to Code Review on the Reading-Web-Sprint-81-We-suck-at-coming-up-with-sprint-names board.Sep 16 2016, 9:44 AM

CI failures abound!

I can reproduce the failure that @Legoktm saw locally by enabling the memcached role, provisioning, and adding $wgMainCacheType = CACHE_ANYTHING; to LocalSettings.php.

If I may, I think the approach being taken here is non-ideal. Instead of trying to refactor the most central object to MobileFrontend with a system that no other extension has tested yet (AFAIK), I would pick some more not-so-central thing to refactor and use well as a service.

So take one function of MobileContext you believe should not be part of it, split it to a new class/service, and register that with MediaWikiServices (and pick something that is not used during the initialization of MW). Don't mess with MobileContext, just leave it as is but slowly move stuff out of it.

In T143875#2644308, @Legoktm wrote:

If I may, I think the approach being taken here is non-ideal. Instead of trying to refactor the most central object to MobileFrontend with a system that no other extension has tested yet (AFAIK), I would pick some more not-so-central thing to refactor and use well as a service.

I think that you're right.

My original thinking was that since MobileContext::singleton was simple (read: hiding a tonne complexity) it should be a quick step to migrate it to a service. With the service created, we could move stuff out of it and inject that stuff back in using good ol' constructor injection in order to maintain backwards compatibility (T143189).

It was my hope that as we did this the MobileContext class would become easier to test in isolation and we could drop a couple of acceptance tests in favour of a load of unit tests.

Your suggestion achieves the same end but with less risk.

I hope to do the same with MobileFrontendHooks as well.

@phuedx I'm a little lost on what's going on here. How can I help you? Are we suggesting reverting the patches so far and starting again? Can you update the task, I'm keen to help in whatever way I can.

@phuedx ditto to T143875#2650474. It seems maybe this task should be declined as invalid and a new one spawned from the ashes? Or do we want to simply repurpose this task? Either way, it would appear from the discussion that at the very least this is no longer a fully analyzed task and probably no longer belongs in the sprint (at least to the right of the Needs Analysis column).

phuedx moved this task from -1 (Needs More Work) to Needs Analysis on the Reading-Web-Sprint-81-We-suck-at-coming-up-with-sprint-names board.Sep 21 2016, 6:47 PM

In T143875#2656374, @jhobs wrote:

@phuedx ditto to T143875#2650474. It seems maybe this task should be declined as invalid and a new one spawned from the ashes? Or do we want to simply repurpose this task? Either way, it would appear from the discussion that at the very least this is no longer a fully analyzed task and probably no longer belongs in the sprint (at least to the right of the Needs Analysis column).

Thanks @jhobs and @Jdlrobson for weighing in on the discussion and yer pings.

I've thought about this some more and I think the best course of action is to not mark this task as Invalid, because it isn't, but do it at a much later date, when MobileContext has been split up enough that it's no longer central to MobileFrontend.

My immediate plan is to:

abandon rEMFR65b35aea2178: Revert "Revert "Add MobileFrontend.Context service"", pulling out renaming MobileContext::setInstance to ::setInstanceForTesting, and adding ::resetInstanceForTesting because I think they're useful changes
resubmit rEMFR353e841cf101: Revert "Revert "Hygiene: Remove BogusMobileContext"" for review
resubmit rEMFR67b825f3b9c6: Remove MFPageActions/MFEnableSiteNotice vars for review

Change 311103 abandoned by Phuedx:
Revert "Revert "Add MobileFrontend.Context service""

Reason:
Per T143875#2658367.

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

Ping us when you need review for the new patches.

Change 311104 abandoned by Phuedx:
Revert "Revert "Hygiene: Remove BogusMobileContext""

Reason:
Per T143875#2658367.

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

Change 312243 had a related patch set uploaded (by Phuedx):
Name singleton overriding methods more clearly

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

Change 312244 had a related patch set uploaded (by Phuedx):
Hygiene: Remove BogusMobileContext

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

rEMFRcafd7fa3b620: Remove MFPageActions/MFEnableSiteNotice vars, rEMFRf0ac32699cff: Name singleton overriding methods more clearly, and rEMFR8504f7238597: Hygiene: Remove BogusMobileContext are ready to be reviewed.

phuedx updated the task description. (Show Details)Sep 22 2016, 12:41 PM

phuedx moved this task from Doing to Code Review on the Reading-Web-Sprint-81-We-suck-at-coming-up-with-sprint-names board.

All patches are +2ed. When the SWAT happens later this will be unblocked and i'll make sure the patches merge and this gets moved to sign off.

Jdlrobson moved this task from Blocked Externally to Code Review on the Reading-Web-Sprint-81-We-suck-at-coming-up-with-sprint-names board.Sep 23 2016, 12:18 AM

Jdlrobson moved this task from Code Review to Ready for Signoff on the Reading-Web-Sprint-81-We-suck-at-coming-up-with-sprint-names board.

Change 311102 merged by jenkins-bot:
Remove MFPageActions/MFEnableSiteNotice vars

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

Change 312243 merged by jenkins-bot:
Name singleton overriding methods more clearly

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

Change 312244 merged by jenkins-bot:
Hygiene: Remove BogusMobileContext

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

ReleaseTaggerBot added a project: MW-1.28-release (WMF-deploy-2016-10-04_(1.28.0-wmf.21)).Sep 23 2016, 1:00 AM

Note that this not being done doesn't block the other parts of this epic, e.g. T143891.

MBinder_WMF added a project: Reading-Web-Sprint-82-Xpect-Rspec.Sep 26 2016, 4:04 PM

ovasileva moved this task from Needs Analysis to Ready for Signoff on the Reading-Web-Sprint-82-Xpect-Rspec board.Sep 26 2016, 4:04 PM

MBinder_WMF moved this task from Triaged but Future to 2016-17 Q2 on the Web-Team-Backlog board.Sep 26 2016, 5:51 PM

Jdlrobson removed a project: Patch-For-Review.Sep 26 2016, 11:17 PM

@phuedx Moving to backlog until the other ones are completed and we can actually tackle this one.

@MBinder_WMF This task was carried over from 81 in signoff, but can't be resolved, needs to go back to backlog for future work, so I've untagged 82 and moved it to backlog's triaged but future.

MBinder_WMF awarded a token.Sep 27 2016, 3:21 PM

ovasileva moved this task from Triaged but Future to Upcoming on the Web-Team-Backlog board.Sep 28 2016, 6:04 PM

ovasileva added a project: Reading-Web-Sprint-83-Y?.Oct 3 2016, 4:19 PM

MBinder_WMF added a project: Reading-Web-Tech-Debt.Oct 3 2016, 8:34 PM

MBinder_WMF edited projects, added Technical-Debt (RW-Tech-Debt); removed Technical-Debt.Oct 4 2016, 6:15 PM

MBinder_WMF removed a project: Reading-Web-Tech-Debt.Oct 4 2016, 7:53 PM

• bmansurov moved this task from Uncategorized to Maintainability on the Technical-Debt (RW-Tech-Debt) board.Oct 5 2016, 10:34 PM

@phuedx - this ended up being lined up for next sprint. Is it ready or still blocked by T143891: Make MobileContext modular by creating the DeviceDetector service?

This is an optional last step. T144085 should probably be next up.

ovasileva removed a project: Reading-Web-Sprint-83-Y?.Oct 11 2016, 4:39 PM

ovasileva moved this task from Upcoming to Triaged but Future on the Web-Team-Backlog board.Oct 11 2016, 5:18 PM

@phuedx this task needs a big rewrite of its description. IT's not clear what's left to do! The TODO list is clear and I don't know what TODONE even means!!

All the changes in TOREVIEW have been merged.

MBinder_WMF moved this task from Needs Prioritization to 2016-17 Q1 on the Web-Team-Backlog board.Oct 10 2017, 7:43 PM

MBinder_WMF moved this task from 2016-17 Q1 to 2016-17 Q4 on the Web-Team-Backlog board.Mar 6 2018, 7:01 PM

Allow the MobileContext class to be broken apart by creating the MobileContext serviceClosed, ResolvedPublic5 Estimated Story PointsActions