Help with OOP and globals

[callumacrae]

cp.php:

<?php

error_reporting(E_ALL); 
ini_set("display_errors", 1); 

function callback($buffer)
{
	global $page;
	if ($page->error)
	{
		$code = $page->header("Error");
		$code .= $page->error;
		$code .= $page->footer();

		return $code;
	}
	else return $buffer;
}

ob_start("callback");

include("inc/includes.php");

$module_id = request_var('m', (int) 1);

$sql = "SELECT * FROM `base11_modules` WHERE `display` = 1 AND `id` = $module_id";
$result = $conn->query($sql);
$result->setFetchMode(PDO::FETCH_OBJ);

if (!$result->rowCount())
	$page->error("404 Error - Page not found");

while ($row = $result->fetch())
{
	if ($error) break;

	$page->tab_id = $row->id;
	if ($u->perms($row->perms))
	{
		include("modules/".$row->location.".php");
		break;
	}
	else
	{
		$page->error("You do not have sufficient permissions to view this page.");
	}
}


$page->footer();

ob_end_flush();

?>

includes.php:

class body
{

	function __construct()
	{
		$this->template = "../template/current/";
		if (!file_exists($this->template))
				$this->template = "template/current/";
		
		$link = "../";
		if (!file_exists($link."inc/includes.php"))
			$link = "./";
		$this->link = $link;

		$this->hasheader = 0;
		$this->error = 0;
	}
	
	function error($error)
	{
		$this->error = $error;
	}

	function display($page)
	{
		if ($page == "header")
			$this->header($title);
		else
		{
		
			$link = "../";
			if (!file_exists($link."inc/includes.php"))
				$link = "./";

			$page = file_get_contents($this->template."pages/".$page.".html");
			echo $this->parse(0, $page);
		
		}
	}
	
	function find()
	{
		return array_keys($this->replace());
	}
	
	function footer()
	{
		if ($this->hasheader)
			$this->display("footer");
		die();
	}
	
	
	function header($title="None Specified")
	{
		$page = file_get_contents($this->template."pages/header.html");
		
		echo $this->parse($title, $page);
		$this->hasheader = 1;
	}
	
	function parse($title, $page)
	{
		return preg_replace($this->find(), $this->replace($title), $page);
	}
	
	function replace($title=0)
	{
		global $conn, $u;
		$result = $conn->query("SELECT * FROM `base11_modules` WHERE `display` = 1 AND `parent` = 0 ORDER BY `order`");
		$result->setFetchMode(PDO::FETCH_OBJ);

		while ($row = $result->fetch())
		{
			if ($u->perms($row->perms))
			{
				if ($row->id == $this->tab_id) $navlinks_a[] = '<b><a href="' .$this->url("cp.php?m=".$row->id, 0). '">' .$row->name. '</a></b>';
					else $navlinks_a[] = '<a href="' .$this->url("cp.php?m=".$row->id, 0). '">' .$row->name. '</a>';
			}
		}
		
		$navlinks_count = count($navlinks_a);
		$this_count = 0;
		
		foreach ($navlinks_a as $value)
		{
			$navlinks .= $value;
			$this_count++;
			if ($this_count < $navlinks_count)
			{
				$navlinks .= " &bull; ";
			}
		}
	
		$replace = array(

			'/{PAGE_TITLE}/'		=>	$title,
			'/{BIG_TITLE}/'			=>	"Base 11 Studios &bull; ".$title,
			'/{NAV_LINKS}/'			=>	$navlinks,
			'/{CSS_LOC}/'			=>	$this->template."css/style.css",
			'/{CSS_DIR}/'			=>	$this->template."css/",
			'/{IMG_DIR}/'			=>	$this->template."images/",

		);
		
		return $replace;
	}
	
	function url($text, $m=1)
	{
		$text = append_sid($text);
		if ($m) $text .= "&m=".$this->tab_id;
		return $text;
	}

}

$page = new body;

?>

There is an error in function callback() - it can find $page->error correctly, but not $page->header() or $page->footer(). Does anyone know why this is? It does not display any errors, just a blank page.

~Callum

 

[callumacrae]

-post deleted-

~Callum

 

[callumacrae]

When I said it was fixed I lied, it still doesn't work.

~Callum

 

[misson]

There isn't enough to test and, at the same time, there's too much (for example, the DB access code is probably extraneous; if not, the sample needs to be extended to include a test table schema & test data). Please post a minimal test case and a link to a live page. Creating the minimal test case just might reveal the source of the problem.

There are a couple of design changes you can make that will make certain tasks easier. One is to abstract out data access to a data access layer. Rather than defining & executing queries all over the place, have specific classes that handle DB access. Among the advantages is that it makes testing easier, since you can replace the DB data access objects with stub class instances. There are a couple of options as to how the data access layer hooks into everything else; one particularly flexible option is to use inversion of control, which also helps to avoid globals.

Another potential change is to replace output buffering with exceptions (body::error would throw an exception, and the body of the callback function would be refactored as the exception handler) or apply the builder pattern (when an error is detected, the builder discards any part of the page already constructed but made unnecessary by the error and replaces it with the error message). This might resolve the blank page issue.

There are a few minor stylistic improvements that could be made, once everything is working.

cp.php:

<?php
[...]
$module_id = request_var('m', (int) 1);
$sql = "SELECT * FROM `base11_modules` WHERE `display` = 1 AND `id` = $module_id"; 
$result = $conn->query($sql);

Without seeing the spec or definition of request_var, I can't say for certain, but this is potentially vulnerable to injection. On the other hand, if request_var is designed to sanitize input, then it's unnecessary with PDO. Since part of the query is notionally a parameter, make it an actual parameter and use a prepared statement.

if (!$result->rowCount())
	$page->error("404 Error - Page not found");

while ($row = $result->fetch())

One nice feature of PDOStatement is that it supports the Traversable interface, which means you can use foreach:

foreach ($result as $row) {

The advantage of foreach here is it makes it easier to replace the SQL result with an array for testing, as does a data access layer.

However, since the result returns at most one element, looping is misleading and error-prone. An over-clever programmer might use a loop to skip empty result sets.

$result = $query->execute(...);
# if result is empty, nothing happens.
foreach ($result as $row) {
    $page->display($row);
}

Since you want to output something when the result is empty, get rid of the loop. It will be clearer that the query returns no or 1 result.

if (($row = $result->fetch())) {
    $page->tab_id = $row->id; 
    if ($u->perms($row->perms)) 
    { 
        include("modules/".$row->location.".php"); 
    } 
    else 
    { 
        $page->error("You do not have sufficient permissions to view this page."); 
    }
} else { # no result
    $page->error("404 Error - Page not found");
}

This also avoids the problem mentioned in the rowCount documentation, that some (rather, many) DBs don't return an accurate value for the row count (though if there is at most 1 result, the row count is probably accurate).

{
    if ($error) break;

Is that supposed to be $page->error, or is $error another global variable? This might be the source of the blank page, assuming there aren't any other die()s hiding in the code.

To avoid global namespace pollution (and the possibility of forgetting to declare a variable as a global in a function), you can use static class variables or (in PHP 5.3) namespaces).

Lastly, lower case class names look off. The convention (see also the PEAR and Zend conventions) is to use upper case for class names.

 

[misson]

I almost forgot: it's best not to use SELECT *. Select only the columns you need.

 

[callumacrae]

I've kinda read half of your post, I'll finish it later XD

I'm too lazy NOT to use *

~Callum

 

[misson]

I've kinda read half of your post, I'll finish it later

Other than the mysterious $error variable, there's nothing that needs immediate attention.

I'm too lazy NOT to use *

Shame, shame. It doesn't take much to type out a few column names.

 

[Zubair]

I've kinda read half of your post, I'll finish it later XD

I'm too lazy NOT to use *

~Callum

Lazy Guy xD
it takes only 2 minutes to read