PHP/MySQL PDOを使った複数のSQLを文を実行する処理

Question

1つ関数内で複数のSQL文を実行する場合の処理なのですが、下記ソースのように2回(実行回数)「execute();」を使用して処理してしまっても、可読性など問題はないでしょうか。

こうすれば可読性が上がったり、短縮してかけるよ、という事があればご教授いただけると、大変助かります。

function cuteInsert() {
    $dbh = connectDb();
    $dbh->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);

    // 1回目
    $post_id = h($_POST['post_id']);
    $sql = "insert into post_ip(post_id, remote_addr, user_agent, created ) values (:post_id, :remote_addr, :user_agent, now())";
    $stmt = $dbh->prepare($sql);
    $stmt->setFetchMode(PDO::FETCH_ASSOC);
    $stmt->bindValue(':remote_addr', $_SERVER['REMOTE_ADDR'], PDO::PARAM_STR);
    $stmt->bindValue(':user_agent', $_SERVER['HTTP_USER_AGENT'], PDO::PARAM_STR);

    try {
      $stmt->execute();
    } catch (\PDOException $e) {
      throw new \Exception('今日は投稿できません。');
    }

    // 2回目
    $eva = h($_POST['evaluation']);
    if ($eva == "0") {
      $evaluation = 1;
    } else {
      $evaluation = $eva + 1;
    }

    $cute_sql = "update post set evaluation = :evaluation where post_id = :post_id";

    $stmt = $dbh->prepare($cute_sql);
    $stmt->setFetchMode(PDO::FETCH_ASSOC);
    $stmt->bindParam(':post_id', $post_id, PDO::PARAM_INT);
    $stmt->bindParam(':evaluation', $evaluation, PDO::PARAM_INT);
    $stmt->execute();

    return $evaluation;
  }

↓Kosuke_Shibuyaさん、KiyoshiMotokiさんにご指摘いただいた内容を加味し、ソースを修正しました。

  function cuteInsert() {
    $dbh = connectDb();
    // $post_id = h($_POST['post_id']); から変更
    $post_id = filter_input(INPUT_POST, 'post_id');

    $sql = "insert ignore into post_ip(post_id, remote_addr, user_agent, created ) values (:post_id, :remote_addr, :user_agent, now())";
    $dbh->beginTransaction();
    $stmt = $dbh->prepare($sql);
    $stmt->bindValue(':post_id', $post_id, PDO::PARAM_INT);
    $stmt->bindValue(':remote_addr', $_SERVER['REMOTE_ADDR'], PDO::PARAM_STR);
    $stmt->bindValue(':user_agent', $_SERVER['HTTP_USER_AGENT'], PDO::PARAM_STR);
    $stmt->execute();
    $count = $stmt->rowCount();

    // 登録できた場合
    if ($count == "1") {
      $eva = filter_input(INPUT_POST, 'evaluation');
      if ($eva == "0") {
        $evaluation = 1;
      } else {
        $evaluation = $eva + 1;
      }

      $cute_sql = "update post set evaluation = :evaluation where post_id = :post_id";

      $stmt = $dbh->prepare($cute_sql);
      $stmt->bindValue(':post_id', $post_id, PDO::PARAM_INT);
      $stmt->bindValue(':evaluation', $evaluation, PDO::PARAM_INT);
      $stmt->execute();
      $dbh->commit();
    // 登録できなかった場合
    } else {
      $dbh->rollBack();
    }
  }

Answer 1

提示のコードだと、「update」の処理は絶対に行われませんね…
（コードに間違いがあるので…）

下記ソースのように2回(実行回数)「execute();」を使用して処理してしまっても、可読性など問題はないでしょうか

「可読性」を考慮するなら、自分だったら処理は分けますし、グローバルな関数は定義せずに、クラスで実装します。

他に気づいたところでは、

この部分は、別の関数に切り分けする。

$dbh = connectDb();
$dbh->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);

提示のコードで以下の部分は無意味

$stmt->setFetchMode(PDO::FETCH_ASSOC);

例外ブロックを変更すべき

try ~ catch (\PDOException $e) {
}

以下の部分

$post_id = h($_POST['post_id']);

ここで使われている h() が htmlspecialchars 相当の機能であるなら、使用方法を間違っています。

以下の2つについても、\ の意味を理解せずに使っているのかなと。

$dbh->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);

catch (\PDOException $e) {

Comments

[junkboy]

大変丁寧にご回答いただき、ありがとうございます。
頂いた回答を1つ1つ読み込みつつ、ソースを修正していきたいと思います。

取り急ぎ、お礼まで。

[junkboy]

ご指摘いただいた部分について

・この部分は、別の関数に切り分けする。
'$dbh->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);'
DBにアクセスする関数'connectDb();'に組み込みました。

'connectDb();'を呼び出す度に指定しており、モヤモヤしていた箇所だったので、ご指摘いただけた事で確信を持って調べる事ができました。ありがとうございます。

処理を分けてクラスで実装される、という部分について、どういうクラスを作成すればよいのか考え中です。

例えばクラス(.phpファイル)が複数あり、それぞれにDBにアクセスする必要がある場合ですと、'connectDb();'のような関数はfunctions.phpに記述しても問題ないでしょうか。

セキュリティ的にクラス毎に実装したほうが良いなどありましたら、ご指摘いただけると幸いです。

・$stmt->setFetchMode(PDO::FETCH_ASSOC);
他の処理で取得した値を配列に格納した時に使用していたものを、そのままコピペしていましたので、削除しました。

ご指摘ありがとうございます。

・htmlspecialchars
仰るとおり、使用方法を間違っていました。
いろいろと調べたところ、HTMLに出力する際にサニタイズするのが正しいということだったので、修正しました。

・\ の意味
こちらもおっしゃるとおり、意識せずに使用していました。

'$dbh->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);'

を指定しているので、エスケープ処理している上に、’\PDOExceptio’でさらにエスケープ処理をしていた、という認識で良かったでしょうか。

通常'\'は記号をエスケープするときに使用するのだと思いますが、'\PDOExceptio'にはどのような意味があるのでしょうか。

質問を重ねる形で大変恐縮ですが、お時間あるときにでもご教授いただけると幸いです。

[退会済みユーザー]

PDO の 前につける `\` は、名前空間を意味する記号です。
http://php.net/manual/ja/language.namespaces.rationale.php

[退会済みユーザー]

クラスについて
同種の関数のまとまりを持たせる意味もあるので、自分は、
・DB接続
・SELECTの実行
・UPDATEの実行
・INSERTの実行
・DELETEの実行
というような感じでクラスにまとめています。

[junkboy]

・'\'について
調べても辿りつけなかったので、大変助かります。

・クラスについて
なるほど、DBで行う処理内容によって、クラスを作っておられるのですね。
大変参考になります。

投稿した内容とは違う質問にも関わらず、ご丁寧に回答いただき、ありがとうございました。

Answer 2

細かい話ですが、
1回目のクエリ実行ではbindValueを、2回目ではbindParamを使用しているのが気になります。
"可読性"という点では、特に理由がなければどちらかに統一した方が良いと思います。

以下、ご質問の内容（行数・可読性）とは若干ズレますが、提示いただいたコードで気になった点を上げさせていただきます。
（Kosuke_Shibuya様の回答と重複する内容もあります）

レコードの存在確認について

post_ipテーブルに既にレコードが登録されているか否かを、
PDOExceptionが送出されたか否かで判断するのはよろしくありません。

この方法では、DBに障害が発生した場合など、本当に例外的な状況との区別がつかないからです。

MySQLをお使いであれば、insert into ...をinsert ignore into ...に変更し、
PDOStatement::rowCount()メソッドで登録された行数を確認する方が良いです。
http://php.net/manual/ja/pdostatement.rowcount.php

トランザクションを使用していない

トランザクションを使用していないため、以下のような状況が発生する可能性があります。
0. 1回目のクエリ実行 -> 成功
0. DB障害発生
0. 2回目のクエリ実行 -> 失敗
0. 結果、post_ipテーブルにレコードは登録されたが、postテーブルは何も変更されない

1回目と2回目のクエリはトランザクションの中で実行し、クエリの実行に失敗したらロールバックしてやる必要があります。
http://php.net/manual/ja/pdo.begintransaction.php

$stmt->setFetchMode(PDO::FETCH_ASSOC); は不要

SQL文の実行後に$stmt->fetch()を実行しない場合、$stmt->setFetchMode()を実行する必要はありません。
http://php.net/manual/ja/pdostatement.setfetchmode.php

Comments

[junkboy]

細かい点までリンク付きでご指摘いただき、ありがとうございます。
Kosuke_Shibuyaさんにご指摘いただいた箇所と合わせて、1つ1つ落とし込んでいきたいと思います。

大変参考になります。
取り急ぎ、お礼まで。

[junkboy]

・bindValue、bindParam
参照している変数の中身が変わると、別の値がバインドされる可能性があるということだったので、bindValueに統一しました。

・FETCH_ASSOC
別の処理で取得結果を配列に格納するために使用しており、それをコピペしていたので削除しました。

・レコードの存在確認/トランザクション
なるほど、'rowCount()'メソッドを使用して登録された行数を確認する方法があるのですね。

トランザクションについても、ご丁寧にありがとうございます。
ご指摘頂いた内容でソースを更新いたしましたので、お時間のあるときに意向にそった内容になっているか、ご確認いただけますと幸いです。