Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Add method to prevent sending of response after action completes #37

Closed
wants to merge 1 commit into from

3 participants

@colinmollenhour

Magento needs some way for a controller action to output and then signal the router to not attempt to send headers. There are cases like the wysiwyg thumbnails that generate "HEADERS ALREADY SENT" errors in the logs because the headers were sent by dynamic image output. I manually added an "exit;" inside the controllers to prevent this in production but there should be a more graceful solution since "exit;" prevents the use of event observers that are dispatched after the controller action. For example:

$this->getResponse()->disableOutput();
@colinmollenhour colinmollenhour Add disableOutput method, use disableOutput method when images are ou…
…tput in controller actions to prevent headers already sent errors in logs.
ac48660
@stalniy

I think there will be useful to create StopDispatchingException (throw it intstead of calling exit function) and NotFoundException (throw it when you want to forward user to page404)

@colinmollenhour

@stalniy there is already a method for 404 pages:

$this->_forward('noroute');
return;

As for the StopDispatchingException, you didn't explain what the advantage of using exceptions is exactly in this case.. Typically exceptions are used to indicate errors, but the uses for disabling the output don't necessarily involve errors so I think disableOutput() is just as good. A programmer might be thinking.. "Ok, I've rendered the image, now I need to disable the default response output.". The proposed method clearly fits this need whereas I would argue a "StopDispatchingException" is counter-intuitive and vague. E.g., what happens when you throw that exception, does it output any http status codes or headers? Will an error be logged somewhere? Will the exception even be caught and handled? Will any further events still be dispatched? Also, how would the programmer know of the existence of this exception? By using a method of the response object, the method can easily be found by the IDE or by looking at the code for the response class, but exceptions with special names for special purposes are arguably not easy to discover.

@stalniy

I knew about $this->_forward. It had better use exception instead action method. Because, for example, in production mode system can forward user to 404 page and on dev mode can show some debug information (exception back stack, etc.). What will you do if you forwarding logic is wrong? With exception in dev mode you can check back trace and be sure everything works in a right way.

Sometimes you need to stop program execution. For this purpose in Magento everybody uses php function "exit". It's not a good way. So, StopDispatchingException is thrown when you want to stop action flow. And it's obvious it will have catched outside action.

Typically exceptions are used to indicate errors

It's not exactly. Exceptions are used to indicate special places in program, not just errors!

@colinmollenhour

I still disagree, but feel free to open your own pull request that adds exceptions. :)

@stalniy

Symfony framework are implemented in such way. My proposition is just another way of implementation the issue that was discribed by you. So i think it's not necessary to create anothe issue.

@colinmollenhour

Magento is not Symfony. Magento uses exceptions almost exclusively for error handling (I can't even think of an example where exceptions are used in a non-error context) so it makes sense to stay consistent with the rest of the framework. If they change Magento 2 to use exceptions for all sorts of status passing schemes (@magento, PLEASE DON'T!) then your proposal would be appropriate, but they haven't.

Please feel free to continue pushing your opinion by creating your own pull request. An issue is not a pull request. A pull request is: fork magento2, write code, make commits, create pull request. An issue is: jot down a poorly organized thought and click submit. It is frankly a waste of my time to continue discussing this because your proposal lacks a real implementation (e.g. pull request), so I won't be discussing further. Sorry to be so blunt.

@stalniy

@colinmollenhour
ok=) Do you know why your suggestion is good and why it is bad?
Good:

  • easy to implement
  • easy to use
  • easy to understand
  • flyweight way for implementation but not extendable

Bad:

  • not universal, if you want to add some another behavior you will add another flag to Response class, then you class will have a lot of flags and methods (that do some work).
  • not logical. Response object must give some result to user, so it's not logically to have method like disableOutput, because you return output to user but with help of another function/method!

ok then about my suggestion.
Good:

  • OOP way
  • tested way by another framework
  • if you need to add some another behavior you will create another exception class and add catch block
  • logical way to use Exceptions (Exception != Error). Code of response class is clear

Bad:

  • hard to implement
  • need to create exception class for each special behavior

I understand you but it's a quite stupid to write something like "PLEASE DONT DO THIS".
I'm just trying to be objective maybe i'm wrong and it's ok.

@colinmollenhour

I just wanted to make it clear that I was in no way advocating that Magento start using exception classes for status passing. A framework needs to be cohesive and establish it's own conventions and abide by them. Using special-case exceptions is not one of Magento's established conventions (for good reason), so it doesn't really matter which approach is "better", it matters which approach best fits the Magento "way". Creating an exception class named "StopDispatchingException" is the Symfony way, it is not the Magneto way. We agree on this, don't we?

@mage2-team
Collaborator

@colinmollenhour
@stalniy
We deem that both proposed solutions attempt to cure symptoms, rather than the root cause.

In Magento framework, the class responsible for sending output of a controller is the front controller. Controllers, not to mention models or ther application layers, should not send any output directly.
We must admit that the cases where there is output sent directly and/or program is terminated are inconsistent with our own approach.

So we have added to the tasks backlog refactoring of such places across the board. The particular case with images in WYSIWYG has been fixed (will be rolled out in one of next code updates updates). We are considering to disallow on framework level direct output by controllers -- ob_start() before dispatching and ob_end_clean() after dispatching, so that there would be no incentive to do direct output.

And thank you for the valuable report and open discussion. You're raising important issues.

@mage2-team mage2-team closed this
@colinmollenhour

I agree that it would be best to avoid direct output whenever possible, but in the case of dynamic file output this could introduce some doubling of required memory. For example if I have a script that generates a 100mb file and it outputs it directly, but I have to store that output in the interim, I'll be using 200mb of memory to do so. PHP's readfile() is an efficient function for streaming large files from disk to the output, but without direct output this couldn't be used so downloading a 1GB file would be impractical. I really don't think direct output should be permanently disabled. Perhaps just disabled by default to promote good practices?

@mage2-team
Collaborator

Good point. We have added a note to take it into account.

The memory issue you described, it could be solved, for example, by implementing a "lazy load" pattern: instead of a string, set an object to the response output. The class of that object could implement __toString() and so flush the file upon request.

That may require some refactoring of the HTTP-response class.

@colinmollenhour

Nice, I hadn't thought of the __toString magic method for this purpose. That might be a viable solution, but I think some image libraries also set headers at the time of rendering such as the content-type or encoding perhaps. Just something to keep in mind for the final design. Thanks!

@mage2-team mage2-team referenced this pull request from a commit
@mage2-team mage2-team Update as of 7/19/2012
* Implemented inheritance of locales. Inheritance is declared in `app/locale/<locale_name>/config.xml`
* Moved declaration of modules from `app/etc/modules/<module>.xml` to `app/code/<pool>/<namespace>/<module>/config.xml`
* Implemented ability to match URLs in format `protocol://base_url/area/module/controller/action` (as opposite to only `module/controller/action`), utilized this feature in backend (admin) area
* Added product attribute set "Minimal Attributes", which consists of required system attributes only
* Improved customers import:
  * Implemented "Delete" behavior for importing customers, customer addresses and financial data
  * Implemented "Custom" behavior, which allows to specify behavior for each item directly from the imported file
* Updated performance tests:
  * Enabled Product View, Category View, Add to Cart, Quick Search and Advanced Search scenarios
  * Added ability to specify configuration parameters per scenario and refactored bootstrap of performance tests
* Implemented `mage.js` for base JavaScript initialization of the application
* Implemented new JS translation mechanism. JavaScript translations are loaded by locale code stored in cookies
* Implemented unit tests for JavaScript widgets in Visual Design Editor
* Added jQuery plugins: Cookie, Metadata, Validation, Head JS
* Fixed issues:
  * Impossible to add configurable product to the cart
  * Impossible to apply Shopping Cart Price Rule with any conditions to cart with simple and virtual product
  * Memory leak in email templates
  * Impossible to place order with Multiple Addresses using 3D Secure
  * Required product attributes are not exported
  * "Forgot Your Password" link on checkout page inactive after captcha reloading
  * Validation of "Number of Symbols" field in Captcha configuration doesn't work
  * Other small fixes
* GitHub requests:
  * [#37](#37) -- fixed particular case of "HEADERS ALREADY SENT" error in WYSIWYG thumbnail
  * [#39](#39) -- added `composer.json`
  * [#40](#40) -- fixed generation of "secret key" in backend URLs to honor `_forward` in controllers
1617a6e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 4, 2012
  1. @colinmollenhour

    Add disableOutput method, use disableOutput method when images are ou…

    colinmollenhour authored
    …tput in controller actions to prevent headers already sent errors in logs.
This page is out of date. Refresh to see the latest.
View
1  app/code/core/Mage/Adminhtml/controllers/Cms/Wysiwyg/ImagesController.php
@@ -182,6 +182,7 @@ public function onInsertAction()
*/
public function thumbnailAction()
{
+ $this->getResponse()->disableOutput();
$file = $this->getRequest()->getParam('file');
$file = Mage::helper('Mage_Cms_Helper_Wysiwyg_Images')->idDecode($file);
$thumb = $this->getStorage()->resizeOnTheFly($file);
View
1  app/code/core/Mage/Adminhtml/controllers/Cms/WysiwygController.php
@@ -40,6 +40,7 @@ class Mage_Adminhtml_Cms_WysiwygController extends Mage_Adminhtml_Controller_Act
*/
public function directiveAction()
{
+ $this->getResponse()->disableOutput();
$directive = $this->getRequest()->getParam('___directive');
$directive = Mage::helper('Mage_Core_Helper_Data')->urlDecode($directive);
$url = Mage::getModel('Mage_Core_Model_Email_Template_Filter')->filter($directive);
View
21 app/code/core/Mage/Core/Controller/Response/Http.php
@@ -40,6 +40,20 @@ class Mage_Core_Controller_Response_Http extends Zend_Controller_Response_Http
protected static $_transportObject = null;
/**
+ * @var bool
+ */
+ protected $_isOutputDisabled = false;
+
+ /**
+ * Disable response output; useful for situations where the response was already rendered such as
+ * with an image library that uses direct output.
+ */
+ public function disableOutput()
+ {
+ $this->_isOutputDisabled = true;
+ }
+
+ /**
* Fixes CGI only one Status header allowed bug
*
* @link http://bugs.php.net/bug.php?id=36705
@@ -77,10 +91,15 @@ public function sendHeaders()
return parent::sendHeaders();
}
+ /**
+ * Dispatch an event before the response is rendered and only render response if it is not disabled.
+ */
public function sendResponse()
{
Mage::dispatchEvent('http_response_send_before', array('response'=>$this));
- return parent::sendResponse();
+ if ( ! $this->_isOutputDisabled) {
+ return parent::sendResponse();
+ }
}
/**
Something went wrong with that request. Please try again.