laravel-notification-channels / fcm

Firebase Cloud Messaging (FCM) notifications channel for Laravel
https://laravel-notification-channels.com/
MIT License
491 stars 126 forks source link

Sending through anything other than a string causes push notification to not send. #77

Closed ktoim closed 2 years ago

ktoim commented 3 years ago

Expected Behavior

When sending through an integer the push notification should be received.

Current Behavior

Passing anything through other than a string causes the push notification to not be received.

Possible Solution

Convert all instances in the array to be of type string.

Steps to Reproduce

Add the following to a notification:

return FcmMessage::create()
            ->setData([
                'foo' => 'bar',
                '1' => 2,
            ])
            ->setNotification(
                \NotificationChannels\Fcm\Resources\Notification::create()
                    ->setTitle('title')
                    ->setBody('body')
            );
nvahalik commented 3 years ago

Minimally, this ought to be documented. Ideally, even a wrapper object: FcmData::create()->set('key1', 1)->set('key2', 'value2') would be nice.

martinjoiner commented 3 years ago

May I suggest an alternative solution. And I am offering to do the work and make the PR for this.

I would like it if some simple validation was in the FcmMessage::setData() method. All it would do is check if any of the values are not strings and if it found any it would throw an Exception with a helpful explanation in the message to say that values need to be strings. This would educate the developer and empower them to fix their code. I much prefer this approach over silently casting their data to a string on their behalf.

Had this validation existed it would have saved me 4 hours today as I was started by debugging my code looking at queue output, error logs, tests, none of which were making any noise to tell me I was doing something wrong. It wasn't until I started side-by-side comparison to another project's code that I noticed they were casting things to strings and in my desperation I tried it and only then did I Google it to find the documentation. I expect many others are having the same experience, maybe even giving up earlier.

Maintainers, give me the go-ahead and I'll happily make a PR.

martinjoiner commented 3 years ago

Tagging @chrisbjr as I think it's you who merges PRs yes?

TristanWeij commented 3 years ago

May I suggest an alternative solution. And I am offering to do the work and make the PR for this.

I would like it if some simple validation was in the FcmMessage::setData() method. All it would do is check if any of the values are not strings and if it found any it would throw an Exception with a helpful explanation in the message to say that values need to be strings. This would educate the developer and empower them to fix their code. I much prefer this approach over silently casting their data to a string on their behalf.

Had this validation existed it would have saved me 4 hours today as I was started by debugging my code looking at queue output, error logs, tests, none of which were making any noise to tell me I was doing something wrong. It wasn't until I started side-by-side comparison to another project's code that I noticed they were casting things to strings and in my desperation I tried it and only then did I Google it to find the documentation. I expect many others are having the same experience, maybe even giving up earlier.

Maintainers, give me the go-ahead and I'll happily make a PR.

This would be great! Had the same issue a while ago; It took me a while before i figured out what prevented the message from being sent.

martinjoiner commented 3 years ago

@TristanWeij thanks for adding your own story. If having the simple validation I am proposing on the setData() method would have saved both you and me time then surely hundreds of others must be having their time wasted. I'm still happy to make the PR. Just need the go ahead from those who will merge it! @chrisbjr are you the right person?

martinjoiner commented 3 years ago

Ah, maybe @atymic is the person who can give the go-ahead?