Page MenuHomePhabricator

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

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.

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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

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
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.

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

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

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

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

phuedx removed phuedx as the assignee of this task.Sep 14 2016, 8:50 AM
phuedx assigned this task to Jhernandez.
phuedx added a subscriber: Jhernandez.

@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
phuedx added a subscriber: Legoktm.Sep 16 2016, 9:12 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.

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.

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 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:

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

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.

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

phuedx reassigned this task from phuedx to Jhernandez.Sep 23 2016, 5:55 AM

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

@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.

@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.

Jdlrobson reassigned this task from Jhernandez to phuedx.Apr 17 2017, 11:08 PM
Jdlrobson moved this task from Triaged but Future to Needs Analysis on the Readers-Web-Backlog board.

@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!!

phuedx closed this task as Resolved.Apr 18 2017, 4:41 AM

All the changes in TOREVIEW have been merged.