plokko / firebase-php

Php integration of Firebase API (FCM Http v1, RealTime database)
16 stars 9 forks source link

added webpush and apns-push configurations #4

Closed andrewalf closed 5 years ago

andrewalf commented 5 years ago

Working with this library I noticed that webpush configuration is not finished and can't be used. The same with apns-pushes. Tried to fix this, checked web notification modifications by setting an icon and it works.

Doc: https://firebase.google.com/docs/reference/fcm/rest/v1/projects.messages

plokko commented 5 years ago

Thanks, webpush and apns were added just as stubs for lack of time, i'll look into your code! Thanks again for your contribution! 👍

plokko commented 5 years ago

I looked at your code, thanks for your submission and good work, very similar to my original code (glad you understood my work! )! Only one question: what is ApnsAlertConfig? i can't find it in the docs!

andrewalf commented 5 years ago

Hi! "alert" is one of the properties of apns notification. It can be either string or dictionary. This class represents the dictionary option. You can see it here: https://developer.apple.com/library/archive/documentation/NetworkingInternet/Conceptual/RemoteNotificationsPG/PayloadKeyReference.html

andrewalf commented 5 years ago

Thanks for the quick response! I'm using this library in my work, and I really miss web and apns pushes :) Generally, you wrote a good library, very close to fcm documentation, and saved lots of my time)

andrewalf commented 5 years ago

Sorry forgot phpdoc for the class. I'll add it in a minute

plokko commented 5 years ago

Shouldn't it be handled by ApnsConfig? For what i've understood all this fields are handled by ApnsConfig headers so i'm more inclined to add helper functions like setTitle setLaunchImage that automatically set header values instead of creating a whole new class not present in official Firebase docs. I'm trying to be as close as possible to official documentation (and Json-serialized result) to avoid confusion and because the main goal of the library is to be easly integrated in other wrappers.

EDIT: i found it in docs, it's on ApnsConfig payload not headers

plokko commented 5 years ago

No worry btw, you're my first contributor and a good one! :) I did try to contribute to other firebase packages but all of them lacked something and they were not prone to updates so after a lot of struggle i decided to create my own from scratch, contributors are more than welcome!

andrewalf commented 5 years ago

Honestly, a don't understand what do you mean saying "all these fields are handled by ApnsConfig headers". Headers are another property, not connected with stuff that is set through "alert" property. For example, in webpush, the WebPushNotification class is also the representation of non-fcm entity: https://developer.mozilla.org/en-US/docs/Web/API/Notification

Also maybe you wanted to say that this should be handled by ApnsNotification, not ApnsConfig? And in notification class add helpers like you said, or magic get/set. Smth like:

class ApnsNotification implements JsonSerializable
{
  private $alert;

    function __get($k)
    {
        if ('title' === $k) {
          return $this->alert->title;
        }

        return $this->{$k};
    }

   function __set($k, $v)
    {
       if ('title' === $k) {
          $this->alert->title = $v;
          return;
       }
        $this->{$k}=$v;
    }
}

That's rather convinient, but dunno if it's neseccary. Anyway users will have to see the apple docs, to understand what properties mean. What do you think? By the way, i'll have some free time on weekends and I'll be glad to make this library even better. I could help you with refactoring and unit tests, if you don't mind

plokko commented 5 years ago

@andrewalf no, you were correct, ApnsAlertConfig should be a subclass of ApnsConfig, i simply could not find it in the docs, i do approve your approach! Yes DO add unit testing please! I do need someone to write them because i have no free time to write them!

andrewalf commented 5 years ago

Thanks man! Have a nice day :)

andrewalf commented 5 years ago

@plokko Don't forget about the new tag!)