API Style and Readability

I am currently working with a team to develop a php API. It is very OOP and is meant to give developers a nice base to build on. It’s not a rapid development framework like cakephp, so we are not looking for “less lines is better” kind of code. We are looking to make it very solid, and easily maintainable.

We have come to a kind of issue here about how to set up database queries. Right now it is done via a global object like this:

$tru->query->run(array(
	'name' => 'get-user',
	'sql' => 'SELECT * FROM users WHERE id = 10',
	'connection' => 'member'
));

while ($arr = $tru->query->getArray('get-user')) {
	$id = $arr['id'];
	...
}

I am trying to make this a much more OOP model. So I want to change it to the following:

$conn = $tru->connectionManager->get('member');
$q = $conn->run('SELECT * FROM users WHERE id = 10');

while ($q->next()) {
	$id = $q->getId();
	...
}

Internally, there seems to be a little concern that this is too radical for the PHP community, so we are looking for some outside opinions.

Any comments would be a great help

Interesting statement. :shifty:

Can you explain why you think your latter approach is radical?

The standard approach to PHP (as far as most script kiddies are concerned) is to do some version of the following:

$conn = mysql_connect(...);
mysql_select_db('member', $conn);

$result = mysql_query('SELECT * FROM users WHERE id = 10');

while ($arr = mysql_fetch_array($result)) {
	$id = $arr['id'];
}

So my team is worried that by moving away from that way of using queries is going to be jarring. From my perspective, that way of doing things is very poor use of code.

  • it isn’t oop meaning that utilizing multiple database connections becomes problematic
  • it utilizes direct data access rather than a read only object
  • it is not clear there are other things you can do with result object, ie:

    php $q->first(); $q->previous();

    are fairly obvious commands once you see the next method.

So what you’re after is something more like this?


$db = $this->_db;
$data = array();
$sql = sprintf(
    'SELECT Alerts.Id, FirstName, LastName, Purchased, Type
    FROM Alerts 
    WHERE
        Enabled = 1 AND
        UserId = %d
    ORDER BY Purchased',
    $db->escape('userId')
);
$result = $db->query($sql);
if ($db->rowCount($result) > 0) {
    while ($row = $db->fetch($result)) {
        $data[] = $row;
    }
}
return $data;

How about simplifying it even further using PEAR’s MDB2?


$db = $this->_mdb2;
$data = $db->queryAll(
    'SELECT Alerts.Id, FirstName, LastName, Purchased, Type
    FROM Alerts 
    WHERE
        Enabled = 1 AND
        UserId = ' . $db->quote($userId, 'integer') . '
    ORDER BY Purchased'
    , array('integer', 'text', 'text', 'timestamp', 'text')
    , MDB2_FETCHMODE_ASSOC
);
if (PEAR::isError($data)) {
    die($data->getMessage());
}
return $data;

Then, switching to using prepared statements is a snap:


$db = $this->_mdb2;
$db->loadModule('Extended');
$data = $db->extended->getAll(
    'SELECT Alerts.Id, FirstName, LastName, Purchased, Type
    FROM Alerts 
    WHERE
        Enabled = 1 AND
        UserId = ?
    ORDER BY Purchased'
    , array('integer', 'text', 'text', 'timestamp', 'text') // return types
    , array($userId) // param
    , array('integer') // param type
    , MDB2_FETCHMODE_ASSOC
);
if (PEAR::isError($data)) {
    die($data->getMessage());
}
return $data;

Thanks for the input!

After reading the “where should I post my thread” thread, I have moved this discussion to the application design subforum, so please post any replies there:

Most users are not going to be using basic SQL in the API, they will be using data models:

$user = $userManager->modelFactory(10); //get user with id 10
$user->setFirstName('Eric');
$user->save();

Due to this, most sql will be written combining multiple tables together, and will almost exclusively be select statements and other multi row queries. I haven’t looked at PEAR::MDB2, but looking over the code it seems there is a lot of extra syntax that doesn’t really aid in the maintainability of the code.

While I get the idea of being able to pull lots of rows out of a database, that becomes very easy for someone to abuse. Pulling an extra 10 or 20 rows out is never a good thing, and making it easy to do that is not really good API design in my opinion. In general, I try to avoid any kind of “getAll” code that is not custom per user.

Let the active record and mapper war begin!!!

Honestly, there isn’t anything special about what your doing. So I don’t see a problem with it. You’ve described an iterator and an active record.

Thanks oddz, for whatever reason I didn’t realize this was just the iterator pattern. That completely answers my question.