Event system

[pnomolos]

Right now we have the hardcoded beforeSave(), afterSave(), etc. methods. I'm wondering what your thoughts are on having a lightweight event system - so in your Entitys constructor you would do something like:

$this->before('save', 'name_of_a_method');
$this->before('delete', function($mapper) { // A callable });
$this->after('loadCollection', array('Static_object', 'static_method'));

[vlucas]

I like that system much better. Great idea. NOW is the time to break BC with these types of changes, since we haven't hit v1.0 yet. I don't like the idea of overriding constructors, because then you also have to know the params and what types, call the parent constructor, etc. I prefer a second method called inside the original constructor like init that you can then provide code for. So basically adding a $this->init() in the bottom of the constructor, and then an empty init method definition so the user can add their own without having to worry about arguments and types, etc. It's much less prone to breakage than overriding constructors.

[vlucas]

Also we probably want to use the new standard on and off methods instead of before and after. If we need before/after is can be something like $this->on('beforeSave', ...) but I don't think we'll need a specific before/after in most cases.

[pnomolos]

Alright, I think I'm going to put this together as I believe it's the last thing I need re-done before I can move my app back over to track master and I'd like to do that sooner rather than later :)

[pnomolos]

My only concern now is how we should register events for static classes from some sort of static interface. For example, if you do:

$mapper->delete('Entity_Post', array('status' => 2));

Then corresponding beforeDelete/afterDelete hooks won't be called because it's on a collection rather than a single entity.

The first thing that jumps to mind would be to have a \Spot\Events class where you can register static event handlers. Something like:

\Spot\Events::on('Entity_Post', 'beforeDelete', $some_callable);

Will that work? Is \Spot\Events the right place to do this, or should it just go on the \Spot class itself because we're keeping this lightweight?

[vlucas]

It's tough to say. The OOP purist in me (admittedly, a pretty small part of me) would say that events are a separate concern, and thus need their own class, but the purpose of this library isn't to be another Doctrine, so I am tempted to say put it somewhere else first. The Mapper seems to be the base Spot thing that you interact with most, so I would say to initially put the events there (and the syntax with the entity name is similar to the other CRUD operations already on the mapper) and then see if they need to be moved out to their own class if it gets too unwieldy.

[pnomolos]

Another thought - do you prefer 'Hook' or 'Event' as the term. I think the proper term should be 'Hook' - Event (to me, at least) implies that you're only responding to something that has happened, and what you do has no effect on the internal processing while Hook feels more like asking for your input.

[vlucas]

I agree. Event does seem to imply async these days and is reactionary, whereas these are more like Filters or Hooks where they are synchronous and can alter the outcome. Rails ActiveRecord calls them callbacks, and Ruby DataMapper calls them hooks, for reference. I think I prefer hooks.

Also I was thinking about the hook points:

• save (insert OR update)
• insert
• update (also multi-update?)
• delete (also multi-delete?)
• validate (will be added when I add in proper validation, most likely)

Anything else?

[pnomolos]

Due to PHP weirdness, a static and non-static method cannot share the same name, but what are your thoughts on using __callStatic so you can do something like this:

$post->on('beforeSave', $callable);
Entity_Post::on('beforeSaveCollection', $callable);

[pnomolos]

As for the event lists - I was also thinking that we could add a beforeWith and afterWith so you could override eager-loading a relation.

[pnomolos]

Alright, this is going to be a breaking change - how should return values work with multiple callbacks? I like the idea that a hook should only operate on the $entity or $collection - perhaps if a hook fires for a single Entity it should ignore the return value completely, and if it fires on a $collection and a $collection is returned it passes that forward. I'm also thinking that we could use something like a \Spot\Exception\Halt to indicate that the current processing should be stopped - similar to how Datamapper does it?

[vlucas]

Hmmm... I thought we were putting the events on the mapper, like:

$mapper->on('Entity_Post', 'save', $callable);

Wouldn't that eliminate the need for the static methods with __callStatic?

I think a simple return false should stop further event propagation and code execution. Functions without return values will return null, so an explicit false is sufficient and intuitive. If an exception is thrown, it would most likely just be handled by PHP itself.

[pnomolos]

Return stuff: sounds good - that keeps it like the existing system.

For the hook stuff: that sounds good as well - I guess if you pass a simple string for $callable it will be assumed that's an instance method on the object?

Note: I remember the issue I had with this. If you need to call $mapper->on() to register a hook it means that you can no longer register them in an Entity::init() method (because there's no access to the mapper yet). Thoughts?

[vlucas]

Ack. Okay - put them wherever you think you need to for now, and we'll sort out the details. Perhaps having them as static methods on the entity is best (it certainly seems that way so far).

[pnomolos]

I actually came up with a method that I believe works - use $mapper to register events but also have a static method hooks() on each Entity so you can register the events there as well. That should cover most use-cases I think :)

[vlucas]

Ah, nice - so the hooks method gets passed a $mapper instance or something?

[pnomolos]

I hadn't thought that much through yet - still writing tests for the basic methods. My initial idea was that it wouldn't even do that - you'd return an array similar to how Entity::relations() or Entity::fields() works.

[pnomolos]

I posted some initial code for this - see 8f95a9c3095aecc89945c3f9869c7a719a6bde27

[vlucas]

That's what I was thinking about at first too, but didn't say it. I guess I should have. The only downside is that you can't register multiple events with the same name that way, although you could just make multiple calls inside a closure as a way around that.

[pnomolos]

That, or you could do this:

return array(
  'beforeSave' => array(function($entity, $mapper){}, function($entity, $mapper){});
);

[vlucas]

True, but how would that differentiate from this:

return array(
  'beforeSave' => array($this, 'uploadFilesToS3');
);

Both are arrays with 2 items. I guess you could check is_callable first and THEN assume it's an array of separate callbacks?

[pnomolos]

Yep, something like that :)

[vlucas]

BTW your work on the hook system looks good so far.

[pnomolos]

I added a few more commits here edb17b6fd6328c39a57401dd212a7cf168b0bdc6

[brandonlamb]

Is there some super light-weight event class that can just be plugged in? I like the idea of being able to

$entity->on('save', $callback1); $entity->on('save', $callback2); or $entity->on(array( 'save' => $callback1, 'save' => $callback2, ));

This way if say in a repository or custom mapper you could hook into those and not just have support for defining a single function for "save"

*edit - sorry i admit i didnt even read the whole thread lol

[brandonlamb]

Also (you touched on this) I would like to see this as a separate concern

https://github.com/brandonlamb/Eve-Framework/blob/master/src/Eve/Events/Event.php (disclaimer, I think I took idea(s) from other frameworks)

Some like that, I dont think I even finished this class but tried keeping it super light. Basically I think the usage was something like

Event::attach('onSave', $callback); Event::trigger($name, $context);

I guess an immediate problem is this would fire 'onSave' for every entity, but I guess my overhaul vote is not having event stuff coded in the entity or existing classes if possible

[pnomolos]

@brandonlamb If you take a look back at the code, I think that I've covered everything you brought up as a concern - super lightweight (just integrated with the Mapper, as that's where all the hooks are fired) and allows you to attach multiple hooks as necessary.

[pnomolos]

While I like the initial idea, I'm not a fan of the shared manager as mappers (and indeed any class using the event/hook manager) aren't necessarily going to be instantiated in or near the same place. As well, they should be light-weight enough that each class can have it's own internal event/hook manager, so something like:

__construct() {
  $this->_hookManager = new \Spot\HookManager();
}

// ...

someFunction() {
  $this->_hookManager->trigger('beforeSomeFunction', $arguments);
}

@vlucas do you have thoughts on how you'd best like this to be handled, seeing as this is ultimately your domain? :smile:

[brandonlamb]

Actually given that example, it fits within the same idea right? I assume that is your mapper's construct, in that case i would not do

$mapper->eventManager($eventManager);

$eventManager is only shared in the case that i actually inject it into both mappers. Otherwise in your example, $mapper1 could instantiate a HookManager object inside itself, and $mapper2 could have it set via injection.

It also allows possibly overriding the hook manager. Isnt this the whole inversion of control thing anyway? From what Ive been reading and retraining myself lately, hard coding $this->resource = new Resource() inside your class is a bad practice and should be moved outside the class?

[brandonlamb]

Ok I finally spent 30 seconds reading how to add code highlighting lol...

class UserMapper
{
    public function __construct()
    {
        $this->hookManager = new \Spot\HookManager();
    }
    public function findById($id)
    {
        $this->hookManager->trigger('findById', $arguments);
        // query stuff
        // return result
    }
}

Then maybe in a controller somewhere

// ...
public function indexAction()
{
    $mapper = new UserMapper();
    $user = $mapper->findById(123);
}

But in my other controller

public function testAction()
{
    $manager = new \Spot\HookManager();
    $manager->on('findById', $callback);

    $mapper = new UserMapper();
    $mapper->hookManager($hookManager);
    $user = $mapper->findById(123);
}

[vlucas]

The primary goal is to keep things simple for the end user. That means both less code and less complexity (to keep it conceptually simple too). I am fine with things being handled by an underlaying Event class or EventManager (similar to the EntityManager), but I want to be careful about revealing whole new concepts to the end user. Namely, the end user never knows about the EntityManager, because they don't have to - they just want to load and save stuff with Entities using the Mapper. Similarly, most times users are just going to want to attach events and hooks the simplest way possible, and that means that we probably shouldn't expose the whole class that manages those under the covers. My aim is to be more like jQuery, and less like Zend Framework and Symfony. So if that means having a class have the actual methods on it (Mapper), but then proxy those to another class to handle (EventManager), then I vote we do that. It's a compromise, because it means a class (the Mapper) ends up having multiple responsibilities (mainly just a Proxy to other classes), but it keeps it super simple for the end user, and doesn't require them to instantiate new classes on their own. To me, the trade-off is worth it for the reduced clutter and complexity for the end-user. Thoughts?

[pnomolos]

First thought: If we were 5.4+ only then we could just use a 'Hook' trait to handle all this and be done with it :D.

As that's not the case, I guess it depends whether or not we think that Hooks are going to be needed anywhere but the Mapper. If they're only needed in the Mapper it makes little sense having a second class just to handle them. However, if we're going to be adding Hooks to other classes then a lightweight class and a HookInterface probably make sense.

[vlucas]

Also, if you're going to depend on hooks always running, you're going to want to put them on the entity itself via the static hooks method returning an array. In a realistic scenario, you would almost never add a hook in a controller, because then it would only exist for a request to that same controller method. The 99% case is pre-validation for custom stuff, post-save for adding related items, post-delete for deleting related items, etc. and those are going to be added on the Entity class itself so that they are always run, no matter where the Entity is created and saved, not externally by loading a separate class somewhere else.