WordPoints / hooks-api

A basic API for hooking into user actions https://github.com/WordPoints/wordpoints/issues/321
GNU General Public License v2.0
0 stars 0 forks source link

Move event firing code out of the router #134

Closed JDGrimes closed 8 years ago

JDGrimes commented 8 years ago

In https://github.com/WordPoints/hooks-api/issues/129#issuecomment-219147197 we brought up the possibility of moving the fire_event() method (and fire_reaction() method, which it calls) out of the router, and possibly into the event app. The reason for this is that these methods are largely separate from the router proper, which is an action router. The only reference to $this within them is indeed to call $this->fire_reaction(). The fire_event() method has been made public as well, and indeed, it is mainly for the sake of providing a more approachable and generic interface to the event firing API that we were considering this change. The fact is that the events API might in the future be triggered by things other than actions, and while that will likely always be a focus, it seems more forward compatible to split it from the router, allowing each to evolve separately.

The only question would be whether we might want to drastically change how event firing was handled at some point, and whether this might involve the router. I'm thinking specifically of #109, which would have changed the way that the reactions are pulled up to fire the events. However, even then the router was just becoming a generic hub for several separate indexes. In truth, the reactor index that was added there was really an entirely separate affair from the action index. An indeed, I even contemplated whether it should be split from the main router because of the fact that it was making it overly complex.

JDGrimes commented 8 years ago

The fact that the $action_type is an arg of the fire_event() method (and the first arg at that), gives me pause. The action type seems to be a concept closely connected to actions, and therefore the action router. And so it is. But I guess that really action types are constructs completely created and controlled by the event API, and so "action" there can be looked at as being more generic than just the WordPress action API.

JDGrimes commented 8 years ago

Perhaps this should really go in the main hook app instead of the events registry app, because it really has no code in common with the events registry either. It doesn't reference the event registry or even use the event slug accept to retrieve the reactions. So since it brings together so many different pieces of the API, maybe it really does belong in the main app.

JDGrimes commented 8 years ago

I just noticed that the router's route_action() method does reference the event registry. So I was thinking maybe about moving that part of the logic to the events registry, along with the other methods above. However, the event action args are constructed with the action object, which obviously connects this to the action API specifically. So I'm thinking we'll just leave that logic in the router where it is.

Alternatively, I guess we could abstract it so that the data to construct the event args with could be passed in to the fire() method (wherever it resides). While this does make some sense, since the hook args do need to be constructed for events to fire, I think that it won't be generic enough. What's to say that we'll want to construct the args with the same data across all of them in every circumstance? We might not. We might not even want to use the arg objects at all in some cases, perhaps. So I think we'll go back to the previous plan, leaving this where it is and moving the other methods to the main hooks app.

JDGrimes commented 8 years ago

I've moved this to the main hooks app, and now I'm wondering whether we should maybe change the order of the args. The $action_type arg has to stay, obviously, but while I've been working on this I've been thinking that maybe the $event_slug arg should be first. That seemed especially better when we were considering moving the method to the events app, but even on the main app putting the action type first feels odd. Another thing to consider is that it is possible that we'll remove action types in the future, in which case it might be best to make the action type parameter last wherever it is used, just in case we want to deprecate it in the future. Of course, that extends to other methods beyond just this one, so maybe we should explore that in another ticket.