Remove legacy Core Data stack #19770

crazytonyli · 2022-12-12T21:39:32Z

Changes

Given there is no major issue found in the ContainerContextFactory, we can now migrate to use the new Core Data stack fully. This PR removes the legacy Core Data stack (LegacyContextFactory) and merge ContainerContextFactory into ContextManager.

This PR also translates ContextManager to Swift, so that we can use all the nice things Swift provides us in the very foundation of our Core Data stack.

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)
None.
What automated tests I added (or what prevented me from doing so)
None.

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.

peril-wordpress-mobile · 2022-12-12T21:39:38Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

wpmobilebot · 2022-12-12T22:05:28Z

You can test the changes in WordPress from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr19770-1e1d18e on your iPhone

If you need access to App Center, please ask a maintainer to add you.

wpmobilebot · 2022-12-12T22:06:50Z

You can test the changes in Jetpack from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr19770-1e1d18e on your iPhone

If you need access to App Center, please ask a maintainer to add you.

crazytonyli · 2022-12-12T22:16:01Z

WordPress/Classes/Utility/ContextManager.swift

+                startupEvent.add(error: error)
+            }
+
+            // TODO: The original Objective C implement calls `loadPersistentStores` again here, but throws an exception regardless the result.


❓ I can certainly keep the code that calls loadPersistentStores again, but I don't see how this additional call is necessary, did I miss anything?

I don't see how this additional call is necessary

Me neither.

…tFactory`

mokagio · 2022-12-15T12:07:54Z

WordPress/Classes/Services/BlogService.m

 #import "PostService.h"
 #import "TodayExtensionService.h"
-#import "ContextManager.h"
+#import "CoreDataStack.h"


It looks like this import is duplicated from line 5.

Suggested change

#import "CoreDataStack.h"

mokagio · 2022-12-15T12:08:47Z

WordPress/Classes/Utility/ContextManager.swift

+import Foundation
+import CoreData
+
+///A constant representing the current version of the data model.


Suggested change

///A constant representing the current version of the data model.

/// A constant representing the current version of the data model.

mokagio · 2022-12-15T12:11:26Z

WordPress/WordPressTest/CoreDataTestCase.swift

@@ -1,5 +1,7 @@
 import XCTest



Nitpick: Does this empty line signifies something or can we remove it?

Suggested change

mokagio · 2022-12-15T12:19:03Z

WordPress/Classes/Utility/ContextManager.swift

+        /// Ensure that the `context`'s concurrency type is not `confinementConcurrencyType`, since it will crash if `perform` or `performAndWait` is called.
+        guard context.concurrencyType == .mainQueueConcurrencyType || context.concurrencyType == .privateQueueConcurrencyType else {


The comment says we want to check that the context != x. The code checks that == y || == z.

The difference might be important. I think it would be good to keep code and comment consistent.

Also, nitpick, /// will result in a documentation header comment, but that doesn't apply for this kind of comment since it's not on a definition but on a guard statement.

The original PR chose to not reference the deprecated confinementConcurrencyType to avoid a warning.

mokagio · 2022-12-15T12:20:07Z

WordPress/Classes/Utility/ContextManager.swift

+            block()
+            return


Is running the given block a fallback mechanism to keep the app working without having to do deal with the runtime complexity of handling an error if the context is incorrect?

Calling performBlock on a context whose concurrency type is confinement crashes, see the original PR for context.

mokagio · 2022-12-15T12:20:44Z

WordPress/Classes/Utility/ContextManager.swift

+                startupEvent.add(error: error)
+            }
+
+            // TODO: The original Objective C implement calls `loadPersistentStores` again here, but throws an exception regardless the result.


I don't see how this additional call is necessary

Me neither.

mokagio · 2022-12-15T12:24:36Z

Given there is no major issue found in the ContainerContextFactory, we can now migrate to use the new Core Data stack fully.

For breadcrumbs, see #19433 where we switched to use the new Core Data stack via feature toogle. That code has been running in production for more than a month.

crazytonyli added 3 commits December 13, 2022 09:25

Remove LegacyContextFactory

f7558bd

Translating ContextManager to Swift

5828347

Reinstate PR #19765

f7c9c8c

crazytonyli added the Core Data Issues related to Core Data label Dec 12, 2022

crazytonyli added this to the 21.5 milestone Dec 12, 2022

crazytonyli requested a review from a team December 12, 2022 21:39

crazytonyli self-assigned this Dec 12, 2022

Rename a test file

264c86b

crazytonyli commented Dec 12, 2022

View reviewed changes

crazytonyli added 2 commits December 13, 2022 21:36

Missed the code that sets mergePolicy when merging `ContainerContex…

ef467fd

…tFactory`

Remove a semicolon

285fda8

mokagio approved these changes Dec 15, 2022

View reviewed changes

crazytonyli added 2 commits December 20, 2022 10:55

Various minor changes

f3ec3f2

Remove a TODO

1e1d18e

crazytonyli enabled auto-merge December 19, 2022 21:56

crazytonyli merged commit 0ffb6a0 into trunk Dec 19, 2022

crazytonyli deleted the remove-legacy-context-factory branch December 19, 2022 22:54

crazytonyli mentioned this pull request Feb 27, 2023

Fix database migration issue with different database models #20214

Closed

4 tasks

	///A constant representing the current version of the data model.
	/// A constant representing the current version of the data model.

		/// Ensure that the `context`'s concurrency type is not `confinementConcurrencyType`, since it will crash if `perform` or `performAndWait` is called.
		guard context.concurrencyType == .mainQueueConcurrencyType \|\| context.concurrencyType == .privateQueueConcurrencyType else {

Remove legacy Core Data stack #19770

Remove legacy Core Data stack #19770

Uh oh!

Conversation

crazytonyli commented Dec 12, 2022

Changes

Regression Notes

Uh oh!

peril-wordpress-mobile bot commented Dec 12, 2022

Uh oh!

wpmobilebot commented Dec 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Dec 12, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mokagio commented Dec 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wpmobilebot commented Dec 12, 2022 •

edited

Loading

wpmobilebot commented Dec 12, 2022 •

edited

Loading