Skip to content
This repository has been archived by the owner on Jun 28, 2024. It is now read-only.

Adding Topic Creation And Subscription And Validating FCM Token #5

Merged
merged 1 commit into from
Oct 5, 2020
Merged

Adding Topic Creation And Subscription And Validating FCM Token #5

merged 1 commit into from
Oct 5, 2020

Conversation

Stevemoretz
Copy link

@Stevemoretz Stevemoretz commented Sep 27, 2020

Essentially I used : brozot#114
And brought it to your repo and made it work removed the errors and all,it works right now without any errors.

However I HAVE NOT TESTED the functionality completely yet.
I have however checked all the methods and it works without any errors.

src/Sender/FCMTopic.php Outdated Show resolved Hide resolved
src/Sender/FCMTopic.php Outdated Show resolved Hide resolved
src/Sender/FCMTopic.php Outdated Show resolved Hide resolved
@Stevemoretz

This comment has been minimized.

@williamdes

This comment has been minimized.

@Stevemoretz

This comment has been minimized.

@williamdes

This comment has been minimized.

@Stevemoretz

This comment has been minimized.

@williamdes

This comment has been minimized.

@Stevemoretz

This comment has been minimized.

@williamdes

This comment has been minimized.

@Stevemoretz

This comment has been minimized.

@williamdes williamdes added the enhancement New feature or request label Sep 28, 2020
@williamdes williamdes added this to the 1.6.0 milestone Sep 28, 2020
@Stevemoretz

This comment has been minimized.

@Stevemoretz
Copy link
Author

I also added some stuff for token validation and the method is tested and it works.

(It's only one method only).

@Stevemoretz Stevemoretz changed the title Adding Topic Creation And Subscription Adding Topic Creation And Subscription And Validating FCM Token Oct 1, 2020
README.md Outdated Show resolved Hide resolved
@williamdes
Copy link
Member

Hi @Stevemoretz
Thank you for the changes, I added one more important comment.
All the other changes are okay

@williamdes williamdes self-assigned this Oct 1, 2020
@Stevemoretz
Copy link
Author

Hi @williamdes ,
Yeah sorry I didn't rewrite that code,I used :

https://github.com/brozot/Laravel-FCM/pull/114

Anyways all fixed I guess.

@Stevemoretz
Copy link
Author

Anyways what's the deal with these phpdocs at all?
I can't get it working,we use a facade so the php docs doesn't help the auto completion at all.
Either some helper php documentation should come in to fix this,or the whole thing should be changed to using directly classes and not a facade,otherwise no autocompletion which is wired.

@Stevemoretz
Copy link
Author

For an example for this fix check FCMValidator class I added a php docs so my method :
FCMValidator::validateToken($validated['token']);
Now simply gets an ide autocompletion because of the phpdoc on the facade.

/**
 * @method static bool validateToken( $token )
 */
class FCMValidator extends Facade{
    protected static function getFacadeAccessor()
    {
        return 'fcm.validator';
    }
}

Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel bad for requesting so much changes, I approve your work
💯

@williamdes
Copy link
Member

For an example for this fix check FCMValidator class I added a php docs so my method :
FCMValidator::validateToken($validated['token']);
Now simply gets an ide autocompletion because of the phpdoc on the facade.

/**
 * @method static bool validateToken( $token )
 */
class FCMValidator extends Facade{
    protected static function getFacadeAccessor()
    {
        return 'fcm.validator';
    }
}

Oh okay, maybe @method static bool validateToken(string $token) is better

@Stevemoretz
Copy link
Author

Sure there you go.

Now I think the next step would be to fix other php docs of the facades.

@williamdes
Copy link
Member

Sure there you go.

Now I think the next step would be to fix other php docs of the facades.

Maybe phpstan is be the tool that we should use to be sure everything is fixed

@Stevemoretz
Copy link
Author

I feel bad for requesting so much changes, I approve your work
💯

No problem man Done all your other requests.

@Stevemoretz
Copy link
Author

In a crazy cool way
barryvdh/laravel-ide-helper
Actually writes the Facade php docs I am extremely fascinated.

@Stevemoretz
Copy link
Author

Have never used phpstan though.

Co-authored-by: Steve Moretz <[email protected]>
Co-authored-by: William Desportes <[email protected]>
@williamdes
Copy link
Member

barryvdh/laravel-ide-helper

Yup, I used IDE helper and it works fine !
This is surely the response to #5 (comment)

Have never used phpstan though.

Give it a try you will love it !

@williamdes
Copy link
Member

I did lint fixes and added the author (@asachanfbd) back onto the commit and set ourselves to co-editors
I am planing to add tests before merging

@Stevemoretz
Copy link
Author

barryvdh/laravel-ide-helper

Yup, I used IDE helper and it works fine !
This is surely the response to #5 (comment)

Have never used phpstan though.

Give it a try you will love it !

Sure it is it magically created those php docs for me.

And Sure I'm right now installing phpstan thank you for introducing it!

@williamdes
Copy link
Member

Could you have a try to see if the façades are okay on the commit f04fdc8 ?

You can git fetch --all -p and git reset --hard f04fdc826456150a2a11303c94b7163b47c7b070 (if you have no unpushed changes)

@Stevemoretz
Copy link
Author

Could you have a try to see if the façades are okay on the commit f04fdc8 ?

You can git fetch --all -p and git reset --hard f04fdc826456150a2a11303c94b7163b47c7b070 (if you have no unpushed changes)

I tried it right now.Not really there isn't any phpdoc on FCMGroup for instance.

@williamdes
Copy link
Member

Could you have a try to see if the façades are okay on the commit f04fdc8 ?
You can git fetch --all -p and git reset --hard f04fdc826456150a2a11303c94b7163b47c7b070 (if you have no unpushed changes)

I tried it right now.Not really there isn't any phpdoc on FCMGroup for instance.

Could you paste here the generated IDE helper part for FCM ?

@Stevemoretz
Copy link
Author

Sure I guess it would be this part?

    namespace LaravelFCM\Facades { 
            /**
     * 
     *
     */ 
        class FCM {
                    /**
         * send a downstream message to.
         * 
         * - a unique device with is registration Token
         * - or to multiples devices with an array of registrationIds
         *
         * @param string|array $to
         * @param \LaravelFCM\Sender\Options|null $options
         * @param \LaravelFCM\Sender\PayloadNotification|null $notification
         * @param \LaravelFCM\Sender\PayloadData|null $data
         * @return \LaravelFCM\Sender\DownstreamResponse|null 
         * @static 
         */ 
        public static function sendTo($to, $options = null, $notification = null, $data = null)
        {
                        /** @var \LaravelFCM\Sender\FCMSender $instance */
                        return $instance->sendTo($to, $options, $notification, $data);
        }
                    /**
         * Send a message to a group of devices identified with them notification key.
         *
         * @param string $notificationKey
         * @param \LaravelFCM\Sender\Options|null $options
         * @param \LaravelFCM\Sender\PayloadNotification|null $notification
         * @param \LaravelFCM\Sender\PayloadData|null $data
         * @return \LaravelFCM\Sender\GroupResponse 
         * @static 
         */ 
        public static function sendToGroup($notificationKey, $options = null, $notification = null, $data = null)
        {
                        /** @var \LaravelFCM\Sender\FCMSender $instance */
                        return $instance->sendToGroup($notificationKey, $options, $notification, $data);
        }
                    /**
         * Send message devices registered at a or more topics.
         *
         * @param \LaravelFCM\Sender\Topics $topics
         * @param \LaravelFCM\Sender\Options|null $options
         * @param \LaravelFCM\Sender\PayloadNotification|null $notification
         * @param \LaravelFCM\Sender\PayloadData|null $data
         * @return \LaravelFCM\Sender\TopicResponse 
         * @static 
         */ 
        public static function sendToTopic($topics, $options = null, $notification = null, $data = null)
        {
                        /** @var \LaravelFCM\Sender\FCMSender $instance */
                        return $instance->sendToTopic($topics, $options, $notification, $data);
        }
         
    }
            /**
     * 
     *
     */ 
        class FCMGroup {
                    /**
         * Create a group.
         *
         * @param string $notificationKeyName
         * @param array $registrationIds
         * @return null|string notification_key
         * @static 
         */ 
        public static function createGroup($notificationKeyName, $registrationIds)
        {
                        /** @var \LaravelFCM\Sender\FCMGroup $instance */
                        return $instance->createGroup($notificationKeyName, $registrationIds);
        }
                    /**
         * add registrationId to a existing group.
         *
         * @param string $notificationKeyName
         * @param string $notificationKey
         * @param array $registrationIds registrationIds to add
         * @return null|string notification_key
         * @static 
         */ 
        public static function addToGroup($notificationKeyName, $notificationKey, $registrationIds)
        {
                        /** @var \LaravelFCM\Sender\FCMGroup $instance */
                        return $instance->addToGroup($notificationKeyName, $notificationKey, $registrationIds);
        }
                    /**
         * remove registrationId to a existing group.
         * 
         * >Note: if you remove all registrationIds the group is automatically deleted
         *
         * @param string $notificationKeyName
         * @param string $notificationKey
         * @param array $registeredIds registrationIds to remove
         * @return null|string notification_key
         * @static 
         */ 
        public static function removeFromGroup($notificationKeyName, $notificationKey, $registeredIds)
        {
                        /** @var \LaravelFCM\Sender\FCMGroup $instance */
                        return $instance->removeFromGroup($notificationKeyName, $notificationKey, $registeredIds);
        }
                    /**
         * 
         *
         * @param \Psr\Http\Message\ResponseInterface $response
         * @return bool 
         * @static 
         */ 
        public static function isValidResponse($response)
        {
                        /** @var \LaravelFCM\Sender\FCMGroup $instance */
                        return $instance->isValidResponse($response);
        }
         
    }
     
}

@williamdes
Copy link
Member

Yup, nothing for this PR ?
Or you would need me to merge so you can use the dev-main version of the composer package ?

@Stevemoretz
Copy link
Author

Stevemoretz commented Oct 2, 2020

Yup, nothing for this PR ?
Or you would need me to merge so you can use the dev-main version of the composer package ?

Well I think these features are fine now, but in other sections are still problems (not related to these though) like #7
And I still haven't tested it all so I think it's better you wait a little I test it then merging it would be a nice idea.

dev-main version of the composer package
I really don't know what this is :(

@williamdes
Copy link
Member

I really don't know what this is :(

Here you are :)
https://packagist.org/packages/code-lts/laravel-fcm#dev-main

williamdes added a commit that referenced this pull request Oct 5, 2020
@williamdes williamdes merged commit 344acd8 into code-lts:main Oct 5, 2020
@williamdes
Copy link
Member

I merged this pull-request so you can test it on dev-main
I will maybe consider adding tests afterards

Anyway, thank you for all :)

@williamdes
Copy link
Member

I added some tests in 1f5ac55

williamdes added a commit that referenced this pull request Oct 8, 2020
@williamdes
Copy link
Member

I just released 1.6.0 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants