正しいでしょうか？

質問

データベースより条件にマッチしたメールアドレスにメールを送りたく、下記のように作りました。結果としてとりあえず機能していますが、書き方がなんとなく正しくないような気がしてなりません。他にBESTな方法があればご教授ください。よろしくお願いいたします。

$sql = "select * from table where categ = '{$_POST['categ']}'";
$rs = mysql_query($sql);

while($cstm = mysql_fetch_array($rs)){

$name = $cstm['name'];
$mail = $cstm['mail'];

$fp = fopen("mail.txt", "r");
$msg = "";
while ($x = fgets($fp)) {
$x = mb_ereg_replace("%%NAME%%", $name, $x);
$msg .= $x;
}

$subject ="件名";
$header = "Content-type: text/plain;charset=\"iso-2022-jp\"";

mb_language("Japanese");

mb_send_mail($mail, $subject, mb_convert_encoding( $msg, "JIS","EUC-JP") ,"From: webmaster@aaa.com",$header);

}
mysql_close($con);

回答No.2

kotaron · ベストアンサー

はじめまして、
BESTとは程遠いと思いますが、
自分なら、という感じで書かせてもらいます。

>$sql = "select * from table where categ = '{$_POST['categ']}'";
ポストされてきたデータをDBのクエリに利用する場合は、
最低限サニタイズをしないと危険ですが、
提示されているソース以外で処理されていますか？
されてない場合は、サニタイズ処理は必要かと思います。

>while($cstm = mysql_fetch_array($rs)){
ここをループ処理される意味があるのか疑問です。
推測するに、マッチするデータが１件しかないという前提ではないのでしょうか？
仮に複数件マッチする場合、fopenで処理されている内容が無駄になる気がするのですが、
読み飛ばしの処理でもされているのですか？
それに加え、マッチするデータがない場合でも、
メールの送信処理が行われてしまう構造になっているので、
if文で判定を行い、マッチしたデータがない場合の処理が必要になると思えます。

>$fp = fopen("mail.txt", "r");
この文も判断すると、スクリプトと同階層のディレクトリにあり、
外部から読み取られる可能性が高いと判断されます。
重要な内容でないにしても、セキュリティ上、隠す癖をつけておかれると良いと思います。

>$header = "Content-type: text/plain;charset=\"iso-2022-jp\"";
>mb_convert_encoding( $msg, "JIS","EUC-JP")
ここの箇所ですが、特に問題はないと思うのですが、
スクリプトの最初の方で、インターナルエンコーディングを適切に設定しておけば、
mb_send_mailに処理を任せておくほうが、タイプミスによる誤りも少なくなると思います。

以上、気が付いた点を書かせてもらいました。

お礼

色々とアドバイス有難うございます。勉強させてもらいます

回答No.1

tarko

こんにちは。

まず、ＳＱＬに含まれている$_POST['categ']ですが、ゴミデータが入っていないかチェックしていますか？管理者のみが使う物であっても、チェックした方が良いと思います。余計なお世話だったらすいません。

あと、メールの本文テンプレートファイルを開くところですが、一つのメールアドレスに配信するたびにファイルオープンするのではなく、はじめに一回だけ配列に読み込んで、その配列を使ってメールアドレス毎に名前の置き換え処理を行った方が良いと思います。

件名の変数も、whileの外に出した方が良いでしょう。

間違っていたらすいません。

お礼

早速のご回答有難うございます。サンプルを加工しながらのPHPの習得中ですので、基本的な事が良く判ってないもので・・