Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prevent HEADERS ALREADY SENT error #23

Merged
merged 3 commits into from
Oct 25, 2016

Conversation

Flyingmana
Copy link
Contributor

$image->display() directly outputs the image together with the mimeType header,
causing the Mage_Core_Controller_Varien_Front->dispatch() to log an
Headers already sent error on sending the actual Response object

The error occurs if magento tries to display an image in the cms wysiwyg editor mode.
The error occurs only in log, it seems to not affect the output of the image.

$image->display() directly outputs the image together with the mimeType header,
causing the Mage_Core_Controller_Varien_Front->dispatch() to log an 
Headers already sent error on sending the actual Response object
@Flyingmana Flyingmana added the review needed Problem should be verified label Jun 19, 2015
@tmotyl
Copy link
Contributor

tmotyl commented Oct 27, 2015

How can I test it?

@@ -62,5 +63,8 @@ public function directiveAction()
imagedestroy($image);
*/
}
$this->getResponse()->setHeader('Content-type', $image->getMimeType(), true);
$this->getResponse()->setBody(ob_get_contents());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use ob_get_clean() here and drop the next line?

@LeeSaferite
Copy link
Contributor

I've reviewed this PR and will merge it if you make the suggested change or explain why it's better to leave it as you originally wrote it.

@drobinson
Copy link
Collaborator

@Flyingmana have you seen this review?

@Flyingmana
Copy link
Contributor Author

sorry for letting you wait, have made the change

@LeeSaferite
Copy link
Contributor

@Flyingmana I know this PR has been in the queue for a VERY long time. I finally had time to review it again this morning and I only have one request. Could you extend the fix to \Mage_Adminhtml_Cms_Wysiwyg_ImagesController::thumbnailAction as well?

@Flyingmana
Copy link
Contributor Author

next try :)
have applied it in \Mage_Adminhtml_Cms_Wysiwyg_ImagesController::thumbnailAction too

@colinmollenhour colinmollenhour merged commit b754092 into OpenMage:1.9.1 Oct 25, 2016
@colinmollenhour
Copy link
Member

Agh, and therein lies the problem with using one branch per version.. This PR was against 1.9.1 and apparently there are many files which conflict between 1.9.1 and 1.9.2.2 so even though it let me merge it into 1.9.1, merging 1.9.1 into 1.9.3.0 is a huge mess...

@colinmollenhour
Copy link
Member

Nevermind, looks like this PR was already in upstream 1.9.3.0 (slightly different)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants