Firebase Cloud Messaging support #141

siwymilek · 2016-06-24T09:33:35Z

No description provided.

richsage · 2016-10-28T10:36:38Z

DependencyInjection/RMSPushNotificationsExtension.php

@@ -95,13 +95,27 @@ protected function setAndroidConfig(array $config)
        $this->container->setParameter("rms_push_notifications.android.c2dm.password", $password);
        $this->container->setParameter("rms_push_notifications.android.c2dm.source", $source);

+        // DEFINE PARAMETERS
+        $this->container->setParameter("rms_push_notifications.android.gcm.api_key", null);


Why are these being set as null and then directly overwritten further down?

richsage · 2016-10-28T10:38:08Z

Message/AndroidMessage.php

@@ -136,7 +150,9 @@ public function setDeviceIdentifier($identifier)
     */
    public function getTargetOS()
    {
-        return ($this->isGCM ? Types::OS_ANDROID_GCM : Types::OS_ANDROID_C2DM);
+        if($this->isGCM) return Types::OS_ANDROID_GCM;


Please reformat this as per PSR-2 if possible - makes it a bit easier to read :-)

richsage · 2016-10-28T10:38:53Z

README.md

@@ -44,6 +44,9 @@ only be available if you provide configuration respectively for them.
              api_key: <string_android_gcm_api_key> # This is titled "Server Key" when creating it
              use_multi_curl: <boolean_android_gcm_use_multi_curl> # default is true
              dry_run: <bool_use_gcm_dry_run>
+          fcm:
+              api_key: <string_android_fcm_api_key> # This is titled "Server Key" when creating it


I'm not that familiar with FCM - is the comment still relevant here about "Server Key" being how the Firebase control panels name the API key?

It's still called "Server Key"

richsage · 2016-10-28T10:40:36Z

Service/Notifications.php

@@ -35,7 +35,6 @@ public function send(MessageInterface $message)
        if (!$this->supports($message->getTargetOS())) {
            throw new \RuntimeException("OS type {$message->getTargetOS()} not supported");
        }
-


Please revert this change

richsage · 2016-10-28T10:42:04Z

README.md


-to send as a GCM message rather than C2DM.
+to send as a FCM message rather than GCM or C2DM.


This is slightly unclear I think, as you should be specifying only one of GCM or FCM - you don't need to specify both. Could you reword this so that it's clear you only need to use one of the set*() methods, depending on your use case?

richsage · 2016-10-28T10:45:32Z

Firstly, thanks for this PR! :-)

Aside from the inline comments above, could you also please add some detail or external reference about the setFCMOptions() method and what it can be used for.

And is the dry_run flag available for FCM at all? I'm not familiar with it, but as it seems to be based closely on GCM...

EmmanuelVella · 2016-12-07T14:32:22Z

@siwymilek Very interested with this PR, thank you ! Do you plan to finalize it ?

EmmanuelVella · 2017-01-18T11:32:30Z

@siwymilek Did you test it for real ? Currently, the data format is :
{"data": {"message": "My message"}}

According to the FCM doc, it should be : {"notification": {"body": "My message"}}

https://firebase.google.com/docs/cloud-messaging/http-server-ref

siwymilek · 2017-01-18T12:36:45Z

@EmmanuelVella As far as I understand the notification parameter is going to show notification immediately, but the data parameter is passing payload to application and developer should handle it by himself.

EmmanuelVella · 2017-01-18T13:00:19Z

That sound right ! Thank you 👍

siwymilek · 2017-02-03T12:13:39Z

@richsage could I please you to review this PR?

richsage · 2017-03-01T10:09:57Z

DependencyInjection/RMSPushNotificationsExtension.php

-            $this->container->setParameter("rms_push_notifications.android.gcm.use_multi_curl", $config["android"]["gcm"]["use_multi_curl"]);
-            $this->container->setParameter('rms_push_notifications.android.gcm.dry_run', $config["android"]["gcm"]["dry_run"]);
-        }
+//        if (isset($config["android"]["gcm"])) {


I'm not sure on the reasoning behind this.

If the user is not interested in GCM configuration (for example, they are only using iOS push notifications), this forces them to supply configuration options. That is why the conditional check is in place, to enable them to leave out the android.gcm key and similar (see the Configuration class which permits the key to be unset/not supplied if not needed).

Commenting this check out will force the user to supply dummy details. Therefore I'd like this to be left in.

This is the same for the FCM change on line 108.

andreiciceu · 2017-06-21T08:43:57Z

@siwymilek interested in your PR, do you plan to finalize it?

gloced · 2017-06-30T08:30:22Z

Service/OS/AndroidFCMNotification.php

+        );
+        $data = array_merge(
+            $message->getFCMOptions(),
+            array("data" => $message->getData())


I checked with the FCM documentation and also tried it out. The Key here must be called "notification" instead of the old "data"

and also in this case of FCM the $message->getData() method should return an array with the 'body' key instead of 'message'.

siwymilek added 4 commits June 24, 2016 11:31

Firebase Cloud Messaging integration

a06bbbf

Fix typo in README.md

1a66f98

Remove dump

519a0ec

Define parameters

9340543

maxtor3569 mentioned this pull request Jun 27, 2016

Support for Firebase Cloud Messaging #140

Open

smurfy mentioned this pull request Sep 23, 2016

WIP Separation of Bundle and Library #145

Open

richsage suggested changes Oct 28, 2016

View reviewed changes

Suggestions from @richsage

6368983

All parameters should be initialized.

320a34f

Check each parameter.

ac376ed

richsage suggested changes Mar 1, 2017

View reviewed changes

richsage mentioned this pull request Mar 1, 2017

Bundle maintained and manage Firebase message #156

Closed

gloced reviewed Jun 30, 2017

View reviewed changes

curiosity26 mentioned this pull request Nov 30, 2017

Add FCM Support #161

Open

alyamovsky mentioned this pull request Apr 8, 2018

Firebase Cloud Messaging support (fixed) #169

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Firebase Cloud Messaging support #141

Firebase Cloud Messaging support #141

siwymilek commented Jun 24, 2016

richsage Oct 28, 2016

richsage Oct 28, 2016

richsage Oct 28, 2016

siwymilek Jan 17, 2017

richsage Oct 28, 2016

richsage Oct 28, 2016

richsage commented Oct 28, 2016

EmmanuelVella commented Dec 7, 2016

EmmanuelVella commented Jan 18, 2017 •

edited

Loading

siwymilek commented Jan 18, 2017 •

edited

Loading

EmmanuelVella commented Jan 18, 2017

siwymilek commented Feb 3, 2017

richsage Mar 1, 2017

andreiciceu commented Jun 21, 2017

gloced Jun 30, 2017

alyamovsky Apr 8, 2018 •

edited

Loading


		to send as a GCM message rather than C2DM.
		to send as a FCM message rather than GCM or C2DM.

Firebase Cloud Messaging support #141

Are you sure you want to change the base?

Firebase Cloud Messaging support #141

Conversation

siwymilek commented Jun 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richsage commented Oct 28, 2016

EmmanuelVella commented Dec 7, 2016

EmmanuelVella commented Jan 18, 2017 • edited Loading

siwymilek commented Jan 18, 2017 • edited Loading

EmmanuelVella commented Jan 18, 2017

siwymilek commented Feb 3, 2017

Choose a reason for hiding this comment

andreiciceu commented Jun 21, 2017

Choose a reason for hiding this comment

alyamovsky Apr 8, 2018 • edited Loading

Choose a reason for hiding this comment

EmmanuelVella commented Jan 18, 2017 •

edited

Loading

siwymilek commented Jan 18, 2017 •

edited

Loading

alyamovsky Apr 8, 2018 •

edited

Loading