getError changed - not anymore a drop-in replacement lib

[nkovacs]

Your getError returns either the exception or the error event, but that's not what callers are expecting. CErrorHandler returns a normalized array: https://github.com/yiisoft/yii/blob/1.1.14/framework/base/CErrorHandler.php#L189 https://github.com/yiisoft/yii/blob/1.1.14/framework/base/CErrorHandler.php#L273

For example, the Yii webApp template uses Yii::app()->errorHandler->error and expects it to have a code and a message, but code will be missing in case of an Exception with yii-whoops: https://github.com/yiisoft/yii/blob/master/framework/cli/views/webapp/protected/controllers/SiteController.php#L40

[igorsantos07]

I decided to leave it that way so the handler wouldn't have the trouble to re-normalize everything and copy code from CErrorHandler. There's a lot of code going on to normalize that, and I think having the two different objects would be enough, as you can decide by yourself what to do with them in your errorAction.

Take a look at the sample errorAction I've made and tell me what you think: would it be really better to copy stuff into and re-normalize all the errors - and then having to re-copy from CErrorHandler whenever it's updated - or simply go for a different API and leave up to the developer what to do with the two error objects?

I would vote for explicitly documenting in this repository about the different API - something I should have done since day 1, actually.

[nkovacs]

The problem is that your extension is no longer a drop-in replacement for Yii's CErrorHandler. I just wanted a nicer exception page instead of CErrorHandler's default, but with the same behavior.

[igorsantos07]

Hmmm I got your idea.

I've already added to the README a section explaining what changed from the original CErrorHandler behavior to this new implementation.

What if we add a small example of a new errorAction to the readme as well, so everyone looking for a drop-in replacement can just change that and be happy?

Something like:

public function actionError() {
    if($error = Yii::app()->errorHandler->error) {
        $code = ($error instanceof Exception)? $error->getCode() : 500;
        if(Yii::app()->request->isAjaxRequest)
            echo "[$code] ".$error->getMessage();
        else
            $this->render('error', ['message' => $message, 'code' => $code]);
    }
}

<?php
$this->pageTitle = Yii::app()->name . ' - Error';
$this->breadcrumbs=array('Error');
?>
<h2>Error <?=$code?></h2>
<div class="error"><?=CHtml::encode($message)?></div>

[nkovacs]

Why? CErrorHandler already abstracts away the details and gives you a code and a message in a consistent form. You're forcing everyone who uses your extension to fix the API that you broke using this workaround. Your problem is that you need the exception object because in your use case you want to do some special processing. That's fine, but it shouldn't be in this extension. You could just extend CErrorHandler or WhoopsErrorHandler (depending on whether you're in development or production), save the event/exception in handleError/handleException, call the parent implementation, and add a getProblem method so that you can get the event/exception inside your errorAction.

[igorsantos07]

I've not broken any API, because there IS NO API. The normalization is done inside each method, the handleError and handleException. The only way to "maintain" this said API is copying it. If CErrorException would normalize this in a higher level, like another method, that would suffice and we could use it, but that's not what happens.

My project's implementation does not matter here, not at all. What I do could be done using the original $error array.

The problem is that I don't want to turn WhoopsErrorHandler in a direct copy of CErrorHandler. If they update the CErrorHandler API we WILL be forced (by our users) to update this extension as well, or else it will "not be a drop-in replacement" anymore. Doing this break since day one is less painful in the long road.

[nkovacs]

Yes, there is an API. It's documented here: http://www.yiiframework.com/doc/api/1.1/CErrorHandler#getError-detail Besides, since Yii2 is almost out, this is unlikely to change much.

You don't have to copy anything, just call the parent method.

[igorsantos07]

That' s not a data normalization method; It only returns what have been done before. You can't call getError() if you haven't yet run any handler method.

If you read the entire class you'll soon discover the normalization is done differently by handleError and handleException, exactly as I said, and then those same methods work on showing the error.

Normalization should be done in a place and error rendering in another. If it's not, I can't make it better unless I copy stuff. Got it now?

[igorsantos07]

On the other hand, I've got another issue with this: the system's error views. Calling parent::render() is tricky because it depends at the same time in the errorAction and the framework error views.

I'll have to decide soon (monday? it's friday night here in the office now) if I'll completely diverge from CErrorHandler behaviour or really copy code in and only include additional behaviour by hand. I may also have a custom, slightly modified version of CErrorHandler (implementing those refactoring ideas) and then extend from it and implement Whoops + additional handlers on it. I think this would work better.

[igorsantos07]

Another idea to consider: find a way to save the original exception before it's normalized into $_error, and then we can use that if we overwrite CErrorHandler to run Whoops instead of looking for exception views.

This could be easily done inside handleException, but I can't think about a way to do this from handleError from the top of my head (without digging around the code).

[igorsantos07]

Suggested approach:

• split the current errorAction behaviour into a configurable preHandler (completely optional) + the standard behaviour of errorAction
• overwrite handleException() so it saves in a different property the received exception
• overwrite render('exception') so it calls Whoops using the new exception property if it exists, if not calls Whoops error handler with getError() information

Looks pretty solid. What do you think?

But yet another issue: with this implementation the errorAction would get higher priority over Whoops pages, I think. Something work on.

[nkovacs]

Yes, getError isn't where the normalization happens, but the fact remains that existing code expects getError to return an array with a certain structure.

The way I see it, all this extension is supposed to do is replace render('exception') with Whoops. Unfortunately, CErrorHandler isn't designed that well, so it's hard to extend. My approach was to duplicate the condition that leads to render('exception') in both handlers and call the parent method. That was the simplest, least intrusive and least likely to break solution I could think of. I think your solution is too complicated, but at least it won't break existing code, so whatever. For example, why would you need preHandler at all?

And yes, errorAction should get higher priority in certain cases (CHttpException and YII_DEBUG === false). That's a feature, not a bug.

[tance77]

8 years later and still using yii 1.1 💀 however someone fixed it so you can drag and drop.

https://github.com/tianye/yii-whoops

8 years late here :) Well I guess 4 years ago when the commit was.