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

WIP Separation of Bundle and Library #145

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

smurfy
Copy link

@smurfy smurfy commented Sep 23, 2016

This is my work in progress of separating Library and Bundle.

This PR also includes #141 and #142

The library part is available here: https://github.com/smurfy/RMSPushNotifications
Tested only iOS and FCM Handlers, but if i did no typo it should work.

I marked this Issue WIP because i removed some stuff and changed things and i want to open a discussion about these things.

  1. Removed logger as hard dependency. It filled my log's with errors i already handled with the result of ->send(). I probably will implement this by adding a global config variable allowing the user to set the logger service. logger service because this allows to set a separate channel logger if required. This was the main reason to do this PR in the first place.
  2. I removed the set PEM as string feature. This has multiple reasons, kind of a hack in notifications class, needs cleanup.... I'm happy to implement it again (hopefully better) if i understand the need for this feature.
  3. If i find the time i add unit tests for the Handlers

The PR can be tested by adding this to the composer.json

 "richsage/rms-push-notifications" : "dev-master",
 "richsage/rms-push-notifications-bundle" : "dev-master",

and

"repositories": [
    {
        "type": "vcs",
        "url": "https://github.com/smurfy/RMSPushNotifications"
    },
    {
        "type": "vcs",
        "url": "https://github.com/smurfy/RMSPushNotificationsBundle.git"
    }
 ],

@@ -36,6 +36,8 @@ public function send(MessageInterface $message)
throw new \RuntimeException("OS type {$message->getTargetOS()} not supported");
}

dump($this->handlers, $message->getTargetOS());
Copy link

Choose a reason for hiding this comment

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

what's this doing here?

Copy link

@toriqo toriqo left a comment

Choose a reason for hiding this comment

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

remove the dump from Notifications.php

@smurfy
Copy link
Author

smurfy commented Aug 7, 2017

Not sure where are you looking, but the file does not exist anymore at all (in the bundle part)
Even in the library part that dump does not exist.

https://github.com/smurfy/RMSPushNotificationsBundle
https://github.com/smurfy/RMSPushNotifications

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants