-
Notifications
You must be signed in to change notification settings - Fork 280
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
feat(android): support targetSdkVersion >= 31 (Android 12) #185
Conversation
This is needed for compatibility with targetSdkVersion>=31. Without it, push messages cause a crash on Android 12 due to missing FLAG_MUTABLE/FLAG_IMMUTABLE on PendingIntent[0], notification trampoline issue, and possibly other such reasons. Note that Firebase has removed the FirbaseInstanceId class in favor of FirbaseInstallations. To restore it for now, add the firebase-iid dependency, see [2]. [0] https://developer.android.com/about/versions/12/behavior-changes-12#pending-intent-mutability [1] https://developer.android.com/about/versions/12/behavior-changes-12#notification-trampolines [2] https://firebase.google.com/support/release-notes/android#messaging_v22-0-0
Since 1ae3428, when `PushInstanceIDListenerService` was changed away from the deprecated `FirebaseInstanceIdService` to use `FirebaseMessagingService::onNewToken`, there is no longer a need to use a separate service anymore. Further, the `INSTANCE_ID_EVENT` intent filter is not needed.
The callback gets the new token already, there is no need to fetch it again.
These have been deprecated in recent firebase-messaging releases.
Thanks for opening this PR. I had just updated my project and have an Android 12 device.... I spent about 30 minutes trying to figure out why the app crashed the moment a push was received. When I try to build using this PR this breaks:
|
Tasks.await throws ExecutionException so it needs to be unwrapped for the existing error handling to work, otherwise an IOException causes the app to crash.
I am in the process of updating my app to targetSdkVersion 31. This PR builds fine (using |
Android 12 compatibility by bluetech havesource#185
Hello and thank you for this fix. Do you know if and when will this be released in npm? Thank you in advance. |
this PR breaks my build :
I'll look into fixing this, but if someone already has a solution that would be much appreciated. |
@erisu This PR breaks my build as well. |
@viktor-shmigol
|
This is merged in the master branch but not released yet in npm so you might need to add it in your
|
No, I tested that like you suggested, this PR broke the plugin because of the errors specified above. DONT RELEASE THIS! |
The workaround is to switch back to FirebaseInstanceId: https://github.com/viktor-shmigol/cordova-plugin-push/commit/2484e4566914dc40eb976afff6bc8e78fba37064 I know this is deprecated. However, it seems working with cordova-android@11 and Android SDK 32.
|
@Mister-CK, this is working for me. But I am using:
I will make those upgrades and check if I am getting the error that you mentioned |
Can you provide a full list of plugins and versions that is installed on the project? I just tested building against main branch with no issues. My project is basically the default Cordova template with Cordova-Android 11. I have no additional plugins installed.
|
@marioshtika I am also using cordova 10.1.2 and targetSDK 31 |
|
@erisu our package.json: |
@erisu I got it working. The problem was in "FCM_VERSION": "18.+" setting.
|
It seems you still have old plugin configurations.
|
Great 👍 |
@Mister-CK Looking at the |
@Mister-CK correction, looking at the version of the plugin that you have installed, the configurations version of the FCM library is correct. What I am confused on, is how you are getting these errors in your build:
Line 466 of the But in the main branch, those lines do have token and deleteToken. And the errors would appear because the configured FCM version is wrong. Seems your using the newer code but have defined the older plugin version in package.json and have still olde FCM configured. You might want to uninstall and reinstall with this repo's main branch to test. |
I mean, it's Cordova, it's full of old configurations, but most of them I cannot update without breaking have a dozen plugins. I tried specifying "FCM_VERSION": "23.+", but that did not help. Also, I don't feel confident about adding the master branch as the dependency (without version pinning). Even if this PR does work on a clean build. It breaks our build, without accompanying changes. So how do I know that won't happen again. I'll try again in a few days when I have more time, but for now I really need to get push notifications working, so I'll keep using the patch. |
@Mister-CK from your As @erisu mentioned you should, uninstall and reinstall with this repo's main branch to test. Also, try deleting your |
I know, I did that. What I am saying is that that doesn't work for me! of course I tried deleting node_modules, plugins and platforms while rebuilding.
I know, I did that. What I am saying is that that doesn't work for me! of course I tried deleting node_modules, plugins and platforms while rebuilding. |
I send you the package.json of my build using the patch. The errors occur when I delete the plugin and instead install the master branch of the plugin |
@Mister-CK you can try pinning it to that specifc commit.
|
thanks, I didn't know that, that is very useful! |
For now, I published the current main branch (
|
@erisu |
Unfortunately 4.0.0.dev.0 fixed the Android push issue under Android 12 #198, but it broke the iOS build as: |
The contents of this PR has been tested and confirmed. I will mark the PR as resolved so we do not keep talking in one area. If there are any new issues, please create a new ticket with details and steps to reproduce so we can focus on the discussion in its own ticket. |
When using the plugin on Android 12 and/or targetSdkVersion >= 31, any push notification causes the app the crash. The following commits fix these issues.
Description
Please see the commits - I split the PR to multiple commits and describe each change in the commit message.
Related Issue
Resolves #184
Motivation and Context
Fix compatibility with Android 12 / targetSdkVersion >= 31.
How Has This Been Tested?
I tested push notifications (regular & data) on an Android 11 and an Android 12 device.
Types of changes
Checklist: