[SOLVED] Design question

[Stooney]

I'm half confused on this.  I have a registry as a singleton which for the most part stores my objects to be used wherever they're needed.  For example a logging class which logs whatever it is I need to log.  Now how should it be accessed?

 

Say my Database class has problems with a query and I want to log it so I can go back and fix it.  Should I just have an instance of the logging class in my database class?

 

<?php
class database{
    private $log;

    __construct($host, $user, $pass, $dbname, $log){
        //connect and stuff
        $this->log=$log;
    }

    public function query($query){
        //query fails, log it
        $this->log->log('sql', 'Failed Query: '.$query);
    }
}

 

This is how registry works

<?php
$registry=Registry::getInstance();
$log=new Log(...);
$registry->set('log', $log);

//etc for other objects
?>

 

That was just an example.  If I did it that way then there would be multiple instances of each class in multiple objects which doesn't seem like the right way to do it.  So what's the better way of doing this?  It seems like a common thing, I just can't figure out the design.

 

(This is all example code, so excuse typos etc.  Edit:  A quick read through the sticky makes me think this should have been in app design, whoops)

[DarkWater]

Well, since a database layer doesn't seem to inherently have anything to do with logging, you could always throw exceptions:

 

try {
    $db->query('something');
} catch (DbQueryException $e) {
    $log->log($e, Log::EXCEP);
}

 

And $db->query() could look like:

    public function query($query)
    {
        //do query
        if (query_failed) {
            throw new DbQueryException("Query $query failed");
        }
    }

[Stooney]

That makes sense DarkWater, thank you.  What about with an MVC architecture?  Should I just have a private $log variable for each controller?  Maybe just pass the whole registry to the controller?

 

As it stands, I instantiate my classes (db, log, auth, etc) at the start of the site.  They're then stored in the registry object (which is a singleton).  After all this the router loads the proper controller and invokes the requested method.  Naturally that method (or entire controller for that matter) doesn't have access to the registry.  Should I pass the entire registry object to my controller?  Or just the necessary parts depending on the controller (which might be tricky considering the router object doesn't know anything about that controller). 

[DarkWater]

You have several choices.  Since it's a singleton, you could always just statically get it and use it globally:

 

class Foo {
    public function index() {
        $log = Registry::get('log');
        //do stuff, use $log, etc
    }
}

 

Or, you could have a base class that all controllers inherit from and set it up like this:

 

<?php
class BaseController {
private $set_vars = array();

public function __construct() {
	//potentially do something here, and if a controller wants to override, have them call parent::__construct()
}

public final function __set($key, $value) {
	$this->set_vars[$key] = $value;
}

public final function __get($key) {
	if (array_key_exists($key, $this->set_vars)) {
		return $this->set_vars[$key];
	}
	throw new Exception('Illegal key retrieval');
}
}

 

Then, when you prepare to call the controller method, do something like:

if (class_exists($controller)) {
$control = new $controller();
}
else {
throw new Exception('Bad controller name');
}

if (method_exists($action, $control)) {
                $control->log = new Log();
                $control->view = new BaseView();
                call_user_func_array(array(&$control, $action), $params);
}
else {
        throw new Exception('Bad action name');
}

 

Now, I just conjured that up now, so it might have some types.  I know that the BaseController class doesn't though, because I tested it.

[Stooney]

Thank you very much DarkWater.  It all makes sense now.  I think I prefer the static method.

[DarkWater]

Thank you very much DarkWater.  It all makes sense now.  I think I prefer the static method.

 

Yeah, I figured you'd say that.  I just want to let you know that it's not REALLY the best way to do it, I was just showing you the option.  Having a base class and properly instantiating the controller and giving it $this->log and stuff is (imo) a much better plan.

[Stooney]

If it's the better option than that's what I will do.  I'd rather learn than cut corners.

[DarkWater]

Can I see how you currently route requests and call your controllers and stuff?

[Stooney]

This is what my router looks like right now.  I just noticed I could do away with setPath and do that in the construct, I'll fix that later.  You'll also notice some comments where code should be, I'm working on write that stuff still (like //show 404)

 

<?php
class Router{
private $path;

public function __construct(){
	//Nothing
}

public function setPath($path){
	$path=trim($path, '/\\');
	$path.=DS;

	if(is_dir($path)){
		$this->path=$path;
		return true;
	}
	throw new Exception('Invalid controller path');
}

private function getController(&$file, &$controller, &$action, &$args){
	if(isset($_GET['d']) && !empty($_GET['d'])){
		$destination=$_GET['d'];
	} else{
		$destination='index';
	}

	//Get Separate Parts
	$destination=trim($destination, '/\\');
	$parts=explode('/', $destination);

	//Find Controller
	foreach($parts as $part){
		$fullpath=$cpath.'Controller_'.$part;

		//Check if $fullpath is a dir
		if(is_dir($fullpath)){
			$cpath.=$part.DS;
			array_shift($parts);
			continue;
		}

		//Find the file
		if(is_file($fullpath.'.php')){
			$controller='Controller_'.$part;
			array_shift($parts);
			break;
		}
	}
	if(empty($controller)){
		$controller='Controller_index';
	}

	//Get Action
	$action=array_shift($parts);
	if(empty($action)){
		$action='index';
	}
	$file=$cpath.$controller.'.php';
	$args=$parts;
}

public function delegate(){
	//Analyze destination
	$this->getController($file, $controller, $action, $args);

	//File available?
	if(!is_readable($file)){
		//Show 404
		return false;
	}

	//Include the file
	include($file);

	//Instantiate the class
	$class=$controller;
	$controller=new $class();

	//Action available?
	if(!is_callable(array($controller, $action))){
		//Show 404
		return false;
	}

	//Run action
	$controller->$action($args);
}
}
?>

[DarkWater]

Try adapting my routing code to your class, and you can also use call_user_func_array() to pass in an array of parameters instead of just one flat array.  Make sure to inherit all controllers from BaseController. =P