PHP 5.4 Event System, just looking for general feedback

Question

So I decided to create a trait that I could add to my classes that would add a simple way of binding functions to an event, and to fire those events. It ended up going a little further than I expected it to, and I am getting into things I do not normally work with, such as defining an object via a variable, $a = new $b(), and working with anonymous functions.

I don't really have any friends who program so I guess I am just looking for a second, third, or twentieth pair of eyes to look it over and give me some feedback. Below is the everything needed to create events, and over at https://github.com/mrkmg/phpevents I have a few examples.

event.php:

<?php

trait EventTemplate
{
    protected $_event_types = array();
    protected $_event_binds = array();
    protected $_event_defaults_processed = false;

    public function bind($type,$action)
    {
        $this->_event_check_defaults();
        return $this->_event_bind($type,$action);
    }

    public function unbind($type,$action)
    {
        $this->_event_check_defaults();
        if(!key_exists($action,$this->_event_binds))
            return false;
        unset($this->_event_binds[$type][array_search($action,$this->_events_binds)]);
        return true;
    }

    public function fire($type)
    {
        $this->_event_check_defaults();
        $this->_event_fire($type);
    }

    private function _event_bind($type,$action)
    {
        if(!key_exists($type,$this->_event_types))
        {
            throw new Exception('Type not defined');
        }
        if(!isset($this->_event_binds[$type]) || !is_array($this->_event_binds[$type])) $this->_event_binds[$type] = array();
        $this->_event_binds[$type][] = $action;
        return true;
    }

    private function _event_check_defaults()
    {
        if(!$this->_event_defaults_processed) $this->_event_process_defaults();
    }

    private function _event_set_type($type,$class="Event")
    {
        $this->_event_types[$type] = $class;
    }

    private function _event_fire($type)
    {
        $event = new $this->_event_types[$type]($type,$this);
        foreach($this->_event_binds[$type] as $bind)
        {
            if(is_callable($bind))
                $this->_event_fire_closure($bind,$event);
            elseif(is_string($bind))
                $this->_event_fire_string($bind,$event);
        }
    }

    private function _event_fire_string($string,&$event)
    {
        call_user_func($string,$event);
    }

    private function _event_fire_closure($closure,&$event)
    {
        $closure($event);
    }

    private function _event_process_defaults()
    {
        if(isset($this->_event_default_types))
        {
            foreach($this->_event_default_types as $type=>$class)
            {
                if(is_int($type))
                {
                    $type = $class;
                    $class = "Event";
                }
                $this->_event_set_type($type,$class);
            }
        }
        if(isset($this->_event_default_binds))
        {
            foreach($this->_event_default_binds as $event=>$methods)
            {
                foreach($methods as $method)
                {
                    $this->_event_bind($event,function($event) use($method){$this->{$method}($event);});
                }
            }
        }
        $this->_event_defaults_processed = true;
    }
}

class Event
{
    public $type;
    public $object;
    public $microtime;
    public $backtrace;

    const PRINT_HTML = 0;
    const PRINT_CMD = 1;

    public function __construct($type,&$object)
    {
        $this->type = $type;
        $this->object = &$object;
        $this->microtime = microtime(true);

        $backtrace = debug_backtrace();
        array_shift($backtrace);
        array_shift($backtrace);
        $this->backtrace = $backtrace;
    }

    /**
     * Bill Getas
     * http://www.php.net/manual/en/function.debug-backtrace.php#101498
    */
    public function print_backtrace($type = self::PRINT_HTML)
    {
        switch($type)
        {
        case 0:
            array_walk( $this->backtrace,function($a,$b) {print "<br /><b>". basename( $a['file'] ). "</b> &nbsp; <font color=\"red\">{$a['line']}</font> &nbsp; <font color=\"green\">{$a['function']}()</font> &nbsp; -- ". dirname( $a['file'] ). "/";}); 
            break;
        case 1:
            array_walk( $this->backtrace,function($a,$b) {print "\n".basename( $a['file'] )."\t{$a['line']}\t{$a['function']}()\t".dirname( $a['file'] ). "/";}); 
            break;
        default:
            throw new Exception('Could not understand type.');
        }
    }
}

?>

Here is an example of how it could be used for logging

logexample.php

<?php
include('event.php');

class Person {
    use EventTemplate;

    private $data = array();
    private $inited = false;

    public function __construct()
    {
        $this->_event_set_type('get_data');
        $this->_event_set_type('save_data');
        $this->_event_set_type('write_property');
        $this->_event_set_type('delete_data');
    }

    public function __get($name)
    {
        if (array_key_exists($name, $this->data)) {
            return $this->data[$name];
        }
        return null;
    }

    public function __set($name,$value)
    {
        if (array_key_exists($name, $this->data)) {
            $this->data[$name] = $value;
            $this->fire('write_property');
            return true;
        }
        return false;
    }

    public function get($id)
    {
        //GET info from database
        $this->data = array(
            'name'=>'Test Person',
            'email'=>'test@demo.com',
            'username'=>'tperson'
        );
        $this->inited = true;

        //Fire the get_data event
        $this->fire('get_data');

        return true;
    }

    public function save()
    {
        //send data to database

        //Fire update_data event
        $this->fire('save_data');
    }

    public function delete()
    {
        //remove data from database

        //Fire delete_data event
        $this->fire('delete_data');
    }

}

function logEv($event)
{
    echo 'Event: '.$event->type.' for user: '.$event->object->username;
    $event->print_backtrace(Event::PRINT_HTML);
    echo "<br /><br />\n";

}

//make a new person
$person = new Person;

//Bind events to logEv function
$person->bind('get_data','logEv');
$person->bind('save_data','logEv');
$person->bind('delete_data','logEv');
$person->bind('write_property','logEv');


//Load an existing user from db
$person->get('id_of_person');

//Change the users users email
$person->email = 'tperson@domain.com';

//Save the user to the db
$person->save();

//Change the users users username
$person->username = 'testp';

//Save the user to the db
$person->save();

//Delete the user from the db
$person->delete();



?>

This example does not use all the features, but gives you an idea.

Answer 1

I'm not sure I really understand the purpose of this code. From the looks of it, you are recreating the factory design pattern but in a more complicated fashion. Not that I can be sure that this statement is true, your code is pretty hard to follow. But that's what comes from using variable-variables and variable-functions. I'll go over the code and give you some suggestions, but I can't get too specific about the implementation because I'm unsure if I'm understanding it completely. If you are actually trying to create a class that creates other classes and uses their functions, then I would definitely take a look at the factory design pattern. If not, my apologies for misunderstanding your post.

Suggestions

Disclaimer: I know this was just an example for usage, but its all I have to go on. So my apologies in advance if I bash this code rather harshly. Take these suggestions with an open mind and try to see how they relate to your actual code.

First of all, please, for the love of god, always, always, always use braces in your statements. For a while there, when you were setting the $_event_binds[ $type ] array, I thought that if statement just went on to the end of the function. Lack of braces leads to difficult to read code. Difficult to read code leads to hard to maintain code. Hard to maintain code leads to a headache. Its a vicious cycle. Speaking of that if statement though: Why are you checking if its an array? I can find no other places where this is set for it to have magically become anything else. I would think just checking to see if it is set would be enough.

if( ! isset( $this->_event_binds[ $type ] ) ) {
    $this->_event_binds[ $type ] = array();
}

What is the purpose of _event_check_defaults()? This method is called from public methods bind(), unbind(), and fire(). Why? After the first run through nothing ever happens again. So even if this should be run, it should only be done once. The fact that this method is called in not one, but three different methods, and methods that are likely to see a lot of use, is not good. Not that anything happens during the first run through anyways, there are no default arrays. At least not in the example you gave me. Nor are those defaults defined as class properties anywhere, which they should be to avoid them falling into the public scope by accident, which I'm assuming you want to avoid seen as how you prefixed them like they are supposed to be private. And since you are setting them in the class properties, that also means changing that isset() to an empty() check instead. If this should only ever be run once, and should be done before anything else, it should be declared in the constructor where you are assured of that. Which also means no more need of the $_event_defaults_processed property, nor the _event_check_defaults() method wrapper, and it also means you will have to define those two arrays in the constructor as well. But why is it necessary at all? Just define default values to the $_event_types and $_event_binds and append onto those as needed. No need to take up processing power to do something you can manually do quicker and easier.

If you end up removing the _event_check_defaults() method, then you will want to look at combining the _event_* prefixed methods with their non prefixed wrappers where applicable.

I did not realize at first that this was a trait. I understand why traits would be ideal here, but I don't think this implementation is an accurate depiction of one. I admit, I'm not 100% on the the whole traits idea myself, but from my understanding they are used to enhance code reuse. So the idea behind it is that you give it code that is going to be run every single time it is used and then cover any specifics in the class that uses it. Right now it looks as if you are doing it backwards. I see no difference between this trait and a normal parent class. Again, not 100% on traits, I just think you might want someone else to take a look at this specifically to determine if its a good example of a trait.

Why do so many of your methods, bind() for example, return TRUE? I can't find anywhere where you use these return values. The only reason I could foresee needing a boolean return value on these kinds of methods is if I were trying to judge the method's success, in which case you need to return FALSE at some point. In some cases you are throwing an error as well. There's nothing wrong with that, I just don't see why the return is necessary even in this instance.

What is the purpose of $id in your get() method? I know this is an example, but was there supposed to be a purpose for this? You pass it as a parameter then never use it. I'm assuming it is meant to fetch the appropriate table from the database, but just wanted to clarify. Again, why does this return true?

Why does your setter return TRUE/FALSE? You don't verify that it has been set, so I see no purpose.

Your getter should not return NULL, it should throw an error. Returning NULL by default could cause problems. What if the actual value of that property is null? You will have no way of verifying that it is actually being returned. Or if you are using the NULL value to determine if it is a valid property, then you will be surprised when it tells you a property that should be accessible isn't just because it hasn't been set yet. Throwing an error ensures that your code does not try accessing properties you don't want it to and ensures you are made aware of the difference between an inaccessible property and an unset one.

I hope this helps.