-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(ios): support new property to remove note #11248
Conversation
Tests:
|
type: Boolean | ||
default: true | ||
platforms: [iphone, ipad] | ||
since: 8.2.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From an architecture point of view, this is the wrong API design I think. Apple will very likely add more properties to the "sensitive" ones, so probably the user should have a property to override all fields that should be fetched - with the default not containing this one anymore. And it's likely a breaking change, just like this one also is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can be agree with you that we have to change/extend the behavior of exiting APIs specially which are fetching use contacts data. Probably android also have to make some changes to have a parity and there may be some breaking change. For that I think we can not make it before 9.0.0.
But this new property will give developer an option to avoid fetching the 'note' in iOS otherwise they have to make some unnecessary setup. So there is no problem in having this new property. And by default this property is true, so there is no breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without implementing this property, apps are being rejected, so it's a breaking change for them. Either adding this property or defining the properties they actually need, which will be more future proof in the first place. But it's just a community comment 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code change looks ok for now, but this is an odd way to work around the issue. Apps will need to set this to false on iOS 13+ or add the entitlement. Is there any way to detect entitlements and make this false on iOS 13+ if the entitlement is not declared/set? Just seems like if we're going to make a change here we should try and "default" to a working version?
I don't think there is any way to read entitlements at runtime. In this release default value of this property is set to 'true' to avoid breaking change. We will make default value of this property to 'false' in major release, 9.0.0. |
FR Passed |
This still doesn't look right, sorry. And I highly suspect that the apps will still be rejected because Apple checks their blacklisted API's from the compiled app symbols rather than manually. So if the binary still contains |
If you see doc https://developer.apple.com/documentation/bundleresources/entitlements/com_apple_developer_contacts_notes, if one is fetching note without entitlement key, it will give error at runtime. Having CNContactNoteKey is not a problem, it should not be used to fetch note without entitlement. |
https://jira.appcelerator.org/browse/TIMOB-27419