Page 1 of 2

Building a front controller from scratch

Posted: Sat Jan 13, 2007 8:48 pm
by alex.barylski
The idea is simplicity (I've already copied Zend and implemented my own - with custom routers, etc) and now I need something simple as in KISS - also the title is slightly misleading as it's more than a front controller.

Lets set the stage using a single index.php as the entry point and using a switch() we delegate actions to certain "modules" or "pages"

Nomenclature is *not* important in this excersize I need practical *not* perfect.

Assume a Smarty 'like' template engine :P

Code: Select all

$tpl = new Smarty(); // Ignore incorrect API

switch($_GET['page']){
  case 'login_form':
  {
    $buff = $tpl->fetch('login_form.tpl');
    break;
  }
  case 'do_login':
  {
    $ret = attemptLogin($_POST['user'], $_POST['pass']);
    if($ret)
      header("Location: index.php?msg=success");
    else
      header("Location: index.php?msg=failure ");

    exit;    
  }

  $tpl->assign('body', $buff);
  echo $tpl->fetch('layout.tpl');
}
This is a simplified version of an application I have be asked to work on and my first order of action is to clean up the index.php by implementing my interpretation of a front controller - which is likely simplified but still...

From the above code I can make the following observations:

1) Both view/page selection and action/event handling is unorganzied

2) If a single event was attached to each view we could just have the event handler CASE come before the view selection CASE statement and use a fall through technique - so events are handled before view selection. This is of course impractical as many views have two or more events.

3) The order of execution is important (see #2) we should have events handled before view selection - as this would allow me to avoid using header() redirect's in some instances.

Any more?

Remember, I am *not* looking for a full blown MVC implementation but rather baby steps I can take to clean it up - nor am I looking for definitions of what a front controller is or is not. You can use pattern definitions, but use them loosely.

As it stands a page is a view and visa-versa and an event is an action and so on...

Knowing this, where would you start? What steps would you take to modularize this massive front controller (so to speak)?

I don't wish to write a fancy URL decomposition routine but need to stick with an approach which has a minimal impact as possible.

Suggestions, comments, ideas, etc???

Cheers :)

Posted: Sat Jan 13, 2007 9:35 pm
by daedalus__
I had to create a very simple and quick to implement front controller recently and here is what I used:
entry point wrote:

Code: Select all

include('view.class.php');
$View = new View();

include('top.inc.php');
if (! method_exists($View, $page))
{
	echo $page;
	echo '<br />';
	echo method_exists($View, $page);
	$View->pageError();
} else $View->$page();
include('foot.inc.php');
view.class.php wrote:

Code: Select all

class View
{
	public static function getNews($limit = null)
	{
		if (is_int($limit))
		{
			$sql = 'select UNIX_TIMESTAMP(news.timestamp), news.content from news limit '.$limit;
		}
		else
		{
			$sql = 'select UNIX_TIMESTAMP(news.timestamp), news.content from news';
		}
		$result = mysql_query($sql) or die(mysql_error());
		while ($rows = mysql_fetch_row($result))
		{
			echo '<b>'.date('r', $rows[0]).'</b>';
			echo '<p>'.$rows[1].'</p><br />';
		}
	}

	public function contact()
	{
		$this->includeFile('contact');
	}

	public function index()
	{
		$this->includeFile('home');
	}

	public function includeFile($string)
	{
		if (is_file('pages/'.$string.'.inc.php'))
		{
			include('pages/'.$string.'.inc.php');
		} else $this->pageError();
	}

	public function pageError()
	{
		echo '<h2>Oops! We\'re Sorry!</h2>';
		echo '<h4>There was an error and the page you requested could not be found!</h4>';
		echo '<p>Please press the back button on your browser or try clicking <a href="javascript: history.go(-1);" title="Back">here</a></p>';
	}
}
This could be cleaned up a lot but right now it is easy to code quickly with and that is what I need for the website I am doing.

Posted: Sat Jan 13, 2007 11:03 pm
by Christopher
1. It looks super

2. It is a Page Controller with all that that implies

Posted: Sat Jan 13, 2007 11:21 pm
by alvinphp
I think you should seperate the call of your view and the call of your event. This is not that much more complicated, but gives you a lot more flexibility. You would have to call your events first though so when you call the page it shows any changed information (duh).

Code: Select all

switch($_GET['page']){ 
  case 'page_login': 
  { 
        //prcoess event (if there is one)
        switch($_GET['event']){ 
           //imagine possible events here that you would call
        }
       //output your page
      $buff = $tpl->fetch('login_form.tpl'); 
       break; 
  } 
  case 'page_contact': 
  { 
    
       //prcoess event (if there is one)
       switch($_GET['event']){ 
           //imagine possible events here that you would call
       }
       //output your page
       $buff = $tpl->fetch('contact.tpl'); 
       exit;    
  } 

  $tpl->assign('body', $buff); 
  echo $tpl->fetch('layout.tpl'); 
}

Posted: Sun Jan 14, 2007 4:55 am
by Ollie Saunders
Daedalus, where is $page coming from? If its coming from the user ($_SERVER or $_GET perhaps) it looks like you've got an XSS vulnerability:

Code: Select all

if (! method_exists($View, $page))
{
        echo $page; // Possible XSS
@Hockey: I'm not really a front controller expert, I've never written one, so that's a good reason to get involved in this thread. First some comments:

Code: Select all

$tpl = new Smarty(); // Ignore incorrect API
What do you mean by this comment?

Code: Select all

case 'do_login':
  {
    $ret = attemptLogin($_POST['user'], $_POST['pass']);
    if($ret)
      header("Location: index.php?msg=success");
    else
      header("Location: index.php?msg=failure ");

    exit;   
  }
I don't think attemptLogin() is really part of the controllers responsibility. Actually you seem to just be serving templates depending on $_GET['page'] when you should really be redirecting program flow to something that can then choose to do whatever it likes.

Also 90% of the time a switch isn't needed and represents a maintenance problem. In this case if you want to add a new page you have to modify the switch, which isn't particularly clever.

OK here's what I would do (untested) I'm unfamiliar with Smarty so I'm going to leave out that stuff:

Code: Select all

<?php
$page = $_GET['page'];
$allowedPages = array('LoginForm', 'DoLogin');

// You will of course have to add data to $allowedPages when adding a 
// new page but this isn't as bad as updating a switch. Using a 
// bullet-proof validation could get round this but for now 
// whitelisting is easy and completely secure.
$toLoad = "controllers/$page.php";
$toInstantiate = "$pageController";

if (!in_array($page, $allowedPages) || !is_readible($toLoad)) {
    badPageRequest();
    exit;
}
require $toLoad;
if (!class_exists($toInstantiate)) {
    badPageRequest();
    exit;
}
$page = $toInstantiate();
$page->invoke();
You might want to use this

Code: Select all

abstract class Page
{
    abstract public function invoke();
}
That can obviously be added to as pages are likely to need common behaviour.

Posted: Sun Jan 14, 2007 12:10 pm
by alex.barylski
ole wrote:What do you mean by this comment?
I don't use Smarty but my own implementation of a native PHP template engine...I figured more people would be familiar with Smarty than my own API so I tried to mock Smarty the best I could remember (it's been a while)...Ignore the API as in...assume it's correct :P
ole wrote:I don't think attemptLogin() is really part of the controllers responsibility
Perhaps. But remember this is not a MVC implementation in the strictest sense. There are no controllers yet.

I can't afford to rip apart the application and rebuild using Zend front controller...so I need to take baby steps.

This is what I was thinking of doing first off to further modularize the switch:

Code: Select all

function eventHandler($p1, $p2)
{
  switch($p1){
    case 'do_login':
      attemptLogin($_GET['user'], $_GET['pass']);
      break;
  }
}

function viewSelector(...)
{
  switch($p){
    case 'login_form':
       //output your page 
       $buff = $tpl->fetch('contact.tpl'); 
       return $buff;
    }
}

// Application entry point - asume $p are the correct parameters
eventHandler($p);
$buff = viewSelector($p); 

$tpl->assign('body', $buff); 
echo $tpl->fetch('layout.tpl');
Of course this is not without it's problems.

1) Not really that modular - ideally the switch would be done with and each page/view or event/action would be in a seperate module. Parsing time is reduced and modularity is acceptable.

2) If I do assign arguments to the handler functions - which arguments do I pass? It would be easy enough to whip togather a trivial registry and pass only that as a sole argument but that requires extensive tinkering to existing codebase. Sure it's possible and this is maybe part of the refactoring process - but it's not feasable atthe moment.
alvinphp wrote:I think you should seperate the call of your view and the call of your event. This is not that much more complicated, but gives you a lot more flexibility. You would have to call your events first though so when you call the page it shows any changed information (duh).
That's another way of solving the problem. Although maybe more intuitive than my approach above...it would also require me to find each event for every page...assume the switch is unorganized so one event handler might be at the bottom whereas another on the top and the view selection in the middle...and then assume there are 10000+ CASE statements (there isn't anywhere near that but I always assume the worst and best scenario when solving problems).

The other problem is that the HTML would need adjustment - I think (which is the real nightmare).

Each page would have to now specify the event name and the view name in order for the handler to be invoked...there is no difference between how an event or a view is invoked...

Code: Select all

index.php?page=do_login
or

Code: Select all

index.php?page=login_form
See the problem?

Cheers :)

Posted: Sun Jan 14, 2007 3:01 pm
by daedalus__
$page comes from $_GET['page']

Posted: Sun Jan 14, 2007 4:41 pm
by Ollie Saunders
Daedalus- wrote:$page comes from $_GET['page']
Then you have an XSS vulnerability. You should escape your output htmlspecialchars() is best for this.

Posted: Sun Jan 14, 2007 4:42 pm
by Christopher
Hockey wrote:I can't afford to rip apart the application and rebuild using Zend front controller...so I need to take baby steps.
Ahhhh ... you are refactoring. It would help if you posted a stripped down version of the code you want to refactor (especially something we could run) so that we can think of ways to get from the current code to a something like very simple MVC controller architecture.

Posted: Sun Jan 14, 2007 4:52 pm
by Ollie Saunders
arborint wrote:Ahhhh ... you are refactoring. It would help if you posted a stripped down version of the code you want to refactor (especially something we could run) so that we can think of ways to get from the current code to a something like very simple MVC controller architecture.
Well I'm not sure if Hockey actually wants us to refactor but I just started doing it anyway :)

What can I say? *shrug* I like refactoring :D

Posted: Sun Jan 14, 2007 5:09 pm
by alex.barylski
arborint wrote:
Hockey wrote:I can't afford to rip apart the application and rebuild using Zend front controller...so I need to take baby steps.
Ahhhh ... you are refactoring. It would help if you posted a stripped down version of the code you want to refactor (especially something we could run) so that we can think of ways to get from the current code to a something like very simple MVC controller architecture.
Assume my "first" switch statement but add 300 CASE's. :)

I'm aware I am refactoring...but I'm not looking to rebuild or redesign. Whatever technique I use *must* work with existing code so I can't tinker with HTML templates to add more information to links (index.php?page=doSomething).

Posted: Sun Jan 14, 2007 8:31 pm
by alex.barylski
arborint wrote:1. It looks super

2. It is a Page Controller with all that that implies
1. Thanks

2. Small diff.

After much thought and deliberation...I would have to argue there is little difference between a front controller and page controller...After all they are both controllers ;)

Oh, and that example was slightly misleading as there are waaay more CASE statements than two logically organized examples...so although on a small scale it resembles something of a page controller...I think it's functioning more like a front controller (maybe a super controller as it sort does both).

But maybe i'm wrong 8)

Cheers :)

Posted: Sun Jan 14, 2007 9:16 pm
by daedalus__
I don't mean to say too much that is off topic but the echo $page; is only in there on the testing server, it's for debugging.

Posted: Mon Jan 15, 2007 12:56 am
by Christopher
Hockey wrote:Assume my "first" switch statement but add 300 CASE's.
All a very simple Front Controller would do is split that out into many Action Controller files. But with only one parameter you will end up with 300 files which is why Front Controllers usually add a second parameter to represent the state of the "page". So 'login_form' and 'do_login' would be two states of a 'login' Action.
Hockey wrote:I'm aware I am refactoring...but I'm not looking to rebuild or redesign. Whatever technique I use *must* work with existing code so I can't tinker with HTML templates to add more information to links (index.php?page=doSomething).
That's sort of the point of Refactoring ... to make internal changes to a subsystem without effecting others.

Posted: Mon Jan 15, 2007 10:43 am
by alex.barylski
arborint wrote:
Hockey wrote:Assume my "first" switch statement but add 300 CASE's.
All a very simple Front Controller would do is split that out into many Action Controller files. But with only one parameter you will end up with 300 files which is why Front Controllers usually add a second parameter to represent the state of the "page". So 'login_form' and 'do_login' would be two states of a 'login' Action.
Hockey wrote:I'm aware I am refactoring...but I'm not looking to rebuild or redesign. Whatever technique I use *must* work with existing code so I can't tinker with HTML templates to add more information to links (index.php?page=doSomething).
That's sort of the point of Refactoring ... to make internal changes to a subsystem without effecting others.
Refactoring without affecting the rest of the system (although ideal) is not always practical. I've always seen refactoring as making a design better overall - as a more cost effective approach to overhauling a codebase.