初めてLaravelを使って本格的な開発をしました。簡単に動くし、開発も楽しかったのですが、このコードで大丈夫なのか？もっとよい書き方があるんじゃないかとずっと感じてました。
ベストプラクティスというと大げさですが、簡潔な記述で、読みやすくて、安全で、コピペのソースにしやすい コードを考えてみました。
サンプルのコントローラー
今回考えたいのは、コントローラーのコード。よくある割に複雑になりがちな、データ更新のメソッド、edit($i)とupdate($i)を取り上げてみます。
公式ドキュメントやよくあるサンプルコードだと、次のようなコードが多い気がする。
更新フォーム表示（edit）
データのキーを受け取って、データを読み込んで、更新フォームを表示します。
function edit($i) {
$entity = Entity::find($i);
return View::make('edit')->with('entity', $entity);
}
ここではEntityというモデルからデータを取得することとします。
データ更新分（update）
ポストしてきた入力をバリデートして、エラーがあればフォームを再表示、OKならデータを更新する。
function update($i) {
$validator = Validator::make(Input::all(), [...]);
if ($validator->fails()) {
return Redirect::to("entity/$i/edit")
->withErrors($validator); // <- (1)
}
$entity = Entity::find($i);
$entity->fill(Input::all());
$entity->save();
return Redirect::to('entity/'.$i)->with('entity', $entity);
}
Eloquentのfillメソッドを使うので、マスアサイメントができるように、Entityクラスの$fillableを設定してあるとします。
入力値をバリデートするコードはLaravelの公式ドキュメントでのバリデーションから適宜修正してます。
更新時のバリデーションで失敗した場合は(1)でフォーム戻します。ここで、withInput($validator)を使うと、再表示されるビュー内に$errorsという変数が自動で作成され、そこにバリデーションエラーが入ってきます。こういうところがLaravelマジックという感じです。
サンプルコードの修正
上のサンプルだと、次のような機能が足りてません。
- entityが見つからない場合、例外とか投げるべき。
- バリデーション失敗のエラーメッセージがない。
- バリデーションに失敗した場合、失敗した入力値を更新フォームに表示したい。
- Redirect::backという便利なメソッドがある。
などの点を考えて、書きなおしてみました。
class EntityController extends BaseController
{
function edit($i)
{
$entity = Entity::findOrFail($i);
$message = '修正してください';
$error = false;
if ($old = Input::old()) {
$entity->fill($old); // ←(1b)
$message = Session::get('error'); // <-(3)
$error = true;
}
return View::make('edit')
->with('entity', $entity)
->with('message', $message)
->with('error', $error);
}
function update($i)
{
$entity = Entity::findOrFail($i);
$validator = Validator::make(
Input::all(), [...]); // <- (2)
// 入力バリデーション
if ($validator->fails()) {
return Redirect::back()
->with('error', '入力を確認して下さい') // <-(3)
->withInput(Input::all()) // <-(1a)
->withErrors($validator);
}
$entity->fill(Input::all()); // <- (2)
$entity->save();
return Redirect::to('entity/'.$i)->with('entity', $entity);
}
}
以下、ポイントを解説します。
(1) バリデーション失敗した値をモデルに設定
エラーで更新フォームを再表示するときの処理です。
バリデーションに失敗して元のフォームを再表示する際に、入力した値も表示できるようにwithInputを使います。
で、その入力した値、つまり不正な値を表示するのに、(1b) $entityに設定してビューに渡してます。
これを思いつくのに一ヶ月かかったのが、これを書いた理由です。最初から思いついていれば。
最初はstdClass使って値を渡したりしましたが、表示側でエラー（Notice）が多発しました。たとえば日付を表示する場合、Eloquentだと自動でDateTimeオブジェクトに変換してくれますが、stdClassだと文字列のままです。両方に対応してたら、コードが汚くなってしまいました。
とはいえ不正な値をモデルに設定するのは落ち着かない。モデルを保存できなくさせるオプションとかがあれば安心なのだけれど…
(2) Input::all()で大丈夫？
Eloquentのモデル側で$fillableを正しく設定していれば、一応マスアサイメントなどのセキュリティ対策は十分なはず。
ただ、管理者とエンドユーザーで変更できる項目が違う場合など、気をつける必要はありそう。適宜
$fillableを変えるか、Input::only(...)などを使って、変更できる項目は必ず限定する必要があるでしょう。
(3) エラーの場合のメッセージ
入力バリデーションなどでエラーがあった場合、失敗直後にエラーメッセージをwithで設定して、フォームに戻してます。
Session::flash使わなくても出来るのに、つい最近覚えた。
$messageの他に$errorでエラーかどうかを判定してますが、このあたりの処理は場合によって変わってきそう。
その他の検討課題
PHPDocsは書いたほうがいい。
サンプルなもので。
CSRFトークンは自動で設定されるよね？
Laravelでは自動でCSRFトークンをフォームに追加してくれますが、値のチェックはしません。
コントローラーではなく、Routeでチェックするのが作法のようで、csrfというフィルターが存在します。HTTPメソッドがget以外ならCSRFをチェックする方法がありました。
EloquentのモデルをDI？
テストのことを考えると、ファサードを使わずにモデルオブジェクトをDIしてもらうことができます。
class EntityController extends BaseController {
protected $entity;
function __construct( Entity $entity ) {
$this->entity = $entity;
}
function edit($i) {
$entity = $this->entity->findOrFail($i);
....
}
}
メリットはテストしやすいこと。後はコピペしやすい、というのがあるかも。
バリデーションもDI?
バリデーション部分も、別クラス/オブジェクトとして管理して、DIする方法があります。
これのメリットは、バリデーションだけをテストしやすい（コントローラーのテストは難しいため）、そしてコードのコピペがし易い。
別クラスにしなくても、独立したメソッドにするだけでもメリットはありそうです。例えば
function validate() {
return Validation::make(Input::all(), [...]);
}
としておけば、このメソッドだけ書き換えればコードの再利用が進みそうです。