Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move xcdatamodeld into WordPressData #24166

Draft
wants to merge 13 commits into
base: trunk
Choose a base branch
from

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Mar 7, 2025

Part of #24165


Note

Work in Progress. So far:

  • Moved the xcdatamodeld folder into the module and updated the code that referenced its URL to read it from the module
  • Run the app and saw it crash because it could not load BloggingPrompt (apparently the first model to load at runtime). The error was with Fatal error: NSArray element failed to match the Swift Array Element type . Expected BloggingPrompt got NSManagedObject.
  • Moved BloggingPrompt to WordPressData
  • App still crashed with Fatal error: NSArray element failed to match the Swift Array Element type . Expected BloggingPrompt got NSManagedObject
  • The only way I found to fix this was to hardcode the module name in the BloggingPrompt definition in the Core Data schema GUI

I don't like this solution because it would require editing the schema for each model. Besides, the compiler (or whichever piece is involved in going from the schema to the binary) should be able to inject the correct module name without hardcoding.

Leaving this up as a draft so that others can look at it.

The next thing I'll try will be to move more of the components involved in making the Core Data fetch request, the NSManagedObjectContext in particular, and see if it gets the namespace resolution to work without hardcoding.


Fixes #

To test:

Regression Notes

  1. Potential unintended areas of impact

  2. What I did to test those areas of impact (or what existing automated tests I relied on)

  3. What automated tests I added (or what prevented me from doing so)

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing checklist:

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

mokagio added 13 commits March 7, 2025 13:00
See how https://buildkite.com/automattic/wordpress-ios/builds/25994
failed:

```
error:
Build input file cannot be found:
'/opt/ci/builds/builder/automattic/wordpress-ios/DerivedData/Build/Intermediates.noindex/ArchiveIntermediates/WordPress Alpha/BuildProductsPath/Release-Alpha-iphoneos/WordPressDataObjC.o'.
Did you forget to declare this file as an output of a script phase or custom build rule which produces it? (in target 'WordPress' from project 'WordPress')
```
Not sure how it was working before — and in CI too, see
https://buildkite.com/automattic/wordpress-ios/builds/25995

My only guess is that the file became available in the DerivedData
somehow because the Swift package importing it was built before the app
targets?

Odd...
It was needed just because there was no file in the package yet, but
we've addressed it whenw e moved `CoreDataService` in there.
@dangermattic
Copy link
Collaborator

1 Warning
⚠️ View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.
1 Message
📖 This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

<entity name="BloggingPrompt" representedClassName=".BloggingPrompt" syncable="YES">
<entity name="BloggingPrompt" representedClassName="WordPressData.BloggingPrompt" syncable="YES">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before

image

After

image

Comment on lines +3 to +27
public let modelURL: URL = {
guard let url = Bundle.module.url(forResource: "WordPress", withExtension: "momd") else {
fatalError("Cannot find model file.")
}
return url
}()

public func urlForModel(name: String, in directory: String?) -> URL? {
let bundle = Bundle(for: TemporaryDummyClassToPickUpModule.self)
var url = bundle.url(forResource: name, withExtension: "mom", subdirectory: directory)

if url != nil {
return url
}

let momdPaths = bundle.paths(forResourcesOfType: "momd", inDirectory: directory)
momdPaths.forEach { (path) in
if url != nil {
return
}
url = bundle.url(forResource: name, withExtension: "mom", subdirectory: URL(fileURLWithPath: path).lastPathComponent)
}

return url
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These free functions should go into an object at some point.

It was just simpler to chuck them in here at this stage while I figure things out.

Also, I'd like to think there will be no need to have them public once all the types involved in setting up the Core Data stack have been moved.

@wpmobilebot
Copy link
Contributor

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr24166-b94d69d
Version25.8
Bundle IDcom.jetpack.alpha
Commitb94d69d
App Center Buildjetpack-installable-builds #10637
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr24166-b94d69d
Version25.8
Bundle IDorg.wordpress.alpha
Commitb94d69d
App Center BuildWPiOS - One-Offs #11606
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@kean
Copy link
Contributor

kean commented Mar 7, 2025

Have you tried overriding an Objective-C class name for the managed objects written in Swift?

[@objc](BloggingPrompt)
class BloggingPrompt: NSManagedObject {

@mokagio mokagio force-pushed the mokagio/share-classNameWithoutNamespaces branch from 81890b9 to 57a9e60 Compare March 7, 2025 20:56
Base automatically changed from mokagio/share-classNameWithoutNamespaces to trunk March 7, 2025 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants