[Notifier] [Firebase] Add data field to options #40102
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
| public function __construct(string $to, array $options) | ||
| protected $data; | ||
|
|
||
| public function __construct(string $to, array $options, array $data) |
There was a problem hiding this comment.
I think I would prefer to use a key data inside options array, WDYT?
There was a problem hiding this comment.
Yes, I get what you're saying. The con for this is the toArray method that returns everything that is in options array for key notification and I would have to do some logic either in FirebaseTransport or here in toArray to retrieve what's there under the data key (and remove it from the notification key), if that makes sense
But it is doable indeed. 😄 Do you think it's worth it?
There was a problem hiding this comment.
Maybe it makes sense, but I don't get it 😄
When you add it to the options array and return it in toArray, we can use it there and if the (I hope I understood this right) transport is instantiated without FirebaseOptions we add the 'data' key to the payload as an empty array.
The main point is: Whenever I send a payload it needs to include a data key, right?
There was a problem hiding this comment.
the options in the constructor does not actually represent the full options. It represents only the notication key in the firebase options (see toArray. To me, that's a naming issue in the FirebaseOptions class.
There was a problem hiding this comment.
I get what you are saying @OskarStark , but from my point of view, that options field is indeed a naming issue there and I suggest, for the sake of clarity in mapping the firebase api, to change it to something like notificationOptions/notification and add a separate data property.
There was a problem hiding this comment.
@stof do you think I should rename that field? I would really need this PR to be merged soon because I am a bit blocked on my implementation for push notifications on the project I'm currently working on.
Thanks 👍
(and thank you for the approval @OskarStark :D )
src/Symfony/Component/Notifier/Bridge/Firebase/FirebaseOptions.php
Outdated
Show resolved
Hide resolved
19f4620 to
fa8064b
Compare
|
Thank you @Raresmldvn. |
The Firebase Notifier must comply to the specifications at https://firebase.google.com/docs/cloud-messaging/xmpp-server-ref.html#notification-payload-support .
The options are missing the
datafield which is a common field for all types of notifications: web, ios and android.