Move model files from apps targets to WordPressData by mokagio · Pull Request #24378 · wordpress-mobile/WordPress-iOS

mokagio · 2025-04-01T11:13:46Z

Builds on top of #24374, with standalone commits from #24375 and #24376, part of #24165.

dangermattic · 2025-04-01T11:14:26Z

	3 Warnings
⚠️	`Modules/Package.swift` was changed without updating its corresponding `Package.resolved`. Please resolve the Swift packages as appropriate to your project setup (e.g. in Xcode or by running `swift package resolve`).
⚠️	View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.
⚠️	This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

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

Generated by 🚫 Danger

mokagio · 2025-04-01T20:40:07Z

@@ -0,0 +1 @@
+// Here only to have the matching .m where to define the DDLogLevel value.


Might not even be necessary. Gotta try that...

mokagio · 2025-04-01T20:40:24Z

+#ifndef WordPress_Swift_h
+#define WordPress_Swift_h
+// FIXME: Defined here to avoid compilation errors while files are migrated over...
+#endif /* WordPress_Swift_h */


Should be able to remove this now...

mokagio · 2025-04-01T20:47:02Z

    var wordPressComRestApi: WordPressComRestApi? {
-        account?.wordPressComRestApi
+        // FIXME: We are banking on the fact that by the time a consumer reads this, WPAccount will already have initialized its API client instance.
+        account?._private_wordPressComRestApi


Maybe we could decouple WPAccount wordPressComRestApi from WordPressAuthenticator using... a Notification?

WordPress-iOS/WordPress/Classes/Models/WPAccount+RestApi.swift

Lines 12 to 20 in b14619b

@objc var wordPressComRestApi: WordPressComRestApi? {

if let api = _private_wordPressComRestApi {

return api

}

guard !authToken.isEmpty else {

DispatchQueue.main.async {

WordPressAuthenticationManager.showSigninForWPComFixingAuthToken()

}

return nil

WordPress-iOS/WordPress/Classes/Models/WPAccount+RestApi.swift

Lines 27 to 38 in b14619b

private func makeWordPressComRestApi() -> WordPressComRestApi {

let api = WordPressComRestApi.defaultApi(

oAuthToken: authToken,

userAgent: WPUserAgent.defaultUserAgent(),

localeKey: WordPressComRestApi.LocaleKeyDefault

)

api.setInvalidTokenHandler { [weak self] in

guard let self else { return }

self.authToken = nil

WordPressAuthenticationManager.showSigninForWPComFixingAuthToken()

I suggest a Notification because I don't think injecting callbacks or using a delegate would be feasible with a model like this.

mokagio · 2025-04-02T05:09:29Z

-            return [self supportsRestApi] && [JetpackFeaturesRemovalCoordinator jetpackFeaturesEnabled];
+            // FIXME: See https://github.com/wordpress-mobile/WordPress-iOS/pull/24332
+//            return [self supportsRestApi] && [JetpackFeaturesRemovalCoordinator jetpackFeaturesEnabled];
+            return [self supportsRestApi];
        case BlogFeatureTenor:
-            return [JetpackFeaturesRemovalCoordinator jetpackFeaturesEnabled];
+            // FIXME: See https://github.com/wordpress-mobile/WordPress-iOS/pull/24332
+//            return [JetpackFeaturesRemovalCoordinator jetpackFeaturesEnabled];
+            return NO;


#24393 should address this.

mokagio · 2025-04-02T05:12:08Z


-        let startupEvent = SentryStartupEvent()
+        // FIXME: Import the Sentry stuff, too — But it accesses the app delegate!
+        // let startupEvent = SentryStartupEvent()


@crazytonyli is this ContextManager Sentry logging something that we need, or is it by any chance something that we added at the time of adoption just to monitor 🤞 ?

Being able to just chop it off would simplify things.

See also p1743115991824589/1743090023.463689-slack-C08HQR4C2TS

I'm sorry I missed this message. I don't think I added the events. It's very likely that they were there before my time. I doubt these events ever occur on production. I did a quick search on Sentry and don't see any related events. However, I think it's best to keep them in case we introduce some issues to the Core Data layer. At the bare minimum, we can send the events to the app via a notification.

wpmobilebot · 2025-04-07T01:51:01Z

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

	App Name	Jetpack
	Configuration	Release-Alpha
	Build Number	`27563`
	Version	`PR #24378`
	Bundle ID	`com.jetpack.alpha`
	Commit	`255fd0c`
	Installation URL	4pqsd6d4r1f50

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

wpmobilebot · 2025-04-07T01:51:04Z

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

	App Name	WordPress
	Configuration	Release-Alpha
	Build Number	`27563`
	Version	`PR #24378`
	Bundle ID	`org.wordpress.alpha`
	Commit	`255fd0c`
	Installation URL	035hhosmgq1go

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

…tAPI Same rationale as 09990b8

Triggered by a few errors like ``` In file included from /Users/gio/Developer/a8c/wpios/WordPress/Classes/ViewRelated/Suggestions/SuggestionsTableViewCell.m:3: /Users/gio/Developer/a8c/wpios/DerivedData/WordPress/Build/Intermediates.noindex/WordPress.build/Debug-iphonesimulator/Keystone.build/DerivedSources/Keystone-Swift.h:295:9: fatal error: module 'WebKit' not found 295 | @import WebKit; | ~~~~~~~^~~~~~ ``` which I got after linking WordPressData against Keystone and moving all the sources there.

Apparently unnecessary, see #24420 (comment)

Via ``` sed -i '' -E 's/representedClassName="(WordPress)?(\.)?([^"]+)"/representedClassName=".\3"/g' \ Sources/WordPressData/Resources/WordPress.xcdatamodeld/WordPress\ 155.xcdatamodel/contents ``` which switched to "Current Product Module" but resulted in unit tests failure in WordPressData, followed by ``` ➜ sed -i '' -E 's/representedClassName="\.([^"]+)"/representedClassName="\1"/g' \ Sources/WordPressData/Resources/WordPress.xcdatamodeld/WordPress\ 155.xcdatamodel/contents ```

Done via ``` find . \ \( -path './DerivedData' -o -path './.git' -o -path './vendor' \) \ -prune -o -type f -name "*.swift" \ -exec sed -i '' -E 's/forEntityName: "([A-Za-z_][A-Za-z0-9_]*)"/forEntityName: \1.entityName()/g' {} + ```

Done via ``` find . \ \( -path './DerivedData' -o -path './.git' -o -path './vendor' \) \ -prune -o -type f -name "*.swift" \ -exec sed -i '' -E 's/forEntityName: NSStringFromClass\(([A-Za-z_][A-Za-z0-9_]*)\.self\)/forEntityName: \1.entityName()/g' {} + ``` The goal is to have a single way to get the entity name so we can adapt it to the new WordPressData setup in one go.

The goal is to have a single way to get the entity name so we can adapt it to the new WordPressData setup in one go.

Done via ``` find . \( -path './DerivedData' -o -path './.git' -o -path './vendor' \) \ -prune -o -type f -name "*.swift" \ -exec sed -i '' -E 's/forEntityName: ([A-Za-z_][A-Za-z0-9_]*)\.classNameWithoutNamespaces\(\)/forEntityName: \1.entityName()/g' {} + ``` The goal is to have a single way to get the entity name so we can adapt it to the new WordPressData setup in one go.

* Move remaining types to WordPressData * Use Current Module for Swift-only types * Fix ReaderTeamTopic compilation * Add missing class names * Remove unused SiteInfo * Add Bundle.wordPressData and update DataMigratorTests * Update CoreDataMigrationTests * Fix unit tests (AppEnvironment required)

mokagio · 2025-05-01T02:19:29Z

        }

-        let startupEvent = SentryStartupEvent()
+        // FIXME: Import the Sentry stuff, too — But it accesses the app delegate!


https://linear.app/a8c/issue/AINFRA-383

mokagio · 2025-05-01T02:21:03Z

+    // FIXME: Crashes because WPAccount fixture sets username and triggers BuildSettings access
+//    func testThatPluginManagementIsEnabledForJetpack() {
+//        let blog = BlogBuilder(mainContext)
+//            .withAnAccount()
+//            .withJetpack(version: "5.6", username: "test_user", email: "user@example.com")
+//            .with(isHostedAtWPCom: false)
+//            .with(isAdmin: true)
+//            .build()
+//
+//        XCTAssertTrue(blog.supports(.pluginManagement))
+//    }


https://linear.app/a8c/issue/AINFRA-384

mokagio · 2025-05-01T02:31:47Z

+/// Creates a Blog
+///
+/// Defaults to creating a self-hosted blog
+// FIXME: Currently duplicated from WordPressTests for ease of working. Eventually, should be extracted in a WordPressDataTestsHelper framework to share here and with the consumers.


https://linear.app/a8c/issue/AINFRA-385

sonarqubecloud · 2025-05-01T02:46:43Z

Quality Gate failed

Failed conditions
1 Security Hotspot

See analysis details on SonarQube Cloud

kean · 2025-05-01T15:54:48Z

+                // EmitSwiftModule normal arm64 (in target 'WordPressNotificationServiceExtension' from project 'WordPress')
+                //    cd /path/to/repo/WordPress
+                //
+                // <unknown>:0: error: missing required modules: 'CocoaLumberjack', 'CocoaLumberjackSwiftSupport'


(nit, let's address later) we now have WPLogging.h for use in non-app targets (or any targets)

Noted. And tracked in https://linear.app/a8c/issue/AINFRA-424/adopt-wploggingh

kean · 2025-05-01T15:58:37Z

+                //
+                "BuildSettingsKit",
+                "FormattableContentKit",
+                "SFHFKeychainUtils",


This is duplicated dependency which produces a new warning.

Suggested change

"SFHFKeychainUtils",

I didn't see this comment, but noticed the warning elsewhere and addressed via #24514

kean · 2025-05-01T16:06:05Z

 				GCC_PREPROCESSOR_DEFINITIONS = (
 					"DEBUG=1",
 					"$(inherited)",
+					"WPDATA=1",


(nit) seems to be unused

Good catch! Thanks!

#24517

mokagio mentioned this pull request Apr 1, 2025

Tweak a few file to allow WordPressData to compile #24375

Merged

mokagio mentioned this pull request Apr 1, 2025

Remove main.swift from Keystone #24383

Merged

14 tasks

Base automatically changed from mokagio/wordpressdata-target-xcdatamodeld-workbench-2 to trunk April 1, 2025 18:54

mokagio force-pushed the mokagio/wordpressdata-target-xcdatamodeld-workbench-2-move-files branch from 5e4258a to 8b47455 Compare April 1, 2025 20:38

mokagio commented Apr 1, 2025

View reviewed changes

mokagio mentioned this pull request Apr 2, 2025

Build and test Keystone and WordPressData in CI #24391

Merged

mokagio force-pushed the mokagio/wordpressdata-target-xcdatamodeld-workbench-2-move-files branch from 710846d to 8e8a6d7 Compare April 2, 2025 03:44

mokagio commented Apr 2, 2025

View reviewed changes

mokagio mentioned this pull request Apr 3, 2025

Work bench PR for one-go Core Data extraction into target #24242

Closed

mokagio force-pushed the mokagio/wordpressdata-target-xcdatamodeld-workbench-2-move-files branch from 8e8a6d7 to caa0730 Compare April 3, 2025 06:10

mokagio mentioned this pull request Apr 3, 2025

Remove WordPressDataObjC and WordPressData modules #24402

Merged

mokagio force-pushed the mokagio/wordpressdata-target-xcdatamodeld-workbench-2-move-files branch from caa0730 to f15a95b Compare April 3, 2025 10:56

mokagio mentioned this pull request Apr 3, 2025

Update access for some methods soon to be in WordPressData #24403

Merged

mokagio force-pushed the mokagio/wordpressdata-target-xcdatamodeld-workbench-2-move-files branch 3 times, most recently from f0b8baf to 9747c86 Compare April 4, 2025 03:50

This was referenced Apr 5, 2025

Update some imports in preparation for WordPressData #24411

Merged

Remove unused save and remove from Media #24416

Merged

mokagio force-pushed the mokagio/wordpressdata-target-xcdatamodeld-workbench-2-move-files branch 3 times, most recently from 1b81897 to c5f7a76 Compare April 7, 2025 01:38

mokagio force-pushed the mokagio/wordpressdata-target-xcdatamodeld-workbench-2-move-files branch from c5f7a76 to 35a3999 Compare April 7, 2025 03:21

mokagio mentioned this pull request Apr 7, 2025

Add remaining public APIs for Keystone #24406

Merged

14 tasks

mokagio force-pushed the mokagio/wordpressdata-target-xcdatamodeld-workbench-2-move-files branch from 35a3999 to 85779b2 Compare April 7, 2025 05:11

mokagio and others added 15 commits May 1, 2025 11:27

Load model from the ContextManager bundle in the unit tests

5232a45

Replace AppEnvironment with WordPressComRestApi.apiBaseURL in Res…

db6a357

…tAPI Same rationale as 09990b8

No longer access _private_wordPressComRestApi in Blog

57d01e3

Fix WordPressData linkage in Keystone and move sources

0472e97

Update access of types required by Keystone for WordPressData link

a9bdc8a

Revert CookieJar public access level

0220033

Apparently unnecessary, see #24420 (comment)

REVERT ME - Only run WordPress tests

6fa16d0

Replace all Self.classNameWithoutNamespaces() with entityName()

84973ec

The goal is to have a single way to get the entity name so we can adapt it to the new WordPressData setup in one go.

Add BlobEntity to WordPressData

200bb3e

mokagio force-pushed the mokagio/wordpressdata-target-xcdatamodeld-workbench-2-move-files branch from 200048d to 200bb3e Compare May 1, 2025 01:53

mokagio commented May 1, 2025

View reviewed changes

Address straightforward FIXME notes

255fd0c

mokagio marked this pull request as ready for review May 1, 2025 03:14

mokagio requested review from crazytonyli and kean May 1, 2025 03:14

mokagio enabled auto-merge May 1, 2025 03:15

kean approved these changes May 1, 2025

View reviewed changes

mokagio added this pull request to the merge queue May 1, 2025

Merged via the queue into trunk with commit c062a59 May 1, 2025

mokagio deleted the mokagio/wordpressdata-target-xcdatamodeld-workbench-2-move-files branch May 1, 2025 16:30

		@@ -0,0 +1 @@
		// Here only to have the matching .m where to define the DDLogLevel value.

	@objc var wordPressComRestApi: WordPressComRestApi? {
	if let api = _private_wordPressComRestApi {
	return api
	}
	guard !authToken.isEmpty else {
	DispatchQueue.main.async {
	WordPressAuthenticationManager.showSigninForWPComFixingAuthToken()
	}
	return nil

	private func makeWordPressComRestApi() -> WordPressComRestApi {
	let api = WordPressComRestApi.defaultApi(
	oAuthToken: authToken,
	userAgent: WPUserAgent.defaultUserAgent(),
	localeKey: WordPressComRestApi.LocaleKeyDefault
	)

	api.setInvalidTokenHandler { [weak self] in
	guard let self else { return }

	self.authToken = nil
	WordPressAuthenticationManager.showSigninForWPComFixingAuthToken()

Uh oh!

Conversation

mokagio commented Apr 1, 2025

Uh oh!

dangermattic commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wpmobilebot commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented May 1, 2025

Quality Gate failed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dangermattic commented Apr 1, 2025 •

edited

Loading

wpmobilebot commented Apr 7, 2025 •

edited

Loading

wpmobilebot commented Apr 7, 2025 •

edited

Loading