Remove the Core Data workaround in EditorMediaUtility #21227
Conversation
|
| App Name |  Jetpack Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr21227-849f018 | |
| Version | 22.9 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 849f018 | |
| App Center Build | jetpack-installable-builds #5658 |
|
| App Name |  WordPress Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr21227-849f018 | |
| Version | 22.9 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 849f018 | |
| App Center Build | WPiOS - One-Offs #6623 |
| init(with blog: Blog) { | ||
| self.init(with: blog) { error in | ||
| // We'll log the error, so we know it's there, but we won't halt execution. | ||
| WordPressAppDelegate.crashLogging?.logError(error) | ||
| } | ||
| } | ||
|
|
||
| init(with blog: Blog, failure: (BlogError) -> ()) { |
There was a problem hiding this comment.
Have you considered using argument default for this over a dedicated init?
E.g.
init(
with blog: Blog,
failure: (BlogError) -> () = { error in
// ...
}
) {Granted this approach assumes that the logging mechanism will remain a singleton shared instance, which might not be desirable in the future.
| let blogObjectID: NSManagedObjectID | ||
| let mediaHost: MediaHost |
There was a problem hiding this comment.
From the PR description:
AuthenticatedImageDownloadno long accessNSManagedObject. It used to have code to queryBlog. The code is now moved to its caller.
Just wanted to +1 this decision. Aside from the hunch that this will help with #20630, having the image download operation know about MediaHost over Blog seems like a better design because the surface of information it can access (thus that we need to be aware of when working with it) is now much smaller.
| // This function is added to debug the issue linked below. | ||
| let safeExistingObject: (NSManagedObjectID) throws -> NSManagedObject = { objectID in | ||
| var object: Result<NSManagedObject, Error> = .failure(AuthenticatedImageDownload.DownloadError.blogNotFound) | ||
| do { | ||
| // Catch an Objective-C `NSInvalidArgumentException` exception from `existingObject(with:)`. | ||
| // See https://github.com/wordpress-mobile/WordPress-iOS/issues/20630 | ||
| try WPException.objcTry { | ||
| object = Result { | ||
| try context.existingObject(with: objectID) | ||
| } | ||
| } | ||
| } catch { | ||
| // Send Objective-C exceptions to Sentry for further diagnosis. | ||
| WordPressAppDelegate.crashLogging?.logError(error) | ||
| throw error | ||
| } | ||
|
|
||
| return try object.get() | ||
| } | ||
|
|
||
| let post = try safeExistingObject(postObjectID) as! Post |
There was a problem hiding this comment.
This is neat.
Hopefully we won't have to use it again, but just in case, I wonder how we could make it more discoverable / flexible so other people can reuse it? 🤔
There was a problem hiding this comment.
I really hope that this PR fixes the issue and makes the extra Objective-C exception become unnecessary. That means we can delete this ugly try catch.
I don't want to move this into a shared places because I don't think we should catch the NSInvalidArgument exception, which to me feels like an indication of programming error, rather than Core Data data accessing error.
mokagio
force-pushed
the
fix-core-data-issue-in-downloading-image
branch
from
7713175 to
849f018
Compare
Generated by 🚫 dangerJS |
This is an attempt to fix #20630. The issue is the
NSManagedObjectContext.existingObject(with:)call inAuthenticatedImageDownloadthrows an Objective-C exception.The root cause is unknown. But this PR cleans up the code that accesses Core Data objects. Hopefully now that the Core Data objects (
PostandBloginstances) are now used correctly, the issue would go away.There are two main code changes:
AuthenticatedImageDownloadno long accessNSManagedObject. It used to have code to queryBlog. The code is now moved to its caller.EditorMediaUtilit.downloadImagefunction is refactored to access thePostargument safely usingperformAndQuery.Test Instructions
Make sure Gutenberg editor and Aztec editor both can still load images.
Regression Notes
Potential unintended areas of impact
None.
What I did to test those areas of impact (or what existing automated tests I relied on)
What's described in the Test Instructions.
What automated tests I added (or what prevented me from doing so)
None.
PR submission checklist:
RELEASE-NOTES.txtif necessary.UI Changes testing checklist: N/A