Ph3nol / NotificationPusher

Standalone PHP library for easy devices notifications push.
MIT License
1.18k stars 273 forks source link

GCM Adapter bug #157

Closed domenico-renna closed 6 years ago

domenico-renna commented 7 years ago

With the new 2.3.2 library version you change GCMAdapter line 148 from $serviceMessage->setData($data); to $serviceMessage->setNotification($data);

but this is not generally correct because Data Message an Notification Message are quite different in android application (Google official documentation: https://firebase.google.com/docs/cloud-messaging/concept-options)

Actually we rollbak to 2.3.1 release to fix this problem. Can you restore old code or otherwise offer a setter methods to support both android notification mode?

seyfer commented 7 years ago

Yes, both need to be provided. @albovieira please, check this too. It happened after #154

albovieira commented 7 years ago

@seyfer what do you suggest? Maybe add to Sly\NotificationPusher\Model\Message class a property that will define the notification type with a default value?

seyfer commented 7 years ago

@albovieira, no, I will just return back an old setter. So, there would be two setters for the same $data.

seyfer commented 7 years ago

@domenico-renna , the setter for Data returned back and the v2.3.2 tag had been updated. https://github.com/Ph3nol/NotificationPusher/commit/a86b2be3aa2336ad03be5a5a7adb270144c7f003

domenico-renna commented 7 years ago

Thanks for your quick fix. I'll like to test 2.3.3 too but I can see that you are you using together

$serviceMessage->setData($data);
$serviceMessage->setNotification($data);

I don't know if your choice can be a problem but Google set a 4KB limit for DATA NOTIFICATION payload (you can find the specification following my origina link) and should be possible that using data+notification in json the limit will be reached before

VonSerpe commented 7 years ago

Using together setter is wrong. There must be an option that gives you the ability to set notification, data payloads, or both.

Google docs describes the difference:

App behavior when receiving messages that include both notification and data payloads depends on whether the app is in the background or the foreground—essentially, whether or not it is active at the time of receipt.

  • When in the background, apps receive the notification payload in the notification tray, and only handle the data payload when the user taps on the notification.
  • When in the foreground, your app receives a message object with both payloads available.
seyfer commented 7 years ago

@domenico-renna, @VonSerpe, yes, it was a quick fix. Definitely, it needs to be separated. When I will have time, I will add the setter. Feel free to do a PR if you will have time earlier.

artperfekt commented 6 years ago

Thx for the great job i want to ask about these setters will You have time for add ?

seyfer commented 6 years ago

Please, somebody test this branch by sending to real devices. https://github.com/Ph3nol/NotificationPusher/pull/162

check documentation changes for an explanation. if it works - I will merge.

seyfer commented 6 years ago

Have added setter via an optional class. See docs. Please, somebody test the branch 157-fix