-
Notifications
You must be signed in to change notification settings - Fork 4k
fix(messaging,nnbd): regression in RemoteMessage.fromMap()
causing silent failure
#5336
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
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
I suspect Issue #5324 of being an occurrence of this as well |
@spacejunky Hello, my background and foreground message handlers, both were not working on Flutter 2.0.1 and Firebase Messaging 9.0.0 with null safety on. Using this fix, it is working now. Thank you. |
packages/firebase_messaging/firebase_messaging/example/lib/message.dart
Outdated
Show resolved
Hide resolved
packages/firebase_messaging/firebase_messaging/example/lib/message.dart
Outdated
Show resolved
Hide resolved
packages/firebase_messaging/firebase_messaging/example/lib/tracking.md
Outdated
Show resolved
Hide resolved
Thanks! Could you add tests for this? |
Done
Analyzer warnings are all in the "rough" coversion of the example app to null-safety, which is not intended (and not suitable, for many reasons) for merging. Cheers, |
If this is not intended for merging, could you revert the changes? As they cause the CI to fail. |
packages/firebase_messaging/firebase_messaging/example/test/remote_message_test.dart
Outdated
Show resolved
Hide resolved
Moved as requested, noting that I am unable to verify correctness as (due to the multiple melos problems I have encounted) nothing in that part of the tree has will even compile on my system as the package paths are broken. PS: Is melos supposed to work on Windows systems? |
packages/firebase_messaging/firebase_messaging_platform_interface/test/remote_message_test.dart
Outdated
Show resolved
Hide resolved
…ace/test/remote_message_test.dart
packages/firebase_messaging/firebase_messaging_platform_interface/test/remote_message_test.dart
Outdated
Show resolved
Hide resolved
…ace/test/remote_message_test.dart
RemoteMessage.fromMap()
causing silent failure
@Salakar hello, is it built to fix the IOS silent issue? haven't seen version updated for this PR. |
Description
Proposed correction of Issue #4949, which causes silent discard of incoming FCM's when running under sound null safety.
This not a breaking change but a correction of a previous regression.
Due to multiple problems getting Melos to run on my windows system, despite attempting to follow the Contributor Guide, I have used the firebase_messaging example app to provide a demo of the faulty behavior and its subsequent correction. This is neither unit or integration testing. Sorry!
The changes made to the example app so that it runs under sound null safety (c1fda5b) -- which is when the faulty behavior shows up -- are not the result of any deep thought and are not proposed for merging. The analysis & formatting fails are all to be found in this commit, I believe.
Likewise not tracking.md (f5cd8e5), which just provides commentary for your convenience.
Only the two lines in remote_message.dart, commit 48bc692, are relevant to the issue. I'm sorry that I don't know how to formulate the PR so that it says only that, while still giving you the visibility of the supporting work.
Related Issues
fix #4949
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
).This will ensure a smooth and quick review process. Updating the
pubspec.yaml
and changelogs is not required.///
).flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?