Yii Framework Forum: Issues With Fixed Bug 835 (Onendrequest) - Yii Framework Forum

Jump to content

Page 1 of 1
  • You cannot start a new topic
  • You cannot reply to this topic

Issues With Fixed Bug 835 (Onendrequest) Rate Topic: ***** 1 Votes

#1 User is offline   Sarke 

  • Junior Member
  • Pip
  • Yii
  • Group: Members
  • Posts: 60
  • Joined: 25-October 10

Posted 18 September 2013 - 09:57 AM

https://github.com/y.../yii/issues/835

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.
0

#2 User is offline   CeBe 

  • Advanced Member
  • Yii
  • Group: Yii Dev Team
  • Posts: 478
  • Joined: 16-July 10
  • Location:Berlin. Germany

Posted 19 September 2013 - 07:17 AM

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.
0

#3 User is offline   Sarke 

  • Junior Member
  • Pip
  • Yii
  • Group: Members
  • Posts: 60
  • Joined: 25-October 10

Posted 19 September 2013 - 07:29 AM

View PostCeBe, on 19 September 2013 - 07:17 AM, said:

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

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.

View PostCeBe, on 19 September 2013 - 07:17 AM, said:

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

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

Quote

Terminates the application. This method replaces PHP's exit() function by calling onEndRequest before exiting.

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.
0

#4 User is offline   CeBe 

  • Advanced Member
  • Yii
  • Group: Yii Dev Team
  • Posts: 478
  • Joined: 16-July 10
  • Location:Berlin. Germany

Posted 19 September 2013 - 07:36 AM

View PostSarke, on 19 September 2013 - 07:29 AM, said:

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?


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.

Quote

So then now there is no difference between exit() and Yii::app()->end()?


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.
0

#5 User is offline   Sarke 

  • Junior Member
  • Pip
  • Yii
  • Group: Members
  • Posts: 60
  • Joined: 25-October 10

Posted 19 September 2013 - 08:06 AM

View PostCeBe, on 19 September 2013 - 07:36 AM, said:

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:

Quote

Terminates the application. This method replaces PHP's exit() function by calling onEndRequest before exiting.

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.
0

#6 User is offline   Sarke 

  • Junior Member
  • Pip
  • Yii
  • Group: Members
  • Posts: 60
  • Joined: 25-October 10

Posted 19 September 2013 - 08:18 AM

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

Quote

what's the difference between exit and Yii::app()->end()

Answer:
Both terminates the application. But end method calls onEndRequest event before doing the exit... In this case as in a lot of others the source code explains itself very-well

public function end($status=0, $exit=true)
{
    if($this->hasEventHandler('onEndRequest'))
        $this->onEndRequest(new CEvent($this));
    if($exit)
        exit($status);
}


Common Yii questions | Wiki | Yii PHP Framework


Quote

Info: Message routing occurs at the end of the current request cycle when the onEndRequest event is raised. To explicitly terminate the processing of the current request, call CApplication::end() instead of die() or exit(), because CApplication::end() will raise the onEndRequest event so that the messages can be properly logged.

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


View Postzaccaria, on 23 October 2012 - 02:45 AM, said:

in the onEndRequest some works are done, for example writing logs and rendering the log panel.

If you are preparing a json response maybe you want to call exit in order to block the debug panel.



View Postthiagovidal, on 30 June 2010 - 06:04 PM, said:

See if it helps:

end() method
public void end(integer $status=0)
$status integer exit status (value 0 means normal exit while other values mean abnormal exit).
Terminates the application. This method replaces PHP's exit() function by calling onEndRequest before exiting.

http://www.yiiframew...tion#end-detail



View Postredguy, on 09 May 2013 - 06:31 AM, said:

Yii::app()->end() should be used in code outside action body (event handlers, behaviors, widgets, etc), which must finish request but cannot prevent execution of rest of the action body. In such case Yii::app()->end() should be used instead of plain exit(), because Yii::app()->end() closes request gently and also calls ending handlers.



View Postzaccaria, on 28 February 2011 - 05:03 AM, said:

I too use die, there are no problems.

Yii::app()->end will render anyway the log, spoiling the json.

0

#7 User is offline   CeBe 

  • Advanced Member
  • Yii
  • Group: Yii Dev Team
  • Posts: 478
  • Joined: 16-July 10
  • Location:Berlin. Germany

Posted 19 September 2013 - 03:58 PM

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.
0

#8 User is offline   Sarke 

  • Junior Member
  • Pip
  • Yii
  • Group: Members
  • Posts: 60
  • Joined: 25-October 10

Posted 19 September 2013 - 11:58 PM

View PostCeBe, on 19 September 2013 - 03:58 PM, said:

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.
0

#9 User is offline   CeBe 

  • Advanced Member
  • Yii
  • Group: Yii Dev Team
  • Posts: 478
  • Joined: 16-July 10
  • Location:Berlin. Germany

Posted 20 September 2013 - 02:54 AM

Well, you are not the only one using yii logging ;-)
0

#10 User is offline   Sarke 

  • Junior Member
  • Pip
  • Yii
  • Group: Members
  • Posts: 60
  • Joined: 25-October 10

Posted 20 September 2013 - 08:25 AM

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.
0

#11 User is offline   Sarke 

  • Junior Member
  • Pip
  • Yii
  • Group: Members
  • Posts: 60
  • Joined: 25-October 10

Posted 20 September 2013 - 09:31 AM

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.
0

#12 User is offline   Sarke 

  • Junior Member
  • Pip
  • Yii
  • Group: Members
  • Posts: 60
  • Joined: 25-October 10

Posted 23 September 2013 - 01:24 AM

https://github.com/y...t/yii/pull/2895
0

Share this topic:


Page 1 of 1
  • You cannot start a new topic
  • You cannot reply to this topic

1 User(s) are reading this topic
0 members, 1 guests, 0 anonymous users