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

昨日の日記で、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のコードが変わろうとも、呼び出しのタイミングで問題を起こすことはありません。（構造が変わっちゃったりしたら、それは動かなくなりますが）。

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