Generic method for database calls

Question

Background

Breaking from MVC, I've implemented the following architecture:

POST/GET ➤ PHP ➤ Database Calls ➤ XML ➤ XSLT ➤ HTML

All database interactions are relegated to the call method in the Database class, without exception. This adheres to the DRY principle. This design allows the following, which is a critical application feature:

... ➤ XML ➤ XSLT ➤ LaTeX ➤ PDF

The same code that retrieves data to render as an interactive web page is exactly the same code and XML used to render a PDF. Eventually additional output formats will be required:

... ➤ XML ➤ XSLT ➤ XML
... ➤ XML ➤ XSLT ➤ ASCII Text

Using this architecture, the same database functions can be used to generate completely different output formats.

Common Code

The code that makes database calls can do so in one of three ways:

$this->call( "database_function", "c1, c2, c3", $v1, $v2, $v3 );
$this->call( "database_function", "c1, c2, c3" ) );
$this->call( "database_procedure" ) );

The parameters are variable arguments.

Database Class

My questions pertain to the database class itself:

<?php
use PDO;
use PDOException;
/**
 * Used for interacting with the database. Usage:
 * <pre>
 * $db = Database::get();
 * $db->call( ... );
 * </pre>
 */
class Database extends Obj {
 private static $instance;
 private $dataStore;
 /**
 * Sets the connection that this class uses for database transactions.
 */
 public function __construct() {
 global $dbhost;
 global $dbname;
 global $dbuser;
 global $dbpass;
 try {
 $this->setDataStore(
 new PDO( "pgsql:dbname=$dbname;host=$dbhost", $dbuser, $dbpass ) );
 }
 catch( PDOException $ex ) {
 $this->log( $ex->getMessage() );
 }
 }
 /**
 * Returns the singleton database instance.
 */
 public function get() {
 if( self::$instance === null ) {
 self::$instance = new Database();
 }
 return self::$instance;
 }
 /**
 * Call a database function and return the results. If there are
 * multiple columns to return, then the value for $params must contain
 * a comma; otherwise, without a comma, the value for $params is used
 * as the return column name. For example:
 *
 *- SELECT $params FROM $proc( ?, ? ); -- with comma
 *- SELECT $proc( ?, ? ) AS $params; -- without comma
 *- SELECT $proc( ?, ? ); -- empty
 *
 * @param $proc Name of the function or stored procedure to call.
 * @param $params Name of parameters to use as return columns.
 */
 public function call( $proc, $params = "" ) {
 $args = array();
 $count = 0;
 $placeholders = "";
 // Key is zero-based (e.g., $proc = 0, $params = 1).
 foreach( func_get_args() as $key => $parameter ) {
 // Skip the $proc and $params arguments to this method.
 if( $key < 2 ) continue;
 $count++;
 $placeholders = empty( $placeholders ) ? "?" : "$placeholders,?";
 array_push( $args, $parameter );
 }
 $sql = "";
 if( empty( $params ) ) {
 // If there are no parameters, then just make a call.
 $sql = "SELECT $proc( $placeholders )";
 }
 else if( strpos( $params, "," ) !== false ) {
 // If there is a comma, select the column names.
 $sql = "SELECT $params FROM $proc( $placeholders )";
 }
 else {
 // Otherwise, select the result into the given column name.
 $sql = "SELECT $proc( $placeholders ) AS $params";
 }
 $statement = $this->getDataStore()->prepare( $sql );
 //$this->log( "SQL: $sql" );
 for( $i = 1; $i <= $count; $i++ ) {
 //$this->log( "Bind " . $i . " to " . $args[$i - 1] );
 $statement->bindParam( $i, $args[$i - 1] );
 }
 $statement->execute();
 $result = $statement->fetchAll();
 $this->decodeArray( $result );
 return $result;
 }
 /**
 * Converts an array of numbers into an array suitable for usage with
 * PostgreSQL.
 *
 * @param $array An array of integers.
 */
 public function arrayToString( $array ) {
 return "{" . implode( ",", $array ) . "}";
 }
 /**
 * Recursive method to decode a UTF8-encoded array.
 *
 * @param $array - The array to decode.
 * @param $key - Name of the function to call.
 */
 private function decodeArray( &$array ) {
 if( is_array( $array ) ) {
 array_map( array( $this, "decodeArray" ), $array );
 }
 else {
 $array = utf8_decode( $array );
 }
 }
 private function getDataStore() {
 return $this->dataStore;
 }
 private function setDataStore( $dataStore ) {
 $this->dataStore = $dataStore;
 }
}
?>

A BaseController class abstracts the persistence layer from subclasses as follows:

 protected function call() {
 $db = Database::get();
 return call_user_func_array( array( $db, "call" ), func_get_args() );
 }

Thus subclasses can call $this->call(...) without knowing about the database.

Example Usage #1

The database functionality is used, for example as an AJAX request, as follows:

class AjaxPhotographCategory extends Ajax {
 function __construct() {
 parent::__construct();
 }
 public function handleAjaxRequest() {
 return $this->json( $this->call(
 "get_photograph_category_list", "id, label, description" ) );
 }
}
?>

Example Usage #2

Here is another example usage:

 private function exists( $cookie_value ) {
 $result = $this->call( "is_existing_cookie", "existing", $cookie_value );
 return isset( $result[0] ) ? $result[0]["existing"] > 0 : false;
 }

The return line could be abstracted and simplified, but that's not an important detail.

Example Usage #3

The code to generate an XML document resembles (where "x" is the column name to hold XML content):

 private function getXml() {
 $result = $this->call( "generate_account_xml", "x", $this->getId() );
 return isset( $result[0] ) ? $result[0]["x"] : $this->getErrorXml( "account" );
 }

Example Usage #4

Information is added to the database as follows:

 private function authenticate() {
 $this->call( "authentication_upsert", "",
 $this->getCookieToken(),
 $this->getBrowserPlatform(),
 $this->getBrowserName(),
 $this->getBrowserVersion(),
 $this->getIp()
 );
 }

Since this calls a stored procedure, the return value can be discarded. As call requires two values (database routine name and return column name) to precede the arguments passed into the database routine, the second parameter must be filled with something (hence "").

This could be abstracted by splitting call into callProc and callFunc, so I'd consider the extraneous empty string parameter to be a minor detail.

Questions

My questions are:

• What performance impacts or security issues may be encountered?
• What improvements could be made to alleviate those issues?
• How could the implementation be changed to ease maintenance?

• mheinzerling: Do I get this right that you are trying to move most of your logic to stored procedures?
• Dave Jarvis: All persistence logic is in stored procedures already. The entire PHP code base has only three SELECT statements (shown). There are no INSERT, UPDATE, or DELETE statements in the code base and my plan is to keep it that way. My concern is around the performance and security impacts of the Database class.
• mheinzerling: Although I'm getting OT here. Isn't this a maintenance and testing nightmare? I'm really interested in some sample procedures here?

Answer 1

Here's my sniff at the first things I saw for the code (not the implementation).

public function __construct() {
 ...
 try {
 $this->setDataStore(
 new PDO( "pgsql:dbname=$dbname;host=$dbhost", $dbuser, $dbpass ) );
 }
 ...

Above, a class is instantiating a class, and not injecting dependencies.

Pass arguments to the constructor with the dependencies that a class requires:

public function __construct(PDO $pdo) {
 $this->dataStore = $pdo;
}

In addition, use the magic __get() and __set() methods instead of explicitly declared accessors to vastly improve readability and maintenance ease. Here's the documentation, remember it, and learn to love it.

public function __get(string $name) {
 // Logic/cases to check name for correct stores here.
}

• Dave Jarvis: Exposing the PDO class through the constructor would mean that any class wanting to use the Database class would need to know about PDO, which would defeat the purpose of a Database singleton.
• Dave Jarvis: The __get and __set methods look interesting. If the __get method can be made private, that pattern will be quite handy.
• Amelia: @DaveJarvis in PHP, magic methods must be public. You can control access inside that function, though, and keep all attributes private/protected
• mheinzerling: The magic methods are 3 times slower. Although you should not start a premature optimization, you should be aware of this fact.
• mheinzerling: Maybe you can replace your global variables by some kind of configuration object.