2011-09-04

CakePHPのErrorHandlerの悪い部分を直してみる

CakePHP MVC

昨日の日記で、cakeErrorの全ての画面のレイアウトを一気に変更する方法を書きました。

なんでdispatchMethodにパッチを当てると動くかというと、ErrorHandlerのコンストラクタの終わりに、必ずメソッドの呼び出し部分があって、そこで呼び出されることを実装ファイルをみてわかったから当てました。

これって、本来パッチを当てる部分ではないですよね。あまり良い方法とは思えません。単に動いているからよし、というレベルの解決策。気持ち悪いです。

そもそも、このErrorHandlerオブジェクト、コンストラクタの最後で_stop()を呼び出してプログラムの動作を止めています。

<?php

		$this->dispatchMethod($method, $messages);
		$this->_stop();

これってよろしくない。この書き方のせいでErrorHandlerはオブジェクトとしての存在意義がほとんどなくなっちゃって、単にメソッドの便利な置き場所的な扱いになっちゃってます。拡張のさせ方も限定的にしかできません。

そもそもErrorHandlerをインスタンス化している部分なんて数えるほどしかなく、というか数えてみたらObjectのcakeErrorでしかインスタンス化していません。それなら、オブジェクトの生成と利用を分離しても実装の手間が増える訳ではないです。

cake/libs/error.php

<?php

	function __construct($method, $messages) {
		$message = $this->cleanupMessages($messages);
		$this->error = compact('method', 'messages');

		static $__previousError = null;
		
		if ($__previousError != $this->error) {
			$__previousError = $this->error;
			$this->controller =& new CakeErrorController();
		} else {
			$this->controller =& new Controller();
			$this->controller->viewPath = 'errors';
		}
	}
	
	function invoke() {
		extract($this->error);
		
		if (method_exists($this->controller, 'apperror')) {
			return $this->controller->appError($method, $messages);
		}

		if (!$this->hasMethod($method)) {
			$method = 'error';
		}

		if ($method !== 'error') {
			if (Configure::read('debug') == 0) {
				$parentClass = get_parent_class($this);
				if (strtolower($parentClass) != 'errorhandler') {
					$method = 'error404';
				}
				$parentMethods = array_map('strtolower', get_class_methods($parentClass));
				if (in_array(strtolower($method), $parentMethods)) {
					$method = 'error404';
				}
				if (isset($messages[0]['code']) && $messages[0]['code'] == 500) {
					$method = 'error500';
				}
			}
		}
		$this->dispatchMethod($method, $messages);
		$this->_stop();
	}
	
	function hasMethod($method) {
		return in_array(
			strtolower($method), 
			array_map('strtolower', get_class_methods($this))
		);
	}
	
	function cleanupMessages($message) {
		App::import('Core', 'Sanitize');

		$options = array('escape' => false);
		$messages = Sanitize::clean($messages, $options);

		if (!isset($messages[0])) {
			$messages = array($messages);
		}
		return $message;
	}

呼び出し側もこれに合わせて修正。

cake/libs/object.php

<?php

	function cakeError($method, $messages = array()) {
		if (!class_exists('ErrorHandler')) {
			App::import('Core', 'Error');

			if (file_exists(APP . 'error.php')) {
				include_once (APP . 'error.php');
			} elseif (file_exists(APP . 'app_error.php')) {
				include_once (APP . 'app_error.php');
			}
		}

		if (class_exists('AppError')) {
			$error = new AppError($method, $messages);
		} else {
			$error = new ErrorHandler($method, $messages);
		}
		
		$error->invoke();
	}

ちょっとリファクタリングも入っちゃっているけど、要は作る作業と呼び出す作業を分離したというだけの話です。

これで昨日の修正はどう変わるかというと、

app_error.php

<?php

App::import('Core', 'Error');

class AppError extends ErrorHandler {
	public function __construct($method, $messages) {
		parent::__construct($method, $messages);
		$this->controller->layout = 'error';
	}
}

まっとうなオブジェクト指向のコードに変わりましたね。行数が減った訳でもないですが、何が改善されたのでしょうか。

dispatchMethod()は汎用的に使われるメソッドで、可能性としては今後別のタイミングでも呼ばれるかもしれなかったものです。それに対し、この修正では最初の一度、初期化の最中に必ず呼ばれることが保証されています。今後元のCakeのコードが変わろうとも、呼び出しのタイミングで問題を起こすことはありません。（構造が変わっちゃったりしたら、それは動かなくなりますが）。

正解があることではありませんが、オブジェクト指向的に言えばこちらの方が望ましいし、テストもしやすいコードであると言えます。

2011-09-04

CakePHP 2.0でのエラー処理

CakePHP CakePHP2

前のエントリーでCakePHPの悪いところを指摘したので、良いところも書いておきます。

CakePHPのErrorHandlerの悪い部分を直してみる - basuke の日記

CakePHP 2.0ではエラー処理は完全に書き直されてますね。ObjectがcakeErrorを受け付けてましたが、メソッドがなくなり、ランタイムのエラーはちゃんとExceptionで処理するようになってます。

https://github.com/cakephp/cakephp/blob/2.0/lib/Cake/Error/exceptions.php

403に対応するForbiddenExceptionも新設されているようです。問題を起こしたい場合は、適当なExceptionをnewしてthrowすることになるようです。

また、Exceptionは単に情報だけを保持していて、それを処理するErrorHandlerクラスと、エラー結果を画面に表示するExceptionRendererクラスに処理が渡されます。いい仕事してますね。

https://github.com/cakephp/cakephp/tree/2.0/lib/Cake/Error

RendererはちゃんとConfigureで替えられるようになってます。期待通り。

viva 2.0.

2011-09-04

AppErrorが自前のエラーメソッドが呼び出されない？ ...なんてことはないです

CakePHP

CakeErrorを調べていると、自前で定義したエラーメソッドが呼び出されない、という記事をちらほら見かけます。

cakeError(): 'error'以外のメソッド名は無視される。 - 呆備録
 CakePHPのデバッグレベル0の時に$this->cakeError('error500')が404のエラーになる - ほげほげ（仮）
cakeError用メソッド作ってもDEBUG=0だと動かない件 | 自転車で通勤しましょ♪ブログ

どうも昔はどうだったのかわかりませんが、現時点での最新版1.3.11ではもちろんそんなことはありません。

疑惑の部分、cake/libs/error.php __construct()

<?php
if ($method !== 'error') {
	if (Configure::read('debug') == 0) {
		$parentClass = get_parent_class($this);
		if (strtolower($parentClass) != 'errorhandler') {
			$method = 'error404';
		}
		$parentMethods = array_map('strtolower', get_class_methods($parentClass));
		if (in_array(strtolower($method), $parentMethods)) {
			$method = 'error404';
		}
		if (isset($messages[0]['code']) && $messages[0]['code'] == 500) {
			$method = 'error500';
		}
	}
}

読み解けば、debugレベルが0の場合で、さらに親クラスがErrorHandlerでない場合、もしくはErrorHandlerクラスに定義されているメソッドだった場合には、強制的にerror404メソッドにしてます。さらにcodeが500になっていればerror500メソッドに変更しているという、よくわからない実装です。もうちょっと書きようがあるかとは思いますが。

なんにせよ、誤解は誤解、ちゃんと解いておきましょう。そして、CakePHP 2.0への期待を高めましょうw