Add nullability annotations to WPAccount #24328

crazytonyli · 2025-03-26T23:21:39Z

This PR should fix the crash in #24320

WordPress/WPAccount+RestApi.swift:15: Fatal error: Unexpectedly found nil while implicitly unwrapping an Optional value

The crash happens at authToken.isEmpty. authToken can be nil, but the Objective-C API didn't have nullabilty annotation which makes it an IUO property.

To test:

Regression Notes

Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
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:

dangermattic · 2025-03-26T23:22:09Z

	2 Warnings
⚠️	View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.
⚠️	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-03-26T23:24:54Z

WordPress/Classes/Models/WPAccount.h

+
+@property (nonatomic, strong, nullable)   NSNumber    *userID;
+@property (nonatomic, strong, nullable)   NSString    *avatarURL;
+@property (nonatomic, copy, nonnull)     NSString    *username;


(nit) nonnull is redundant.

Are we sure username is nonnull?

Yes, according to the Core Data schema.

kean · 2025-03-26T23:25:42Z

WordPress/Classes/Models/WPAccount+RestApi.swift

        get {
            guard let api = objc_getAssociatedObject(self, &apiAssociatedKey) as? WordPressComRestApi else {
-                guard authToken.isEmpty else {
+                if let authToken, !authToken.isEmpty {


I'm also changing these lines here #24327 👀

I just saw your PR. I can resolve the conflicts once yours is merged. 😄

My PR is missing the nullability check, so this change will be needed.

kean · 2025-03-26T23:26:32Z

WordPress/Classes/Models/WPAccount+Lookup.swift

    @objc
    var hasBlogs: Bool {
-        return !blogs.isEmpty
+        return blogs?.isEmpty == false


(nit) Is there any way to make blogs in WPAccount nonnull for convenience?

At runtime, it probably won't be null. Core Data should return an empty list if there is no Blog instance associated with this account. However, considering an "Optional" property in the relationship configuration, I feel it is safer to declare it nullable.

wpmobilebot · 2025-03-26T23:32:33Z

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

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr24328-d9f9d89
	Version	25.8
	Bundle ID	org.wordpress.alpha
	Commit	`d9f9d89`
	App Center Build	WPiOS - One-Offs #11860

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

wpmobilebot · 2025-03-26T23:33:08Z

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

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr24328-d9f9d89
	Version	25.8
	Bundle ID	com.jetpack.alpha
	Commit	`d9f9d89`
	App Center Build	jetpack-installable-builds #10883

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

wpmobilebot · 2025-03-30T23:00:59Z

📲 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	`27020`
	Version	`PR #24328`
	Bundle ID	`com.jetpack.alpha`
	Commit	`ddc2105`
	Installation URL	3nr1nu8s78mbg

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

wpmobilebot · 2025-03-30T23:01:10Z

📲 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	`27020`
	Version	`PR #24328`
	Bundle ID	`org.wordpress.alpha`
	Commit	`ddc2105`
	Installation URL	4s4g51ihlfoo8

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

kean · 2025-03-31T13:08:34Z

WordPress/Classes/Models/BloggingPromptSettings+CoreDataClass.swift

-        } else if let account = try WPAccount.lookupDefaultWordPressComAccount(in: context) {
-            siteID = account.primaryBlogID
+        } else if let account = try WPAccount.lookupDefaultWordPressComAccount(in: context), let primaryBlogID = account.primaryBlogID {
+            return nil


Looks like a function change. Previous:

siteID = account.primaryBlogID ```

Good catch! Fixed in a185c04

kean · 2025-03-31T13:09:08Z

WordPress/Classes/Models/WPAccount.h

+
+@property (nonatomic, strong, nullable)   NSNumber    *userID;
+@property (nonatomic, strong, nullable)   NSString    *avatarURL;
+@property (nonatomic, copy, nonnull)     NSString    *username;


Are we sure username is nonnull?

kean · 2025-03-31T13:10:37Z

WordPress/Classes/Networking/WordPressClient.swift

-        if let account = blog.account {
-            self.init(baseUrl: url, type: .dotCom(authToken: account.authToken))
+        if let account = blog.account, let authToken = account.authToken {
+            self.init(baseUrl: url, type: .dotCom(authToken: authToken))


As long as a blog has an associated WP.com, it should be using a .dotCom client.

All WPAccount instances should have an authToken because they are created after signing into WP.com. We can change the WordPressClient to make it support anonymous API requests, but I don't think that's necessary because it won't be used to make anonymous API calls?

The issue is that if account.authToken is nil, it will now fall into the else with .selfHosted, which is the logic change. Should it throw an error instead?

Ah of course. I have addressed the issue in 8af7561.

kean · 2025-04-01T19:51:42Z

I'm going to merge it if you don't mind – need some of this code in another branch.

crazytonyli · 2025-04-01T20:13:58Z

@kean 👍 Thanks!

Add nullability annotations to WPAccount

d9f9d89

crazytonyli added the [Type] Tech Debt label Mar 26, 2025

crazytonyli added this to the 25.9 milestone Mar 26, 2025

crazytonyli requested review from kean, mokagio and pmusolino March 26, 2025 23:21

kean reviewed Mar 26, 2025

View reviewed changes

crazytonyli added 2 commits March 31, 2025 11:45

Merge branch 'trunk' into fix-wp-account-nullablity

6462d86

Fix compiling issues after resolving conflicts

6204d41

crazytonyli requested a review from kean March 30, 2025 22:49

kean reviewed Mar 31, 2025

View reviewed changes

crazytonyli added 4 commits April 1, 2025 14:01

Return the correct primary blog id

a185c04

Merge branch 'trunk' into fix-wp-account-nullablity

c82bd3b

Fix using the primaryBlogID variable

1193afc

Make the function that gets WPAccount.authToken throw

8af7561

kean approved these changes Apr 1, 2025

View reviewed changes

Merge branch 'trunk' into fix-wp-account-nullablity

ddc2105

kean enabled auto-merge April 1, 2025 19:51

kean added this pull request to the merge queue Apr 1, 2025

Merged via the queue into trunk with commit c9b4af3 Apr 1, 2025
25 checks passed

kean deleted the fix-wp-account-nullablity branch April 1, 2025 20:52

Add nullability annotations to WPAccount #24328

Add nullability annotations to WPAccount #24328

Uh oh!

Conversation

crazytonyli commented Mar 26, 2025

Regression Notes

Uh oh!

dangermattic commented Mar 26, 2025

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 Mar 26, 2025

Uh oh!

wpmobilebot commented Mar 26, 2025

Uh oh!

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

Uh oh!

Uh oh!

wpmobilebot commented Mar 30, 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!

kean commented Apr 1, 2025

Uh oh!

crazytonyli commented Apr 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wpmobilebot commented Mar 30, 2025 •

edited

Loading

wpmobilebot commented Mar 30, 2025 •

edited

Loading