WordPress-iOS icon indicating copy to clipboard operation
WordPress-iOS copied to clipboard

WPAccount needs nullability info

Open nheagy opened this issue 7 years ago • 9 comments

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.

screen shot 2018-02-26 at 9 38 15 am

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.

nheagy avatar Feb 26 '18 15:02 nheagy

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 nil means 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.

koke avatar Mar 07 '18 10:03 koke

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, or good 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).

stale[bot] avatar Mar 07 '19 11:03 stale[bot]

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 avatar Mar 12 '19 20:03 designsimply

@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?

nheagy avatar Mar 12 '19 21:03 nheagy

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, or good 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).

stale[bot] avatar Mar 11 '20 21:03 stale[bot]

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.

designsimply avatar Sep 28 '20 19:09 designsimply

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, or good 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.

stale[bot] avatar May 01 '22 15:05 stale[bot]

Thanks for reporting! 👍

dangermattic avatar Sep 12 '24 18:09 dangermattic