Issue #2788: fixed breaking change of APNs deviceToken format for iOS 13

Nkzn commented 5 years ago

Description

Fix parsing method for registered device token. On iOS 12 or lower, it will work as before. On iOS 13 or higher, new hexadecimalStringFromData method makes binary to hexadecimal string.

The following article was helpful:

Related Issue

2788 iOS token weird format

Motivation and Context

Until iOS 12, the result of [deviceToken description] was like "<124686a5 556a72ca d808f572 00c323b9 3eff9285 92445590 3225757d b83967be>" , so we only had to remove the symbols and spaces.

However, on iOS 13, the result of [deviceToken description] becomes "{length = 32, bytes = 0xd3d997af 967d1f43 b405374a 13394d2f ... 28f10282 14af515f}" , and deviceToken cannot be extracted by the previous conversion. The broken token is provided as the data for on('registration', callback) .

With this fix, both formats can be handled.

How Has This Been Tested?

iPad 6Gen iOS@13.0.0 iPhone 8 iOS@12.4 Cordova@8.1.2 Cordova ios@5.0.0 Xcode@11.0 beta (11M336w)

On iOS13, following log shows hexadecimal string like 124686a5556a72cad808f57200c323b93eff9285924455903225757db83967be.

push.on('registration', data => {
  console.log(data.registrationId);
});

And on iOS 12 works same.

Screenshots (if appropriate):

N/A

Types of changes

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[x] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[x] I have read the CONTRIBUTING document.
[ ] I have added tests to cover my changes.
[x] All new and existing tests passed.

Nkzn commented 5 years ago

Oh...CI fails with JS error. It's not my fault (I touched only objc)

jamespsterling commented 5 years ago

Having this issue as well, is this a working solution?

danielpassos commented 5 years ago

Any progress on it?

coreymcmahon commented 5 years ago

Hi @Nkzn!

Thanks for fixing this issue. It seems the build is failing because your .travis.yml file references an old Node JS version.

Here's your .travis.yml file:

https://github.com/Nkzn/phonegap-plugin-push/blob/fix-ios13-devicetoken-breaking-change/.travis.yml

language: node_js
node_js:
- '4'
branches:
  only:
  - master
notifications:
  slack:
    secure: QI+nroFHoaX5vBTtMiJwBt9pYBvFKC8TPwyREEY0yZNjp4+bF/rk7Sj7nNK136m4+nP+wPrAPSC+8jk7jdjRWP2j+CRbnGCSf/29xeDWgXpRUoOGTe8/XWhHlLKwwJ6zm+eB6kwNN3wqrQ+C/9L7gckbj6BCvpp9SwH1q02lpNU=
  email:
  - PhoneGapCI@adobe.com

... and if you check on master, phonegap-plugin-push has the following config:

https://github.com/phonegap/phonegap-plugin-push/blob/master/.travis.yml

language: node_js
node_js: 8
branches:
  only:
  - master
notifications:
  slack:
    secure: QI+nroFHoaX5vBTtMiJwBt9pYBvFKC8TPwyREEY0yZNjp4+bF/rk7Sj7nNK136m4+nP+wPrAPSC+8jk7jdjRWP2j+CRbnGCSf/29xeDWgXpRUoOGTe8/XWhHlLKwwJ6zm+eB6kwNN3wqrQ+C/9L7gckbj6BCvpp9SwH1q02lpNU=
  email:
  - PhoneGapCI@adobe.com

It seems your master branch is 6 commits behind origin. If you merge back from master on phonegap/phonegap-plugin-push it should bring in the change to the Travis config and get your build passing again.

Ping me if you need help with this or don't have time and I'll open a separate PR that incorporates your fix. We're using this in production, so we're quite anxious to get this fixed before iOS 13 is released.

Nkzn commented 5 years ago

Hi, @coreymcmahon !

thunk you for great advice. I rebased the branch.

Nkzn commented 5 years ago

Anyone knows who is the member should i reply to?

Nkzn commented 5 years ago

Hi, @purplecabbage ! Could you review this PR?

coreymcmahon commented 5 years ago

I believe @purplecabbage merged the last few PRs, perhaps they can take a look?

acurrieclark commented 5 years ago

Having this merged soon would be tremendously good timing! A new release would also be handy.

danielpassos commented 5 years ago

Intense man clapping

Nkzn commented 5 years ago

Released with v2.3.0 スクリーンショット 2019-09-20 13 49 19

acurrieclark commented 5 years ago

Many thanks @Nkzn, @purplecabbage and @coreymcmahon for getting this change through. I am sure there will be many devs waking up today to find this an important change.

PC-Nitin commented 5 years ago

I am facing the same issue even after upgrading the plugin. Notifications are working fine with iOS 12 devices but they are not working on iOS 13 devices. Can somebody please guide me?

kamiljackiewicz commented 5 years ago

We have an issue that none of iphone devices works with latest version. All iphone devices we tested are not being registered for push notifications. No issues with android. What can be the reason?

PC-Nitin commented 5 years ago

We have an issue that none of iphone devices works with latest version. All iphone devices we tested are not being registered for push notifications. No issues with android. What can be the reason?

I am also facing issue with device registration.

Nkzn commented 5 years ago

hmm... 🤔

push.on('registration', (data) => {
  console.log(data.registrationId);
});

what is your result above? @PC-Nitin @kamiljackiewicz

if it start with "{length = 32, bytes =..., you haven't use this fix. check library version. it should be 2.3.0.

Or there are several other possible causes.

#1.x branch does not includes this fix (it should be backported)
it is not registration issue. the iOS 13 also has apns-push-type issue. This causes the problem that push notifications are not sent (or delay).

PC-Nitin commented 5 years ago

registrationId

Hello,

I can see the version as 2.3.0 in conig.xml and after running the app I can see the format of registrationid as "{length = 32, bytes =..., in device logs.

Nkzn commented 5 years ago

hi, @PC-Nitin . thank you for confirm.

it is strange state...

if you opened your project with xcode, PushPlugin.m can be show as below?

PC-Nitin commented 5 years ago

hi, @PC-Nitin . thank you for confirm.

it is strange state...

if you opened your project with xcode, PushPlugin.m can be show as below?

@Nkzn I can't see this code in PushPlugin.m file. May be I will try to delete the plugin folder, remove node modules and do npm install again.

Nkzn commented 5 years ago

I will try to delete the plugin folder, remove node modules and do npm install again

Okay! please try it!

alex-steinberg commented 5 years ago

hi, @PC-Nitin . thank you for confirm. it is strange state... if you opened your project with xcode, PushPlugin.m can be show as below?

@Nkzn I can't see this code in PushPlugin.m file. May be I will try to delete the plugin folder, remove node modules and do npm install again.

I am running v2.3.0, can see the changed code above but am still not getting just the token on push registration.

PC-Nitin commented 5 years ago

@alex-steinberg and @Nkzn Yes the issue is resolved. I can see the changes in PushPlugin.m file now. I removed and added plugins and platform and it worked after that. Thank you for your help.

alex-steinberg commented 5 years ago

@PC-Nitin I see it is resolved, thank you. I was misled by the NSLog which fires before the OS discrimination (line 363 of PushPlugin.m).

lock[bot] commented 5 years ago

This thread has been automatically locked.

phonegap / phonegap-plugin-push