第1回　HTMLとPHPの見事なハーモニー（診断編）

こんにちは。新原です。PHPプログラミング診断室はじまりました。巷に溢れる病めるPHPコードを診断していきたいと思います。

PHPのコードと聞くとどういったイメージを想像されるでしょう？昔からPHPを知っている方であれば、まずイメージするのが、HTMLとPHPが混在するコードではないでしょうか。HTMLの中にPHPが書けるのは大きなメリットでもあります。ただ、すべての処理がHTMLの中に混在すると、これはなかなか理解しづらいコードになっていきます。もしかすると、そんなコードを見て、PHPに良くないイメージを持った人がいるかもしれません。

初めての診断は、まさにHTMLとPHPが混在するコードです。では、お入りください。

関数定義がなく、流れるようなコードの妙技

今回のPHPコードは、2002年ごろに書かれたものです。とあるWebサイトで稼働していました。内容は、よくあるメールマガジンの申し込みフォームです。フォームでメールアドレスと購読するメールマガジンを入力して送信すると、DB（PostgreSQL）に保存します。元のコードでは、メール送信などの処理もあったのですが、ここでは省略しています。

では、さっそくPHPコードを見てみましょう。

<html>
<head>
    <meta http-equiv="Content-Type" content="text/html; charset=euc-jp">
<title></title>
</head>
<body>
<center>
<form method="POST" action="">
<table border=1 cellspacing=2 cellpadding=5 width=700><tr><td>

<?php
$err_idx = -1;

switch($mode) {
case "":
  print "<input type=\"hidden\" name=\"mode\" value=\"add_check\">\n";
  print "<div align=\"center\">\n";
  print "<br><br>\n";
  print "<br>\n";
  print "<table border=0 cellspacing=2 cellpadding=5>\n";
  print "<tr><td align=\"center\" colspan=2 nowrap>メールマガジン購読</td></tr>\n";
  print "<tr><td align=\"left\" colspan=2 nowrap>メールアドレスをご記入下さい。</td></tr>\n";
  print "<tr>\n";
  print "<td align=\"right\" nowrap>E-mail : </td>\n";
  print "<td align=\"left\" nowrap><input type=\"text\" name=\"frm_email\" size=30 maxlength=100 value=\"$frm_email\"></td>\n";
  print "</tr>\n";
  print "</table>\n";
  print "</div>\n";

  $cn = pg_connect('user=vagrant dbname=app');
  if ( $cn == false ) { $err_idx++; $err_value[$err_idx] = "system error"; break; }
  $sql = "select * from magazines;";
  $rec_magazines = pg_exec($cn,$sql);
  if ( $rec_magazines == false ) { $err_idx++; $err_value[$err_idx] = "system error"; pg_close($cn); break; }

  $mailm_cnt = pg_numrows($rec_magazines);
  if ( $mailm_cnt == 0 ) {
    $err_idx++; $err_value[$err_idx] = "購読できるメールマガジンがありません";
  } else {
    if ( $mailm_cnt != 1 ) {
      print "<div>\n";
      print "<hr>\n";
      print "購読するマガジンにチェック後、購読ボタンをクリックしてください。\n";
      print "</div>\n";
      print "<table border=0 cellspacing=2 cellpadding=5>\n";

      for ( $idx = 0;$idx < $mailm_cnt;$idx++ ) {
        $id = pg_result($rec_magazines,$idx,id);
        $name = pg_result($rec_magazines,$idx,name);

        print "<tr><td align=\"left\" valign=\"top\">\n";
        print "<input type=\"checkbox\" name=\"frm_id[$idx]\" value=$id>\n";
        print "<b>$name</b><input type=\"hidden\" name=\"frm_name[$idx]\" value=\"$name\">\n";
        print "</td></tr>\n";
      }

      print "</table>\n";
    } else {
      $id = pg_result($rec_magazines,0,id);
      $name = pg_result($rec_magazines,$idx,name);

      print "<input type=\"hidden\" name=\"frm_id[0]\" value=$id>\n";
      print "<input type=\"hidden\" name=\"frm_name[0]\" value=\"$name\">\n";
    }
    print "<input type=\"hidden\" name=\"frm_mailmg_cnt\" value=$mailm_cnt>\n";
    print "<div align=\"center\">\n";
    print "<br>\n";
    print "<input type=\"submit\" value=\"購読\">\n";
    print "</div>\n";
    print "<div align=\"center\">\n";
    print "<div class=\"mainlink\">\n";
    print "<br><br>\n";
    print "<br><br>\n";
    print "</div>\n";
    print "</div>\n";
  }
  pg_freeresult($rec_magazines);
  pg_close($cn);

  break;

case "add_check":
  if ( trim($frm_email) == "" ) {
    $err_idx++; $err_value[$err_idx] = "メールアドレスを入力して下さい！";
  }
  elseif ( eregi("^[_]{1}|^[\.]{1}|^[\-]{1}",$frm_email) == True ) {
    $err_idx++; $err_value[$err_idx] = "メールアドレス記入に誤りがあります！";
  }
  elseif ( eregi("(^[0-9A-Za-z_\.\-]+[@]{1})([^_\.\-]{1})([0-9A-Za-z_\.\-]+)([^_\.\-]{1})$",$frm_email) == false ) {
    $err_idx++; $err_value[$err_idx] = "メールアドレス記入に誤りがあります！";
  }
  else {
    $cn = pg_connect('user=vagrant dbname=app');
    if ( $cn == false ) { $err_idx++; $err_value[$err_idx] = "system error"; break; }
    $sql = "select * from members where lower(trim(email)) = '" . strtolower(chop($frm_email)) . "';";
    $rec_members = pg_exec($cn,$sql);
    if ( $rec_members == false ) { $err_idx++; $err_value[$err_idx] = "system error"; pg_close($cn); break; }
    $members_cnt = pg_numrows($rec_members);
    pg_freeresult($rec_members);
    pg_close($cn);
    if ( $members_cnt != 0 ) {
      $err_idx++; $err_value[$err_idx] = "すでにメールマガジン購読受付が完了しています！<br>" . "[ " . $frm_email . " ]";
      break;
    }
  }

  $ok_flg = 0;
  print "<input type=\"hidden\" name=\"frm_mailmg_cnt\" value=$frm_mailmg_cnt>\n";
  if ( $frm_mailmg_cnt > 1 ) {
    for ( $idx = 0;$idx < $frm_mailmg_cnt;$idx++ ) {
      if ( $frm_id[$idx] ) { $ok_flg = 1; break; }
    }
  } else {
    $ok_flg = 1;
  }
  if ( $ok_flg == 0 ) { $err_idx++; $err_value[$err_idx] = "購読希望するマガジンへチェックをお願いします。"; }

  if ( $err_idx != -1 ) { break; }

  $mode = "add";

  break;
}

switch($mode) {
case "add":
  $cn = pg_connect('user=vagrant dbname=app');
  if ( $cn == false ) { $err_idx++; $err_value[$err_idx] = "system error"; break; }

  $sql = "select * from members order by id desc;";
  $rec_members = pg_exec($cn,$sql);
  if ( $rec_members == false ) { $err_idx++; $err_value[$err_idx] = "system error"; pg_close($cn); break; }
  $members_cnt = pg_numrows($rec_members);
  $gid = 0;
  if ( $members_cnt != 0 ) { $gid = pg_result($rec_members,0,id); }
  $gid++;

  $gentymd = date("Y/m/d",time());
  $gemail = ereg_replace("\t","",$frm_email);

  $sql = "BEGIN TRANSACTION;";
  $result = pg_exec($cn,$sql);
  if ( $result == false ) { $err_idx++; $err_value[$err_idx] = "system error"; pg_close($cn); break; }

  $ng_flg = 0;

  $sql = "insert into members values('".$gid."','".chop($gemail)."','".$gentymd."');";
  $rd = pg_exec($cn,$sql);
  if ( $rd == false ) { $err_idx++; $err_value[$err_idx] = "system error"; $ng_flg = 1; break; }

  if ( $frm_mailmg_cnt > 1 ) {
    for ( $idx = 0;$idx < $frm_mailmg_cnt;$idx++ ) {
      if ( !$frm_id[$idx] ) {
      } else {
        $sql = "insert into member_magazines values($gid,$frm_id[$idx]);";
        $rec_member_magazines = pg_exec($cn,$sql);
        if ( $rec_member_magazines == false ) { $err_idx++; $err_value[$err_idx] = "system error"; $ng_flg = 1; break 2; }
      }
    }
  } else {
    $sql = "insert into member_magazines values($gid,$frm_id[0]);";
    $rec_member_magazines = pg_exec($cn,$sql);
    if ( $rec_member_magazines == false ) { $err_idx++; $err_value[$err_idx] = "system error"; $ng_flg = 1; break; }
  }

  if ( $ng_flg == 1 ) {
    $sql = "ROLLBACK TRANSACTION;";
    $result = pg_exec($cn,$sql);
    if ( $result == false ) { $err_idx++; $err_value[$err_idx] = "system error"; pg_close($cn); }
    break;
  } else {
    $sql = "COMMIT TRANSACTION;";
    $result = pg_exec($cn,$sql);
    if ( $result == false ) { $err_idx++; $err_value[$err_idx] = "system error"; pg_close($cn); break; }
  }

  pg_freeresult($result);
  pg_freeresult($rec_members);
  pg_freeresult($rec_member_magazines);
  pg_close($cn);

  print "<div align=\"center\">\n";
  print "<table border=0 cellspacing=2 cellpadding=2>\n";
  print "<tr><td colspan=3 align=\"center\" nowrap> メールマガジン購読完了</td></tr>\n";
  print "<tr><td colspan=3 align=\"center\">\n";
  print "登録が完了しました。";
  print "</td></tr>\n";
  print "</table>\n";
  print "</div>\n";

  break;
}

if ( $err_idx != -1 ) {
  print "<br>\n";
  print "<div align=\"center\">\n";
  print "<table border=0 cellspacing=2 cellpadding=5>\n";
  print "<tr><td><font color=\"#ff0000\">\n";
  for ($idx=0;$idx<=$err_idx;$idx++) {
    print "・$err_value[$idx]<br>\n";
  }
  print "</font></td></tr>\n";
  print "<tr><td align=\"center\">\n";
  print "<input type=\"button\" value=\"戻 る\" onclick=\"history.back()\">\n";
  print "</td></tr>\n";
  print "</table>\n";
  print "</div>\n";
}
?>
</td></tr></table>
</form>
</center>
</body>
</html>

診断

1. ロジックとビューの混在

このコードを見て、まず気になるのが、HTMLの中に多くのPHPコードが含まれている点です。

先頭の<html>タグからHTMLが開始しているのですが、9行目の<table><tr><td>タグ以下から、210行目の</td></tr></table>タグまでの200行がPHPコードになります。このPHPコードでは、状態ごとの処理分岐、入力値のチェック、データベースへのアクセスなど多くの処理を行っています。すべての処理はこのコードの中で実行されています。また、PHPコードの中ではprint文によるHTMLの出力処理も行われています。

このように、PHPで行う処理（ロジック）とHTMLの出力部分（ビュー）が混在しているコードは、処理の流れが読みづらくなります。特にこのコードは、状態に応じて処理の流れが変化しており、その状態ごとに出力されるHTMLも変わります。このコードを一見しただけで、実際にどのようなHTMLが出力されるかをイメージするのは難しいでしょう。

ビューを動的に出力するためにHTMLの中にPHPコードが混在するのは、問題のない使い方です。ですが、このコードのようにロジックとビューが混在するコードでは、ロジックの流れが追いづらく、出力されるビューも把握しづらいものになります。一般的に、ロジックは状態に応じて処理の流れが変わるので複雑になりがちです。それとビューが混在すると、いつどのようなHTMLが出力されるか、どのような画面になるのかというのが、実際に動かしてみるまでわからないということになります。

理解しづらいコードは、不具合に気づきづらくなり、またその修正や変更も難しいものになります。

まずは、ロジックとビューを明確に分ける。そして、ロジックは先に実行しておいて、ビューではHTML出力に関連する処理のみを書く、というように分離すると役割と流れが明確になり、理解しやすいコードになるでしょう。

2. print文によるHTML出力

PHPコードの中では、HTMLがprint文にて出力されています。print文での出力自体が悪いわけではないのですが、HTMLをPHPの文字列として記述しているので、読みづらくなっています。エディタによっては、文字列についてはHTMLと認識せずにコードハイライトなどが効かない場合もあります。HTMLを記述する際も、出力個所をダブルクォーテーションで囲む、HTMLの属性値を囲んでいるダブルクォーテーションをエスケープする、改行コードを入れるなど手間がかかります。

下記が実際にコードの中でprint文でHTMLを出力している個所の一つです。

  print "<tr><td><font color=\"#ff0000\">\n";
  for ($idx=0;$idx<=$err_idx;$idx++) {
    print "・$err_value[$idx]<br>\n";
  }
  print "</font></td></tr>\n";

このコードを、PHP開始タグ、終了タグを使い、HTML部分をPHPコードから出して書き直したのが以下です。どちらが読みやすいかは一目瞭然でしょう。

  <tr><td><font color=#ff0000>
  <?php for ($idx=0;$idx<=$err_idx;$idx++): ?>
    <?php echo htmlspecialchars($err_value[$idx]) ?>;
  <?php endfor; ?>
  </font></td></tr>

HTMLの中でPHPを書く場合、PHP開始タグ、終了タグをうまく組み合わせることで、HTMLタグのように見やすく書けるのがPHPの利点です。ここは素直に、HTMLタグは、PHPの文字列ではなく、HTMLとして記述するようにしましょう。

3. 処理の流れが追いづらい

PHPコード全体の流れを見ていきます。今回のコードは大きく分けて14行目から123行目のswitch文と、125行目から192行目にあるもう一つのswitch文に分けられます。どちらも同じ$mode変数の値で分岐を行っているのですが、なぜかswitch文が2つに分けられています。実は1つ目のswitch文の中で$mode変数の値を書き換えており、この内容によって2つ目のswitch文による処理を実行するか否かが決定されます。

1つ目のswitch文では、$mode変数が空の場合とadd_checkの場合で処理を分岐しています。前者はフォームを表示する初期状態、後者はフォームから値が送信されてきた状態の処理です。add_checkの場合、送信されてきた値のチェックを行い、値が正常であれば$modeの値をaddに変更します。そして2つ目のswitch文では、$mode変数がaddの場合のみ登録処理を行う、という流れです。

単純に考えれば、add_checkの処理で入力値検査が正常に終了したあとにそのまま登録処理を続ければよいと思うのですが、なぜかswitch文を分けるという構造になっています。なぜこうなったかの理由はわかりませんが、あとでコードを追う身としてはわかりにくいことこのうえないです。

この2つのswitch文による条件分岐は、単にわかりにくいだけではありません。$mode変数の値はフォームから送信されてくることが想定されているので、任意に変更することが可能です。つまり、もし$modeの値をaddに改ざんして送信すると、1つ目のswitch文を飛ばして、いきなり2つ目のswitch文にて登録処理を実行できてしまいます。こうなると、せっかくの入力値検査をスキップしてしまうので、任意の値を登録されてしまう可能性があります。これは良くないですね。

改善策としては、まずは全体の流れがわかりやすいように素直にコードを書きましょうということです。このコードで必要な状態は「フォームを表示する（初期状態⁠）⁠」と「フォームから値が送信される（送信された値を処理する状態⁠）⁠」の2つしかないわけです。そうであれば、まずは全体をその2つに分ける。そしてそれぞれの処理を行うコードを書くというのが見通しも良く、理解しやすいコードになります。

4. 外部から送信されてきた値の参照

前項の$mode変数の値は、外部から送信されると書きました。このコードを見て、なぜそう動くのかがわかる人はPHP歴がなかなか長い方です。このコードでは、$_GETや$_POST、$_REQUESTなどHTTPリクエストで送られてくる値を参照するスーパーグローバル変数が見当たりません。では、どうやって$mode変数に送信されてきた値が入るのでしょうか。実は、以前のPHPにはregister_globalsという機能があり、これを有効にすると外部から送信されてきた値をPHPの変数値として参照することが可能となっていました。

つまり、クエリストリングやフォームから送信された値にmodeというパラメータがあると、$mode変数にその値が格納されるという具合です。

一見便利なようにも見えるのですが、システム外部から任意の値をセットできてしまうので、セキュリティ上問題になることが多くありました。そこで、スーパーグローバル変数の登場以後は、この機能は非推奨となり、PHP 5.4では完全に廃止されました。

また、外部から送信される値は、いかようにも改ざんすることが可能です。外部から送信される値を元に処理を分岐させること自体は問題ないのですが、その場合は、どのような値が送信されてきても動作に支障がないように注意する必要があります。

5. 同じ処理を実直に書き続けている

このコードは実に実直です。まじめにこつこつと書かれています。まったく同じコードが何度も何度も丁寧に書かれています。同じことを同じように寸分違わず、こつこつと積み重ねていく。人生訓として大切に思える言葉ですが、コードにおいてはこういった誠実さは不要です。できるだけ、ずぼらになって同じコードは書かない、繰り返さないということが必要です。

たとえば、このコードではpg_connect()関数が何度も書かれています。しかもDBに接続するための情報も引数に何度も書かれています。同じ作業に嫌気をささずに、実直にしたためられているのがわかります。ただ、これはコードを変更する際に大きな足枷となります。もしDBの接続先が変わったら、あちらこちらのコードを直す必要があります。困りましたね。あと、何より書くのが面倒です。同じ処理を何度も書くのは本当に無駄です。面倒、無駄と書くと怠惰なように思えるかもしれませんが、むしろコードを書くうえでの怠惰は奨励されるべきことです。

では、どうすればよいか。簡単です。同じ処理は関数にまとめてしまえばよいのです。

pg_connect()関数であれば、下記のようにconnect_db()などの関数を定義して、実際にDBに接続する際はこの関数を呼びます。こうすれば、いざ接続先を変更したいとなってもconnect_db()だけを修正すればよいのです。

function connect_db() {
        return pg_connect('user=vagrant dbname=app');
}

さらに接続情報などの設定値は環境に応じて変わるものなので、コードの中に直接書くのではなく、コード上部などに定数などで定義しておきます。こうすれば設定を変更したいときは、コードを読まなくても設定値だけを変更すればよくなります。

前項でロジックとビューを分離したように、内容や変更頻度に応じて、コードを分離すると、変更しやすく、理解しやすいコードになります。

// コード上部で設定値を定数定義
// DBの接続先が変わっても、ここだけ変更すればよい
define('DB_USER', 'vagrant');
define('DB_NAME', 'app');

(snip)

// 関数では定数を参照
function connect_db() {
        $dsn = sprintf('user=%s dbname=%s', DB_USER, DB_NAME);
        return pg_connect($dsn);
}

治療すべきポイントを洗い出したところで、次回はいよいよこれらの問題点を治療していきます。