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?

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.
}