Issues With Fixed Bug 835 (Onendrequest)

Before this fix, "onEndRequest" would be fired if the action ended properly or Yii::app()->end() was called. It would not be fired if exit(); was called.

This was useful because I echo some debug info at the end of the output in an HTML comment (render time, cache timestamp). This would break any sort of non-HTML output (files, AJAX json, etc) because it would get appended at the end. So in those cases I very easily just called exit();

However, since this bug fix, "onEndRequest" gets fired in all cases.

So, is there a better way of outputting my debug info, or a way of detecting how the script was ended in the "onEndRequest"?

Also, it should be noted very clearly in the documentation that any output in "onEndRequest" will be made even on exit/die.

Most people would think that exit() would kill the script right there, but that’s not the case, so IMO this fix is not ideal.

You can check for ajax request with Yii::app()->request->isAjaxRequest

onEndRequest is ment to be called regardsless how the request ends so the fix makes it consistent.

Yes, I realize that, but there are other times when an action renders something other than HTML or AJAX, such as a PDF or image. How would you recommend I handle this?

What about having CProfileLogRoute or CWebLogRoute on, for example? It now breaks anything non-HTML. Before I could use exit(), now I can’t. isAjaxRequest will not help there.

So then now there is no difference between exit() and Yii::app()->end()? Even the Yii documentation suggests Yii::app()->end() is different:

Reading that, I would expect exit() to NOT call onEndRequest.

IMO, there needs to be a way of exiting without CProfileLogRoute, CWebLogRoute, or onEndRequest being called.

When you use CHttpRequest::sendFile() and other yii helpers they are already handling these situations as far as I remember so there shouldn’t be any problem with them.

it should be the same, yes. In yii2 we completely removed Yii::app()->end() and use exit() instead.

It is more important to make sure logs are written in every case.

You can disable Web and Profile logroute when you do not need them in certain requests.

Well that’s a whole lot of hoopla to do something simple. I doubt I’m the only one to use exit() instead of end() to not get anymore output. Even the Yii documentation suggests Yii::app()->end() is different:

Reading that, I would expect exit() to NOT call onEndRequest.

I realize making sure the logs are written is important, but there should be a difference. Also, there’s much more to the onEndRequest than just silent logs. Anything after exit() is called should not output anything to the browser.

You have fundamentally changed how the exit() function behaves.

At least offer some form of alternative. This function, for example, would do what exit did before this fix:




function kill()

{

    ob_start();

    Yii::app()->end(0, false);

    ob_end_clean();

    exit(0);

}



However, this would involve replacing all the exit() instances used instead of end(). For us, that’s about 250 Yii sites, and a quick scan shows 766 instances…

And what about third-party libraries? When they output something and call exit or die they expect no more output. What happens when CProfileLogRoute is on? I can’t be expected to go through all the libraries and site codes because this fix changes how one of the basic PHP functions behave.

I urge you to find a better solution, one that is backwards compatible.

Examples of how this fix changes what people know and use, causing potential upgrade and backwards compatibility issues:

Common Yii questions | Wiki | Yii PHP Framework

Special Topics: Logging | The Definitive Guide to Yii | Yii PHP Framework

I’m not going to answer all of the points as I don’t have much time right now, but I understand that you are not happy with this change.

Please answer one question: how do you deal with logs that are not written to logfiles yet when calling exit() ?

without onEndRequest being called they are lost.

I only use logs for errors, and in those cases the error handler will kick in before the exit() is called.

Well, you are not the only one using yii logging :wink:

I know that, but before there was an option: Use Yii::app()->end() instead of exit() to fire onEndRequest and all the logging, etc. Now that option is no more. Or rather, the option to NOT fire onEndRequest is no more.

Ok, I think I have a solution that satisfies the original reason for the bug fix, any backwards compatibility issues, and the ones affecting myself and others (e.g. no broken JSON or files).

In CApplication, change the shutdown function from "end" to a new one called "shutdown". The shutdown method simply calls end but silences any output. No output is needed at this point anyways (or desired!) because the user aborted, the script timedout, or the exit function was called.

Like so:




public function run()

{

    if($this->hasEventHandler('onBeginRequest'))

        $this->onBeginRequest(new CEvent($this));

    register_shutdown_function(array($this,'shutdown')); // shutdown instead of end

    $this->processRequest();

    if($this->hasEventHandler('onEndRequest'))

        $this->onEndRequest(new CEvent($this));

}


public function shutdown() // either exit/die, script timeout, or user abort.  either way, don't send any more output.

{

    ob_start();

    $this->end(0,false);

    ob_end_clean();

    

    exit(0);

}



With this, any existing code works as expected, but bug #835 is still fixed.