HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Large Zend_Db query

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
zend_dblargequery

Problem

Is there a better way to accomplish the following?

/**
 * Performs an access check given a user.
 *
 * @param Cas_Acl_Sid $user The user or SID being checked.
 * @param Cas_Acl_Privilege $privilege The privilege to check.
 * @return int|null 1 if user access allowed, 2 if group access allowed, false if access is denied, null if access cannot be determined.
 */
public function accessCheck(Cas_Acl_Sid $user, Cas_Acl_Privilege $privilege)
{
    $db = Zend_Db_Table_Abstract::getDefaultAdapter();
    $usersQuery = $db->select()->from('AccessControlEntries', array('Allowed', new Zend_Db_Expr('1 AS Type')))
        ->where('Acl = ?', $this->_id)
        ->where('Sid = ?', $user->GetGuid())
        ->where('Privilege = ?', $privilege->GetId());

    $groupsQuery = $db->select()->from('AccessControlEntries', array('Allowed', new Zend_Db_Expr('2 AS Type')))
        ->join('GroupMembers', $db->quoteIdentifier(array('GroupMembers', 'Group')) . ' = ' .
                               $db->quoteIdentifier(array('AccessControlEntries', 'Sid')), array())
        ->where('Acl = ?', $this->_id)
        ->where($db->quoteIdentifier(array('GroupMembers', 'User')) . ' = ?', $user->GetGuid())
        ->where('Privilege = ?', $privilege->GetId());

    $query = $db->select()
        ->union(array($usersQuery, $groupsQuery), Zend_Db_Select::SQL_UNION_ALL)
        ->order('Type')
        ->order('Allowed')
        ->limit(1);

    $dbResult = $db->fetchAll($query);

    if (!count($dbResult))
    {
        return null;
    }
    else {
        if ($dbResult[0]['Allowed'])
        {
            return (int)$dbResult[0]['Type'];
        }
        else
        {
            return false;
        }
    }
}

Solution

Stored procedure

The query above is enough big to move it from the application layer to the database in the form of a stored procedure. It will be clearer and faster, the only disadvantage is that an SP has a hard dependency on the database type (MySQL, MSSQL, other).

Return value(s)

You are returning 3 types in your method: null, integer and boolean.
Keep your logic clean, and return always one type if you can (yes, PHP has it's type juggling "issue" but still, clean code talks).

public function accessCheck(Cas_Acl_Sid $user, Cas_Acl_Privilege $privilege)
{
    //execute the query in a stored procedure

    $dbResult = // stored procedure result

    if (empty($dbResult) || !$dbResult[0]['Allowed'])
    {
        return 0;
    }

    return (int)$dbResult[0]['Type'];
}

Code Snippets

public function accessCheck(Cas_Acl_Sid $user, Cas_Acl_Privilege $privilege)
{
    //execute the query in a stored procedure

    $dbResult = // stored procedure result

    if (empty($dbResult) || !$dbResult[0]['Allowed'])
    {
        return 0;
    }

    return (int)$dbResult[0]['Type'];
}

Context

StackExchange Code Review Q#676, answer score: 2

Revisions (0)

No revisions yet.