WildPHP / irc-bot

A simple and modular PHP IRC bot
MIT License
84 stars 24 forks source link

Modules! #21

Closed Amunak closed 9 years ago

Amunak commented 9 years ago

The idea is to have a clean base that can connect to an IRC server and provides a robust system for managing what else it does. The goal is to have a system of modules where each can do what commands and listeners do now, except that you can bundle more commands, listeners and other functionality into a single module.

The bot core would provide:

Am I missing something? Everything else would be provided by the modules. All current commands and listeners would move there, along with planned stuff like channel manager, user and group manager, etc.

NanoSector commented 9 years ago

I think, albeit not completely perfect, that the new module manager takes this point on. It provides dependency resolving, module loading and unloading (though not reloading due to PHP limitations) and ties in with the event manager.

So unless you have more ideas for it, I think it's ready for action.

Amunak commented 9 years ago

I just noticed one huge mistake: the modules are supposed to have a common predecessor (similar to how we had CommandBase and ListenerBase) that would define some of the basic, important methods and dictate the "style" of all modules).

Other than that I copied over some stuff from the "hooks system" issue and made it all in a checkbox-list so that it'S easier to keep track of. Feel free to tick anything off the list, I may have not noticed all the changes. From what's left I'm not even sure if we need to

pass data through the event, cancelling the event, editing the data, etc. which means that priority may have little use, too.

About the optional event listener registration - if that's not an option yet we could probably just provide a method that checks if an event exists and then whoever writes the module can use it to make a hook optional.

Oh, and I don't like how we mix hook and event and stuff, we could use some better, clearer naming.

But it all looks just awesome, great work.

NanoSector commented 9 years ago

About the terminology, an Event is something that can be triggered, and a Hook is something that ties in to an Event and gets called upon trigger of the Event.

I agree the method names and stuff could be more clear.

Thanks for providing a checklist, that helps :)

Amunak commented 9 years ago

Oh yeah, what I thought was that we should probably use only the word event, and then you have stuff like:

NanoSector commented 9 years ago

I'll rename the methods, that sounds much better and more logical :)

On Wed, 4 Feb 2015 11:15 Amunak notifications@github.com wrote:

Oh yeah, what I thought was that we should probably use only the word event, and then you have stuff like:

  • registerEvent (registers a new event with the bot)
  • registerEventListener (hooks you into the event basically; I just think this is much more descriptive as you can't mistake an event listener for anything else)
  • triggerEvent
  • removeEventListener
  • removeEvent

— Reply to this email directly or view it on GitHub https://github.com/WildPHP/Wild-IRC-Bot/issues/21#issuecomment-72830531.

Amunak commented 9 years ago

Thanks.

Oh and about the cancellable events and stuff, do we have those? I was thinking you could have a command, say, !quit. Its event listener that actually does the quit thing would be last in the call chain, and any immediate module could cancel it. So someone types !quit, it first goes through the auth module, (the module that has !quit told the auth module earlier that this command is of the BOT_OWNERS group and the auth module knows who should have that privilege) the auth module then decides if theuser who sent it should have the right to do it, and if not, it cancels the event (by setting a cancelled flag on it to true or something). Then when it arrives at the module that implements !quit, the module sees that it was cancelled, and does nothing.

If we manage to make it work like that it has some awesome implications:

NanoSector commented 9 years ago

We should probably cancel the event in the event manager itself, so modules don't have to take care of this themselves.

As for loading order, I'm not sure how to handle that. If you do a priority number type of thing you might face the problem of having multiple listeners register on the same priority.

Amunak commented 9 years ago

We should probably cancel the event in the event manager itself, so modules don't have to take care of this themselves.

While there probably should be an option where you would choose if your listener wants to listen for all or only non-cancelled events, a cancelled event is not necessarily useless. You may still want to pass it down the event chain if you wanted to, say, log the cancelled event.

I'm also not even sure if we should make the cancellation a specific thing - you should be able to pass any data through the event chain and allow all plugins to access them - while in the example a cancellation is the only thing you really need, in some scenarios you would want something more specific and stuff.

I believe the real challenge here is mainly to decide on how all the parts should communicate. It should be simple and intuitive but versatile.

As for loading order, I'm not sure how to handle that. If you do a priority number type of thing you might face the problem of having multiple listeners register on the same priority.

You do, but it can still work. Make priorities highest, high, medium, low, lowest, log (higher priorities run first). This should be sufficient in most cases. In the example I provided you would probably want auth manager to listen on high and the module that executes the comand on lowest. Log would be a special level that would allow only reading stuff from the event, not changing it (basically for logging purposes and such).

NanoSector commented 9 years ago

Listeners with the highest priority (rated 'critical') set can now cancel events simply by returning false, and priorities are implemented.

Anything lower than the highest priority cannot cancel events because they're not 'critical'. If a module wants to be able to cancel an event they'll need to make their listeners critical.

If the module does not specify a priority level, the listener is dumped into the 'medium' category.

Does this all sound good to you?

Amunak commented 9 years ago

I believe this is now all implemented. We just need to fix the modules themselves, but that's another story.

NanoSector commented 9 years ago

Everything but the log manager is implemented properly, and the modules themselves are ported.

NanoSector commented 9 years ago

Done!