keeB Posted June 28, 2008 Share Posted June 28, 2008 i have a dispatcher class with a dispatch method. The chunk in question is: // ...do the stuff with whatever we have from the router/request object $class = $this->getClassName($this->rq->getParams()); $controller = new $class($this->rq); $controller->init(); if (method_exists($controller, $this->rq->action)) { $action = $this->rq->action; // could be 'viewAction' $p = $this->rq->getParams(); $out = call_user_func_array(array($controller, $action), $p['args']); } echo $out; I don't like that. Standardize, standardize, standardize. I can understand why you do it this way -- because PHP lets you, but, reading that is a mess and a half. First this line $class = $this->getClassName($this->rq->getParams()); $controller = new $class($this->rq); $controller->init(); These should be updated to use an Abstract Factory pattern [1]. The idea is to create the object for you and to return a 'known' type based on input. What if, $this->rq->getParams() was tainted and $class doesn't exist? I would prefer to see it like this: <?php interface Controller { public function init(); public function generate(); public function afterLoad(); public function afterRender(); public function setContext(Context $context); } class ActionController { private $args; private $context = null; public function __construct($args) { $this->args = $args; } public function setContext(Context $context) { $this->context = $context; } public function init() { //i assume this sets local variables, etc } public function afterRender() { } public function afterLoad() { } public function generate() { $this->afterLoad(); $tpl = new Template(); $tpl->setAll($this->context->getParams($page)); $tpl->setOutput($tpl->render($this->context->getPageTemplate())); $output = $this->afterRender(); return $output; } } class ControllerFactory { public static function getClass($type, $args) { switch ($type) { case 'viewAction': $a = new ActionController($args); $a->init(); return $a; case 'viewSomethingElse': $p = new SomethingElseController($args); $p->init(); return $p; default: throw new ActionNotFoundException('Could not find requested Action'); } } } // client code try { $class = ControllerFactory::getClass($this->rq->action, $this->rq->getParams()); $output = $class->generate(); if ($output == NULL) //do something ; echo $output; } catch (ActionNotFoundException) { //generate some error } ?> [1]: http://en.wikipedia.org/wiki/Abstract_Factory Quote Link to comment Share on other sites More sharing options...
keeB Posted June 28, 2008 Share Posted June 28, 2008 hmm possibly another thought - adding everything to a context, including the plugins? public function testAction() { $c = new Context($this); // context can access getRequest, getUser, etc methods if required by plugins $page = Page::loadByRequest($this->rq); $c->setParams($page->getParams()); // pass page vars (title, body, etc) to context - Django-ish, I suppose... $c->addPlugin('test'); // add a default plugin that ALWAYS gets used $c->addPlugins($page->getPlugins()); // add plugins that are attached to a page. $c->addView($this->tpl); // set the view object to use to render page return $c->renderContext('page.tpl'); } the thought being that my context object is left to handle plugin calls, variable setting, filters, etc and its render method uses a View class to render the final HTML response. thoughts? As long as the Context is not initializing anything, I don't see any harm. But, you should be careful about *what* you want to expose. Exposing too much is not a good thing Quote Link to comment Share on other sites More sharing options...
redbullmarky Posted June 28, 2008 Author Share Posted June 28, 2008 going back to earlier - as it's not always necessary to attach a plugin to the controller (by $this->addPlugin('my_plugin')), is it considered bad practice to have a sort of registry purely for plugins, so I can just pass in what I need as required? For example (I'll call it PluginManager for less of a better name for now) public function testAction() { $page = Page::loadByRequest($this->rq); PluginManager::addPlugin('nav'); // add nav by default PluginManager::addPlugins($page->getPlugins()); // add those associated with my page // when editing a page, I might want to do this... $form = $page->getEditForm(); $form = PluginManager::getEditForm($form); // makes a magic method call, which in turn calls the method of same name in each of the plugins (if exists) // when viewing a page, I might do something like this instead... $params = PluginManager::beforeRender($page->getParams()); // manipulate page vars // the page is constructed as normal $out = $this->tpl->render('page.tpl', $params); $out = PluginManager::afterRender($out); } as my need is specific, and plugins are essentially not just reacting to and changing page properties but all sorts of things, is this acceptable, or am I really going off tangent now? I still like the context idea and, to some extent, helps with a lot of my issues - however, when it comes to extra processing like the form that's generated by the model, I can't see it working as i'd like it to. (nb - the code above is just scratchy as an example/proof of concept, rather than tried/testing/working code or anything resembling what the final code may be like) Quote Link to comment Share on other sites More sharing options...
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.