Skip to content
This repository has been archived by the owner on Sep 4, 2020. It is now read-only.

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

Merged

Conversation

Nkzn
Copy link
Contributor

@Nkzn Nkzn commented Aug 20, 2019

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

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

  • 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:

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

@Nkzn Nkzn changed the title 🐛 🍎 Issue #2788: fixed breaking change of APNs deviceToken format for iOS 13 Issue #2788: fixed breaking change of APNs deviceToken format for iOS 13 Aug 20, 2019
@Nkzn
Copy link
Contributor Author

Nkzn commented Aug 20, 2019

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

@jamespsterling
Copy link

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

@danielpassos
Copy link

Any progress on it?

@coreymcmahon
Copy link

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 Nkzn force-pushed the fix-ios13-devicetoken-breaking-change branch from 8ce5272 to f1f1ce8 Compare September 19, 2019 08:26
@Nkzn
Copy link
Contributor Author

Nkzn commented Sep 19, 2019

Hi, @coreymcmahon !

thunk you for great advice. I rebased the branch.

@Nkzn
Copy link
Contributor Author

Nkzn commented Sep 19, 2019

Anyone knows who is the member should i reply to?

@Nkzn
Copy link
Contributor Author

Nkzn commented Sep 19, 2019

Hi, @purplecabbage ! Could you review this PR?

@coreymcmahon
Copy link

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

@acurrieclark
Copy link

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

@purplecabbage purplecabbage merged commit 84781fd into phonegap:master Sep 19, 2019
@danielpassos
Copy link

Intense man clapping

@Nkzn Nkzn deleted the fix-ios13-devicetoken-breaking-change branch September 19, 2019 22:25
@Nkzn
Copy link
Contributor Author

Nkzn commented Sep 20, 2019

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

@acurrieclark
Copy link

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.

dpeacock pushed a commit to dpeacock/phonegap-plugin-push that referenced this pull request Sep 24, 2019
DavidBriglio added a commit to DavidBriglio/phonegap-plugin-push that referenced this pull request Sep 25, 2019
Merge pull request phonegap#2808 from Nkzn/fix-ios13-devicetoken-breaking-change
@PC-Nitin
Copy link

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
Copy link

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
Copy link

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
Copy link
Contributor Author

Nkzn commented Sep 26, 2019

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
Copy link

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
Copy link
Contributor Author

Nkzn commented Sep 26, 2019

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?

スクリーンショット_2019-09-26_23_36_55

@PC-Nitin
Copy link

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?

スクリーンショット_2019-09-26_23_36_55

@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
Copy link
Contributor Author

Nkzn commented Sep 26, 2019

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

Okay! please try it!

@alex-steinberg
Copy link

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?
スクリーンショット_2019-09-26_23_36_55

@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
Copy link

@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
Copy link

@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
Copy link

lock bot commented Oct 30, 2019

This thread has been automatically locked.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants