Update access of types required by Keystone for WordPressData link by mokagio · Pull Request #24420 · wordpress-mobile/WordPress-iOS

mokagio · 2025-04-07T12:09:56Z

I finally got to the point where WordPressData compiles in WordPress and Jetpack in #24378 and progressed to make it work from within Keystone.

Once I removed the WordPressData sources from Keystone, a few access level changes became necessary for Keystone to compile again, despite them looking unrelated to WordPressData.

While it's possible I made a mistake or that there was some caching issues going on, it seems safe to merge these + public changes and move on.

Similar to #24326, #24348, and #24406. Part of #24165.

dangermattic · 2025-04-07T12:10:39Z

	1 Warning
⚠️	This PR is assigned to the milestone 25.9. The due date for this milestone has already passed. Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

kean · 2025-04-07T12:19:54Z


-extension WordPressAppDelegate {
+public extension WordPressAppDelegate {
    @objc class var appURLScheme: String {


This code is removed in https://github.com/wordpress-mobile/WordPress-iOS/pull/24399/files#diff-ea62902a21f923873ab366b4e1f7bab41807abf9f5227c1709f6e65e8971df73.

Even better. I'll wait for that to land and rebase. If it's not landed by the time I come online tomorrow, I'll review it and land it mysefl.

kean · 2025-04-07T12:20:56Z

 /// cookie storage systems.
 ///
-@objc protocol CookieJar {
+@objc public protocol CookieJar {


I don't think you need it in WordPressData, do you?

I'll double check in #24378.

I made these changes as a result of some code not seeing CookieJar when building Keystone the cherry picked of #24378, but it's of course possible I made a mistake elsewhere or there was caching involved.

I'll let you know in my tomorrow 👀

@kean now that I come back to your comment in the morning instead of late in my evening I see that the question of target membership was for WordPressData, but I read it as for Keystone.

In that case, rest assured, the protocol is not part of WordPressData, only Keystone (and WordPress and Jetpack till the compilation with Keystone as a dependency is sorted).

In that case, it shouldn't need public modifiers, but they won't hurt.

Mmm... I reverted this in #24378 and only got these build failures

I'll rebase then...

This PR fixes it #24399.

wpmobilebot · 2025-04-07T12:32:52Z

📲 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	`27208`
	Version	`PR #24420`
	Bundle ID	`org.wordpress.alpha`
	Commit	`15a2fb1`
	Installation URL	7vmoo43k5m9fo

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

wpmobilebot · 2025-04-07T12:33:20Z

📲 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	`27208`
	Version	`PR #24420`
	Bundle ID	`com.jetpack.alpha`
	Commit	`15a2fb1`
	Installation URL	25bdkl8oqmhno

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

Apparently unnecessary, see #24420 (comment)

I finally got to the point where WordPressData compiles in WordPress and Jetpack in #24378 and progressed to make it work from within Keystone. Once I removed the WordPressData sources from Keystone, a few access level changes became necessary for Keystone to compile again, despite them looking unrelated to WordPressData. While it's possible I made a mistake or that there was some caching issues going on, it seems safe to merge these `+ public` changes and move on. Part of #24165.

Apparently unnecessary, see #24420 (comment)

kean · 2025-04-11T17:09:36Z

These changes are now obsolete – the affected code was updated in trunk, closing.

Apparently unnecessary, see #24420 (comment)

* Configure `Package.swift` with WordPressData framework deps * Add `WordPress-Swift.h` to WordPressData to work around build fail * Add prefix header * Add all required sources to WordPressData * Add required imports to Swift layer * Extract `defaultWordPressComAccountRestAPI` method to WordPressData Only the method, not the rest of the extension to avoid dependencies galore. * Add `Blog+SelfHosted` to WordPressData, including dependencies * Hack `ContextManager.swift` to compile * Replicate Keystone preprocessor macro hack for WordPressData-Swift.h * Add `Media+Blog.swift` to WordPressData to compile `PostHelper.m` * Add `RemotePost+Metadata` source to WordPressData for `PostHelper.m` * Add `PostServiceRemoteFactory` to WordPressData to fix `PostService.m` * Add `PostHelper+JetpackSocial` to WordPressData to fix `PostHelper.m` * Add `WordPressOrgRestApi+WordPress` to WordPressData Made possible by #24351 * Add `.m` file in WordPressData only to config CocoaLumberJack * Add some `import CocoaLumberJackSwift` required by WordPressData * Use `_private_wordPressComRestApi` in `Blog+WordPressComRestAPI.swift` to build WordPressData * Add CocoaLumberjack as a dependency to the extensions to fix compilation Otherwise, we'd get: ``` Undefined symbols for architecture arm64: "_OBJC_CLASS_$_DDLog", referenced from: in AppExtensionsService.o ld: symbol(s) not found for architecture arm64 clang: error: linker command failed with exit code 1 (use -v to see invocation) ``` * Remove `WordPress-Swift.h` hack from WordPressData * Add `Blog+Lookup` to WordPressData * Add `WordPress.xcdatamodeld` to WordPressData * Duplicate test infra in WordPressData in order to run `BlogTests` * Remove unnecessary `CocoaLumberJackConfiguration.h` header We don't need the header, the implementation file is enough for the CocoaLumberJack shenanigans to take place. * Remove WordPressData types from WordPress and Jetpack Not Keystone yet because I don't know if that compiles okay to verify the change. * Update imports after removing WordPressData files from WP and Jetpack * Disambiguate `Notification` usages — `/: Notification\)/` * Disambiguate `Notification` usages — `/: Notification$/` * Disambiguate `Notification` usages — `/, Notification(?!s)/` * Disambiguate `Notification` usages — `/as\? Notification/` * Disambiguate `Notification` usages — `/ Notification \{/` * Disambiguate `Notification` usages — `/ Notification\?/` * Disambiguate `Notification` usages — `/ Notification,$/` * Disambiguate `Notification` usages — `/\[Notification\]/` * Disambiguate `Notification` usages — `/ Notification\:/` * Disambiguate `Notification` usages — `/: Notification,/` * Disambiguate `Notification` usages — `/$Notification$/` * Disambiguate `Notification` usages — `/Notification else/` * Disambiguate `Notification` usages — `/Notification\!/` * Disambiguate `Notification` usages — `/as\? Notification/` * Add CocoaLumberjack to notification service extensions * Update imports after `Notification` disambiguation * Replace all `#import` of headers now in WordPressData with `@import WordPressData` * Work around (temporarily?!) a build failure after the WordPressData move * Remove unused `AccountService` import from `Blog.m` It matters because once `Blog` is in WordPressData, `AccountService` will no longer be available. * Move some `MenuItem` imports from `.h` to `.m` `MenuItem` was not used in the interface. Moving the import in the implementation will make it cleaner to update once WordPressData is integrated. * Remove WordPressData types from `WordPress-Bridging-Header.h` * Address module verifier warnings in WordPressData * Extract Swift-dependant logic from `Theme` * Make `Media` extension `public` * Remove unused WordPressData import from NotificationServiceExtension * Add CocoaLumberjack to extensions Jetpack uses to avoid compile failure Undefined symbol: _OBJC_CLASS_$_DDLog * Make WordPressData Swift API available to ObjC framework consumers * Add `testable import WordPressData` to unit tests * Update `WordPress.Notification` to `WordPressData.Notification` in tests * Add more required `@testable import WordPressData` Via ``` find WordPress/WordPressTest \ -name "*.swift" \ -type f \ -exec perl -i -0pe 's/(@testable import WordPress\n)(\nclass (\w+): CoreDataTestCase \{)/\1\@testable import WordPressData\n\nclass \3: CoreDataTestCase {/g' \ {} \; ``` and ``` find WordPress/WordPressTest \ -name "*.swift" \ -type f \ -exec perl -i -0pe 's/(@testable import WordPress\n)(\nfinal class (\w+): CoreDataTestCase \{)/\1\@testable import WordPressData\n\nfinal class \3: CoreDataTestCase {/g' \ {} \; ``` * Add WordPressData as a WordPressTests dependency * Add missing dependencies to WordPressTesting post WordPressData addition * Load model from the `ContextManager` bundle in the unit tests * Replace `AppEnvironment` with `WordPressComRestApi.apiBaseURL` in RestAPI Same rationale as 09990b8 * No longer access `_private_wordPressComRestApi` in `Blog` * Fix WordPressData linkage in Keystone and move sources * Update access of types required by Keystone for WordPressData link * Explicitly add WebKit as a Keystone dependency 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. * Revert `CookieJar` `public` access level Apparently unnecessary, see #24420 (comment) * REVERT ME - Only run WordPress tests * Switch all models to use "Global Namespace" 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 ``` * Replace all string values passed to `forEntityName` with `entityName()` 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' {} + ``` * Replace all `NSStringFromClass` values passed to `forEntityName` 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. * Replace all `Self.classNameWithoutNamespaces()` with `entityName()` 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. * Replace all `classNameWithoutNamespace()` calls with `entityName()` 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. * Fix WordPressData runtime issues (#24494) * 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) * Add `BlobEntity` to WordPressData * Address straightforward `FIXME` notes --------- Co-authored-by: Alex Grebenyuk <alex.grebenyuk@automattic.com>

mokagio added the Core Data Issues related to Core Data label Apr 7, 2025

mokagio added this to the 25.9 milestone Apr 7, 2025

mokagio self-assigned this Apr 7, 2025

kean reviewed Apr 7, 2025

View reviewed changes

mokagio force-pushed the mokagio/wordpressdata-precursor-cvxcvzc branch from 9fd7416 to ce3b155 Compare April 7, 2025 12:21

mokagio added a commit that referenced this pull request Apr 7, 2025

Revert CookieJar public access level

664eb06

Apparently unnecessary, see #24420 (comment)

mokagio added a commit that referenced this pull request Apr 7, 2025

Revert CookieJar public access level

41476a1

Apparently unnecessary, see #24420 (comment)

mokagio added 2 commits April 8, 2025 09:48

Revert CookieJar public access level

15a2fb1

Apparently unnecessary, see #24420 (comment)

mokagio force-pushed the mokagio/wordpressdata-precursor-cvxcvzc branch from 41476a1 to 15a2fb1 Compare April 7, 2025 23:48

mokagio added a commit that referenced this pull request Apr 11, 2025

Revert CookieJar public access level

7aa6eca

Apparently unnecessary, see #24420 (comment)

mokagio added a commit that referenced this pull request Apr 11, 2025

Revert CookieJar public access level

c55fb07

Apparently unnecessary, see #24420 (comment)

kean closed this Apr 11, 2025

mokagio deleted the mokagio/wordpressdata-precursor-cvxcvzc branch April 14, 2025 23:50

mokagio added a commit that referenced this pull request Apr 23, 2025

Revert CookieJar public access level

4f2d1ba

Apparently unnecessary, see #24420 (comment)

mokagio added a commit that referenced this pull request Apr 23, 2025

Revert CookieJar public access level

3f1727e

Apparently unnecessary, see #24420 (comment)

mokagio added a commit that referenced this pull request May 1, 2025

Revert CookieJar public access level

0220033

Apparently unnecessary, see #24420 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update access of types required by Keystone for WordPressData link - #24420

Update access of types required by Keystone for WordPressData link#24420
mokagio wants to merge 2 commits into
trunkfrom
mokagio/wordpressdata-precursor-cvxcvzc

mokagio commented Apr 7, 2025

Uh oh!

dangermattic commented Apr 7, 2025

Uh oh!

kean Apr 7, 2025

Uh oh!

mokagio Apr 7, 2025

Uh oh!

kean Apr 7, 2025

Uh oh!

mokagio Apr 7, 2025

Uh oh!

mokagio Apr 7, 2025

Uh oh!

kean Apr 7, 2025

Uh oh!

mokagio Apr 7, 2025

Uh oh!

kean Apr 8, 2025 •

edited

Loading

Uh oh!

wpmobilebot commented Apr 7, 2025 •

edited

Loading

Uh oh!

wpmobilebot commented Apr 7, 2025 •

edited

Loading

Uh oh!

kean commented Apr 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

mokagio commented Apr 7, 2025

Uh oh!

dangermattic commented Apr 7, 2025

Uh oh!

kean Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

mokagio Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

kean Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

mokagio Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

mokagio Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

kean Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

mokagio Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

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

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!

kean commented Apr 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kean Apr 8, 2025 •

edited

Loading

wpmobilebot commented Apr 7, 2025 •

edited

Loading

wpmobilebot commented Apr 7, 2025 •

edited

Loading