search

EmicoEcommerce / Magento2TweakwiseExport-archived

Magento 2 module for Tweakwise export
Other
6 stars 16 forks source link

Don't output feed to output stream directly in _toString() method but… #59

Closed Echron closed 6 years ago

Echron commented 6 years ago

Don't output feed to output stream directly in _toString() method but return the output instead (Fix 'Warning: Cannot modify header information - headers already sent')

Because in some cases Magento already sends headers, just outputting the feed to the output stream causes and "Cannot modify header" error. Plus, now the __toString() function really does what it should to, unless there was a reason to do so (maybe large outputs?)

edwinljacobs commented 6 years ago

Hello,

Could you give us some information on how the error is generated? It is not an option to write the entire feed into memory, in some (Magento 1) cases it is several GB, this is too much. During testing I could not reproduce the error you described. Could you give us some more information on how this happens?

As is I think this should be the solution (see src/Controller/Feed/Export.php) `$requestKey = $this->getRequest()->getParam('key'); $configKey = $this->config->getKey(); if ($requestKey != $configKey) { throw new NotFoundException(__('Page not found.')); }

    $response = $this->getResponse();
    if (!$response instanceof HttpResponse) {
        throw new NotFoundException(__('Page not found.'));
    }

    $response->setHeader('Content-Type', 'text/xml');
    (new FeedContent($this->export, $this->log))->__toString();`

But then again I dont know when the issue arises. Though I agree that __toString is not the appropriate name for outputting the feed.

Echron commented 6 years ago

I see how this fix can give issues with large files. I don't think a good "fits for all" solution is available within what Magento2 provides. The issues that Magento was adding headers in a edge case after this module already had some output. We wrote a plugin in our project to fix this.

edwinljacobs commented 6 years ago

Hello,

For now ill close this pull request. We did some cleanup of the Feed/Export Controller which will be released later on. If this happens more often and we have a situation where we can reproduce the issue we will either make a new issue or reopen this one.

With kind regards, Edwin Jacobs