2

在阅读一本书时，我遇到了以下功能：

/*
Update records in the database
@param String $table the table being updated
@param Array $changes array of changes field => value
@param String $condition the condition
@return Boolean
*/
public function updateRecords($table, array $changes, $condition)
{
    $update = "UPDATE " . $table . " SET ";
    foreach($changes as $field => $value)
    {
        $update .= "`" . $field . "` = '{$value}', ";
    }
    //remove trailing , (comma)
    $update .= substr($update, 0, -1);

    if($condition != '')
    {
        $update .= "WHERE " . $condition;
    }
    $this->executeQuery($update);
    //Not sure why it returns true.
    return true;
}

如果我错了，请纠正我，但这不是一个设计糟糕的功能，绝对没有数据过滤/检查。最重要的是，该函数总是返回“真”。

4

1 回答 1

0

正确的。它似乎是通过字符串连接构建 SQL 语句而不执行任何卫生处理。如果所有输入都可以信任，这可能public是可以接受的，但鉴于它是一个功能，我认为情况并非如此。

这return true似乎是多余的，因为它从不返回任何其他内容 - 作者可能打算将其作为确保函数完成的一种方式，这意味着它使用的代码静默失败，而不是调用die()或抛出异常。

在风格方面，作者与连接不一致：在某些地方它是与.运算符的直接连接，在其他地方它使用$placeholders.

如果$changes为空，则调用substr将失败，在构建列表后进行修剪是用逗号分隔列表项的错误方法。我就是这样做的（当然，从不使用 SQL 除外）：

$first = true;
foreach($changes as $field => $value ) {
    if( !$first ) $update .= ", ";
    $first = false;
    $update .= "`$field` = '{$value}'"; // NEVER DO THIS
}

想一想——这个功能唯一的好处就是它有据可查。

于 2013-08-06T02:35:25.390 回答