_exportIterateCollection performance optimization #1467
Conversation
|
|
||
| foreach ($collection as $item) { | ||
| call_user_func_array(array($this, $callback), array_merge(array($item), $args)); | ||
| } | ||
| $collection->clear(); | ||
| unset($collection); | ||
| } | ||
|
|
||
| return $this; |
There was a problem hiding this comment.
I added this return to match the phpdoc
There was a problem hiding this comment.
I'd go the other way with it. Make the PhpDoc match what's actually happening.
There's no good reason why anyone should use this as a fluid interface. Too much return $this in Magento
There was a problem hiding this comment.
Turned your note into code.
|
|
||
| $count = $collection->count(); | ||
|
|
||
| if ($count < $this->_exportPageSize) { |
There was a problem hiding this comment.
I didn't test this. But assuming $originalCollection has 1001 items, given that protected $_exportPageSize = 1000;, it will require 2 iterations. On the second iteration, $count will be 1, but it won't get exported since it'll break here. So, we need $break = true; instead of break;. Or am I missing something.
There was a problem hiding this comment.
If I am right, $count can be init to $this->_exportPageSize:
$originalCollection = $this->getCollection();
$count = $this->_exportPageSize;
//...
while ($count != $this->_exportPageSize) {
//...
$count = $collection->getSize(); // I prefer the standard method.
// we can skip `if ($count < $this->_exportPageSize) {` and continue to export
}There was a problem hiding this comment.
Your thoughts seems right. My mistake. I swear I tested it, we are using the patched version in our Magento instance. I will test it again.
There was a problem hiding this comment.
Ok I figured it out. There is two methods getCsv and getCsvFile. Only getCsvFile is using _exportIterateCollection. But in most grids, the getCsv is used, so when I was testing my code, I actually run the other method. The right place to test it is tax rates grid:
There was a problem hiding this comment.
It is fixed now and tested.
The infinite loop happens when you try to export collection that has number of items divisible by 1000(`$this->_exportPageSize`). In this case the Varien collection will reset the page number to `lastPageNumber` when `$page` is higher. `$page` is higher than `lastPageNumber` because the current logic assume that there is always one more page if the current page count is the same as page export size which is not true in case when count is divisible by page size.
|
regression fix: #1888 |
Description (*)
In my opinion, there is no reason to export like:
Instead of it, we can skip the step 1. and load the data immediately.
Related Pull Requests
Manual testing scenarios (*)
getCsvFilemethod, e.g. tax rates gridContribution checklist (*)