Abstract `ContextManager` usages behind `CoreDataStack` #24191

mokagio · 2025-03-11T22:14:55Z

Problem I can't move AbstractPost and other Objective-C models in WordPressDataObj because they access ContextManager which is defined in the Swift WordPressData.

Solution Decouple from knowing about a specific type (ContextManager) and use a protocol instead (CoreDataStack). This came with a few compromises (see inline comments) but I think it got the job done relatively clean.

I tested this by running Jetpack on my iPhone and doing a smoke test checking the various tabs.

An alternative solution if we run into more Swift leakages into Objective-C would be to backtrack and create a framework target where we can have mixed languages. I might set aside some time today to search for #import "WordPress-Swift.h" usages and see how much extra steps like this would be needed if we kept the packages setup.

dangermattic · 2025-03-11T22:15:31Z

	1 Message
📖	This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

mokagio · 2025-03-11T22:32:15Z

WordPress/Classes/Services/JetpackSocialService.swift

-    @objc init(contextManager: ContextManager) {
-        self.coreDataStack = contextManager
+    @objc init(contextManager: CoreDataStack) {
+        guard let typeCastStack = contextManager as? CoreDataStackSwift else {
+            fatalError("Expected a CoreDataStackSwift-conforming type even though this initializer is marked @objc.")
+        }
+        self.coreDataStack = typeCastStack


Not proud of this one. I don't know if there's a neater way to go about it because if CoreDataStackSwift could be defined in Objective-C, then we wouldn't have needed it in the first place.

Luckily, this hack has to be used only twice, so I think we can live with it...?

Of course, one possible solution would be to convert the Objective-C consumers to Swift... But I think we ought to focus on moving forward with the Core Data migration at the moment.

IMO, it'd be okay to remove the argument and use the shared instance internally.

mokagio · 2025-03-11T22:32:59Z

WordPress/Classes/System/UITesting/UITestConfigurator.swift

-        ContextManager.shared.resetEverything()
+        //
+        // We can afford to force cast here because we are in a test-specific code
+        (ContextManager.shared as! ContextManager).resetEverything()


In particular, this code will run only in the UI tests and only if a certain launch argument is given

https://github.com/wordpress-mobile/WordPress-iOS/pull/24191/files#diff-666427fc460fbbf821022ce337766cb3f1f3e56a2900c16e041d0fb28c51ae19L26-L28

mokagio · 2025-03-11T22:34:03Z

WordPress/Classes/Models/Blog/Blog+SelfHosted.swift

@@ -17,7 +18,7 @@ extension Blog {

    static func createRestApiBlog(
        with details: WpApiApplicationPasswordDetails,
-        in contextManager: ContextManager,
+        in contextManager: CoreDataStackSwift,


In this PR, I decided not to rename the contextManager variable that now have CoreDataStackSwift to keep the diff smaller.

We might want to do it later on, but it does not seem urgent.

wpmobilebot · 2025-03-11T22:39:52Z

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr24191-cb9162d
	Version	25.8
	Bundle ID	org.wordpress.alpha
	Commit	`cb9162d`
	App Center Build	WPiOS - One-Offs #11635

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2025-03-11T22:40:28Z

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr24191-cb9162d
	Version	25.8
	Bundle ID	com.jetpack.alpha
	Commit	`cb9162d`
	App Center Build	jetpack-installable-builds #10666

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

crazytonyli

Sorry if I missed the discussion, but any reason not moving ContextManager into WordPressData too?

crazytonyli · 2025-03-12T00:10:56Z

WordPress/Classes/Services/JetpackSocialService.swift

-    @objc init(contextManager: ContextManager) {
-        self.coreDataStack = contextManager
+    @objc init(contextManager: CoreDataStack) {
+        guard let typeCastStack = contextManager as? CoreDataStackSwift else {
+            fatalError("Expected a CoreDataStackSwift-conforming type even though this initializer is marked @objc.")
+        }
+        self.coreDataStack = typeCastStack


IMO, it'd be okay to remove the argument and use the shared instance internally.

mokagio · 2025-03-12T01:00:09Z

@crazytonyli

Sorry if I missed the discussion, but any reason not moving ContextManager into WordPressData too?

ContextManager will eventually go into WordPressData, yes. (Current broken WIP PR that reveals all these issues #24166)

crazytonyli · 2025-03-12T01:06:47Z

I wonder whether moving ContextManager first would be easier for moving the managed objects. Like, this decoupling refactor would not be necessary if ContextManager is already in WordPressData.

mokagio · 2025-03-12T03:29:01Z

this decoupling refactor would not be necessary if ContextManager is already in WordPressData.

I think it would still be necessary if we have WordPressData the Swift package + WordPressDataObjC the Swift package with only Objective-C files. Because WordPressData depends on WordPressDataObjC, and therefore WordPressDataObjC cannot depend on WordPressData. But ContextManager is in WordPressData and the AbstractPost in WordPressDataObjc.

mokagio · 2025-03-12T03:30:30Z

@crazytonyli your question actually made me realize that some of the models are Objective-C and others are Swift. I'll need to quickly verify whether they can work sparse between the two packages, or if I should just call it quits on the package and use a framework instead :/

This is to decouple the Objective-C implementation from knowing about components (i.e. `ContextManager`) in the Swift layer.

mokagio force-pushed the mokagio/abstract-contextmanager branch from c4bae80 to 963f98e Compare March 11, 2025 22:26

mokagio commented Mar 11, 2025

View reviewed changes

mokagio changed the title ~~Replace all ContextManager sharedInstance() usages with shared~~ Abstract ContextManager usages behind CoreDataStack Mar 11, 2025

crazytonyli reviewed Mar 12, 2025

View reviewed changes

mokagio added 2 commits March 12, 2025 15:04

Remove duplicated CoreDataStackMock class

d02b1a9

Declare a sharedInstance in CoreDataStack

cb9162d

This is to decouple the Objective-C implementation from knowing about components (i.e. `ContextManager`) in the Swift layer.

mokagio force-pushed the mokagio/abstract-contextmanager branch from 963f98e to cb9162d Compare March 12, 2025 04:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abstract `ContextManager` usages behind `CoreDataStack` #24191

Abstract `ContextManager` usages behind `CoreDataStack` #24191

mokagio commented Mar 11, 2025 •

edited

Loading

dangermattic commented Mar 11, 2025

mokagio Mar 11, 2025

crazytonyli Mar 12, 2025

mokagio Mar 11, 2025

mokagio Mar 11, 2025

wpmobilebot commented Mar 11, 2025 •

edited

Loading

wpmobilebot commented Mar 11, 2025 •

edited

Loading

crazytonyli left a comment

crazytonyli Mar 12, 2025

mokagio commented Mar 12, 2025

crazytonyli commented Mar 12, 2025

mokagio commented Mar 12, 2025

mokagio commented Mar 12, 2025

Abstract ContextManager usages behind CoreDataStack #24191

Are you sure you want to change the base?

Abstract ContextManager usages behind CoreDataStack #24191

Conversation

mokagio commented Mar 11, 2025 • edited Loading

dangermattic commented Mar 11, 2025

mokagio Mar 11, 2025

Choose a reason for hiding this comment

crazytonyli Mar 12, 2025

Choose a reason for hiding this comment

mokagio Mar 11, 2025

Choose a reason for hiding this comment

mokagio Mar 11, 2025

Choose a reason for hiding this comment

wpmobilebot commented Mar 11, 2025 • edited Loading

wpmobilebot commented Mar 11, 2025 • edited Loading

crazytonyli left a comment

Choose a reason for hiding this comment

crazytonyli Mar 12, 2025

Choose a reason for hiding this comment

mokagio commented Mar 12, 2025

crazytonyli commented Mar 12, 2025

mokagio commented Mar 12, 2025

mokagio commented Mar 12, 2025

Abstract `ContextManager` usages behind `CoreDataStack` #24191

Abstract `ContextManager` usages behind `CoreDataStack` #24191

mokagio commented Mar 11, 2025 •

edited

Loading

wpmobilebot commented Mar 11, 2025 •

edited

Loading

wpmobilebot commented Mar 11, 2025 •

edited

Loading