WPAccount needs nullability info
Because WPAccount is written in Objective-C and has no nullability information there are situations in which accessing its properties can crash in swift. To compound this problem, sometimes they can occur at launch within signinForWPComFixingAuthToken. This leads to a situation in which the user must delete the app and reinstall.
Luckily, this specific situation arose from my working in signup code, and I think is very unlikely to happen in production (at least, not for the same reasons).
My preferred solution would be to rewrite the class in swift, but at the least we should accurately tag the properties with nullability attributes.
I tried adding nullability annotations, which results in 25 build errors, which it doesn't look that bad. However, there are some tough issues:
- MyProfileViewController relies on the account knowing the userID, which is not guaranteed. We have a related crash, but bailing on
nilmeans the users would tap on My Profile and nothing would happen. - AztecVerificationPromptHelper relies on emailVerified. We could default to true, but this means bringing back the issue where a post wouldn't actually publish for a unverified account
The deeper problem is that we often rely on data to be present when there's no guarantees of it, and it will work 99% of the time. Also Core Data doesn't help, with its models with all-optional properties.
This issue has been marked as stale because:
- It has been inactive for the past year.
- It isn't in a project or a milestone.
- It hasn’t been labeled
[Pri] Blocker,[Pri] High, orgood first issue.
Please comment with an update if you believe this issue is still valid or if it can be closed. This issue will also be reviewed for validity and priority (cc @designsimply).
I looked through the code for WPAccount and found https://github.com/wordpress-mobile/WordPress-iOS/blob/4ac54edd8c4db2978a0122f97e137c755e627d4a/WordPress/Classes/Services/AccountService.h and the latest change was https://github.com/wordpress-mobile/WordPress-iOS/pull/9042.
@nheagy does this mean the class still needs to be rewritten in swift?
@designsimply This appears to still be valid. We don't need to rewrite this class in Swift, but we should either do that or add nullability annotations. I'm not sure how work like that will be prioritized/assigned. It seems like tech debt may be a special case?
This issue has been marked as stale because:
- It has been inactive for the past year.
- It isn't in a project or a milestone.
- It hasn’t been labeled
[Pri] Blocker,[Pri] High, orgood first issue.
Please comment with an update if you believe this issue is still valid or if it can be closed. This issue will also be reviewed for validity and priority (cc @designsimply).
| Fails | |
|---|---|
| :no_entry_sign: | Please add a feature label to this issue. e.g. 'Stats' |
Generated by :no_entry_sign: dangerJS
I believe this issue is still valid.
This issue has been marked as stale because:
- It has been inactive for the past year.
- It isn't in a project or a milestone.
- It hasn’t been labeled
[Pri] Blocker,[Pri] High, orgood first issue.
Please comment with an update if you believe this issue is still valid or if it can be closed. This issue will also be reviewed for validity and priority during regularly scheduled triage sessions.
Thanks for reporting! 👍