Skip to content

fix(firebase_messaging): ensure plugin callback registered #1774

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

Merged
merged 5 commits into from
May 12, 2020

Conversation

Eddydpyl
Copy link

@Eddydpyl Eddydpyl commented Jan 7, 2020

The fact that the isolate is already running should not prevent the callback from being registered.

Description

The onBackgroundMessage callback stops triggering after app restart (see related issues). This seems to be caused by the pluginRegistrantCallback not being registered every time the FcmDartService#start method is called, due to the fact that the isolate is already running. Removing the check for the isolate not running solves the problem, and I'm yet to observe any side effects. That being said, I cannot guarantee this is the best way to handle the issue.

Related Issues

Solves the issue descrived in #1709 and might also solve #1590.

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • If the pull request affects only one plugin, the PR title starts with the name of the plugin in brackets (e.g. [cloud_firestore])
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

The fact that the isolate is already running should not prevent the callback from being registered.
@hb0nes
Copy link

hb0nes commented Jan 11, 2020

Very nice, merge this ASAP.
We need (and use) this change.

@Finn0811
Copy link

I'd also like to see it merged 👍

@brokenalarms
Copy link

bump for this please, just spent the past few hours trying to get this working, background messages was the first thing I tried to get working after installing this plugin and it doesn't work without this change.

Eddydpyl added 2 commits April 6, 2020 08:45
Update changes from FirebaseExtended/flutterfire
@Eddydpyl
Copy link
Author

Eddydpyl commented Apr 6, 2020

@brokenalarms I've updated CHANGELOG.md and it looks like that was necessary for the code owners to be notified, my bad. In the meantime, you could use my fork (I've updated it just now): https://github.com/Eddydpyl/flutterfire

@brokenalarms
Copy link

brokenalarms commented Apr 6, 2020 via email

@batuyazilim
Copy link

so this is the solition ? i have same problem
how can i solve this problem?

@windows7lake
Copy link
Contributor

why not merge?

@loolooii
Copy link

loolooii commented May 11, 2020

Why is this PR not being reviewed? After hours of trying to solve this, realized that this is a known issue and it's blocking.

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@Salakar
Copy link
Member

Salakar commented May 12, 2020

@googlebot I consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@Salakar
Copy link
Member

Salakar commented May 12, 2020

Thanks for sending this PR. The change makes sense to me and as far as I can tell this is the correct fix.

I've gone ahead and published firebase_messaging version 6.0.15. Will now merge (ignoring failing publishable CI step - have a separate PR in progress to fix this for all plugins).

@Salakar Salakar changed the title [firebase_messaging] Register pluginRegistrantCallback every FcmDartService#start fix(firebase_messaging): ensure plugin callback registered May 12, 2020
@Salakar Salakar merged commit 4511f7c into firebase:master May 12, 2020
@Salakar Salakar mentioned this pull request May 12, 2020
13 tasks
@Fbrusca
Copy link

Fbrusca commented May 19, 2020

@Eddydpyl In my case "onBackgroundMessage" still works inconsistently. Sometimes it works correctly and sometimes it returns "Service took too long to process intent: com.google.android.c2dm.intent.RECEIVE App may get closed."

Could someone be kind enough to pass me the plugin registry settings for embedding v2?

Thanks


class Application: FlutterApplication(), PluginRegistrantCallback {
    override fun onCreate() {
        super.onCreate()
        FlutterFirebaseMessagingService.setPluginRegistrant(this);
    }

    override fun registerWith(registry: PluginRegistry) {
        CustomPluginRegistrant.registerWith(registry)
    }
}

class CustomPluginRegistrant {
    companion object {
        fun registerWith(registry: PluginRegistry) {
            if (alreadyRegisteredWith(registry)) {
                return
            }
            FirebaseMessagingPlugin.registerWith(registry.registrarFor("io.flutter.plugins.firebasemessaging.FirebaseMessagingPlugin"))
            FlutterLocalNotificationsPlugin.registerWith(registry.registrarFor("com.dexterous.flutterlocalnotifications.FlutterLocalNotificationsPlugin"));
        }

        fun alreadyRegisteredWith(registry: PluginRegistry): Boolean {
            val key: String = FirebaseCloudMessagingPluginRegistrant::class.java.getCanonicalName()
            if (registry.hasPlugin(key)) {
                return true
            }
            registry.registrarFor(key)
            return false
        }
    }
}

@Eddydpyl
Copy link
Author

@Fbrusca I'm afraid I don't know how to help you with this. You should probably open a new issue.

@firebase firebase locked and limited conversation to collaborators Aug 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.