Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Adapters for modular extending through 3rd parties #22

Open
bobmulder opened this issue Oct 19, 2016 · 21 comments
Open

Add Adapters for modular extending through 3rd parties #22

bobmulder opened this issue Oct 19, 2016 · 21 comments
Assignees
Milestone

Comments

@bobmulder
Copy link
Contributor

Suggestion; implement kind of 'adapters' which are able took hook into notifications. Can be useful to connect to 3rd parties. This idea was brought internally and by #20.

Some ideas of adapters:

  • PusherAdapter
  • SocketIOAdapter
  • GoogleCMAdapter
  • FirebaseAdapter
  • WebhookAdapter
  • SlackAdapter
  • HipchatAdapter

(the idea of adapters can be seen like http://flysystem.thephpleague.com/)

If you want to join the realization of this implementation, feel free to hang into this issue!

Some points of discussion:

  • For sure we need to make use of a base adapter to create new adapters on
  • A cake bake command for adapters would be cute :)
  • Technically, the base adapter should listen to several events
  • What would be a standard Adapter class?
@bobmulder bobmulder added this to the v1.3 milestone Oct 19, 2016
@bobmulder bobmulder self-assigned this Oct 19, 2016
@suhaboncukcu
Copy link

I think https://laravel.com/docs/5.3/notifications is a pretty good point to start. What do you think?

If we are supporting lots of different adapters and again if we are planning to extend it in the future; I think these could be different plugins. So people could use whatever the adapter they want, configure it via configuration file and go. Notifier plugin could take which adapters to use from config and fire the related function from SlackAdapter (or similiar) plugin.

@bobmulder
Copy link
Contributor Author

I think we can use some ideas from Laravel. I like the way how they do have separate classes for each notification.

I don't think every Adapter implementation should be a separate plugin. Why not adding all the implementations in this plugin? Much easier to use :)

@suhaboncukcu
Copy link

@bobmulder
Also much easier to implement I guess but;
If we separate adapters from this plugin; people could create their own adapters for their own cases.

@bobmulder
Copy link
Contributor Author

When we provide some default adapters, they are still able to create own providers right? How easy would it be to just add your credentials of Slack and get started? :)

@suhaboncukcu
Copy link

Why wouldn't we create different plugins and add them as dependency though? :)
Nothing would change for the user.

@bobmulder
Copy link
Contributor Author

Agree, but if nothing is changing for the user, why would we seperate it in multiple plugins? Every implementation of an Adapter will extend to an Adapter instance from this plugin ;)

Via PR's new Adapters can be added or modified, and you'll always be free to create your own adapters in your local app or personal plugin. However, I prefer to have all adapters - that makes sense - in this plugin :)

@suhaboncukcu
Copy link

I sincerely do not share the same vision. Because for me, being able to give other developers to power up their own plugins by using a core plugin is a better way to go. Although PRs are a good way to go, there are tons of developers who won't comply with the standards this plugin uses.

Also, if this plugin reaches more and more people everyday; it could be nice to have some people updating just one of the adapters according to target change. Imagine that there are people always working with Firebase and they know about updates and Firebase specifications. Instead of getting a PR for whole plugin, getting PRs for just Firebase plugin would be a lot more modular I would say. Also, who knows there won't be someone creating a Smooch adapter for example following defined rules.

Yes, they can create their own personal plugins. But providing examples and giving opening adapters to communities -in my opinion- is a better approach. Of course, that's only what I think.

@kaffineaddict
Copy link

I like the way that AdMad handles this for social auth plugin. Provide some base adapters and then have another repository with many new adapters people can download and manually install. I don't see the point in having a seperate module for this as it seems like it should be a core feature to me.

@bobmulder
Copy link
Contributor Author

Can you provide some urls @kaffineaddict?

@suhaboncukcu
Copy link

My example would be this: https://github.com/CakeDC/users/blob/master/composer.json

check the "suggest" part. if you don't need an adapter, just don't require it. :)

@kaffineaddict
Copy link

@kaffineaddict
Copy link

I like a hybrid of those two solutions. AdMad includes some good basic ones so the core interface and the most common templates and then I really like the requiring other templates.

Actually I am really sold on doing it this way.

Everything including cake notifications should technically run through the adapter pattern so include he core interface and a cake db adapter by default and require a repository for the others. Then if people make an adapter we don't care about they can do all the support and we can streamline how they get installed

@bobmulder
Copy link
Contributor Author

Am I right you want to create some 'basic' first adapters we include by default, and support 'community based' adapters? For the 'community based' adapters, I would like to keep them hosted on our organisation itself... Sounds good anyway!

@kaffineaddict
Copy link

I would do this the built in functionality for cake notifications should be converted to an adapter if possible.

A base set of adapters should be created as seperate repositories like bakkerij/notifier-slack this way they can be more modular and support including their own API/Library with it including a ton of bloat in the base notifier

@suhaboncukcu
Copy link

Would that cause an overall change in architecture? @kaffineaddict ?

@kaffineaddict
Copy link

@suhaboncukcu I would imagine so. It would definitely be a new major version I would think.

@bobmulder
Copy link
Contributor Author

I would do this the built in functionality for cake notifications should be converted to an adapter if possible.

Can you explain this to me @kaffineaddict?

@suhaboncukcu mentioned the Laravel example of notifications before... It gave me the following idea to create classes to create notifications (so a notification will be a class). Would that help us working with adapters?

@kaffineaddict
Copy link

Sorry it took so long to respond basically what I would say is this. There should be an adapter class and then an engine that receives an event and processes it. It should not care what the adapter is it should always do the same thing. Lets say those methods are Convert Data->Process Data->Report or something like that. If that is the case then we should add the built in "normal" base or whatever you want to call it functionality as an adapter too.

Right now the plugin does something and saves to the database and then pulls from the database etc. This base/built in functionality should be converted to an adapter with the same Convert->process->report methods.

@bobmulder
Copy link
Contributor Author

bobmulder commented Oct 27, 2016

Sounds good to me! So there would be an DatabaseAdapter as well...

Side note (I don't want to start new discussions :$) but what if we would make this idea PHP-wide and not CakePHP specific? We could make an CakePHP implementation beside that...

Beside your example of convert, process and report, we could say we have a producer and consumer... The producer part should be extended by making notifications though a class (like the Laravel example). Multiple consumers can be attached to the notification. As well we need a 'processor' you noticed @kaffineaddict...

In this case the structure of the plugin would change that much it wouldn't be an 1.3 update, but 2.x (major), agree?

EDIT: Would be worth looking at https://github.com/ziadoz/awesome-php#notifications

@suhaboncukcu
Copy link

@bobmulder although a php package sounds tempting, I think keeping this as a CakePHP plugin has it's own advantages.

a quick list what comes in my mind;

  1. should be much faster to code with model conventions
  2. should be much easier to create new adapters with CakePHP conventions
  3. I think it would be more developer friendly if we also provide and document events, behaviors, etc. to use.

I might be thinking about my own cases though. Shame on me :)

@bobmulder
Copy link
Contributor Author

No problem you think about your own cases @suhaboncukcu. I was thinking; if we can make this PHP-wide, we create a bigger user-base so more people who would help improve the code. That means it would be less work for us and better and more secure code :)

However, I agree with your list. Beside that I could come up with arguments against it like when we're using interfaces we can rely on CakePHP for ourselves, but give the freedom for other frameworks and ORM's.

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

No branches or pull requests

3 participants