From 2ead3746d62de8df64c671a32762b52cc360710a Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Tue, 17 Jan 2017 12:51:37 +1300 Subject: [PATCH 01/18] Replace Map_Iterator with a generator. Generators (PHP 5.5+) make this kind of code structure much easier to build. --- src/ORM/Map.php | 49 ++++++++-- src/ORM/Map_Iterator.php | 200 --------------------------------------- 2 files changed, 42 insertions(+), 207 deletions(-) delete mode 100644 src/ORM/Map_Iterator.php diff --git a/src/ORM/Map.php b/src/ORM/Map.php index 6bcd68494..a79177f8d 100644 --- a/src/ORM/Map.php +++ b/src/ORM/Map.php @@ -260,13 +260,48 @@ class Map implements ArrayAccess, Countable, IteratorAggregate #[\ReturnTypeWillChange] public function getIterator() { - return new Map_Iterator( - $this->list->getIterator(), - $this->keyField, - $this->valueField, - $this->firstItems, - $this->lastItems - ); + $keyField = $this->keyField; + $valueField = $this->valueField; + + foreach($this->firstItems as $k => $v) { + yield $k => $v; + } + + foreach($this->list as $record) { + if(isset($this->firstItems[$record->$keyField])) { + continue; + } + if(isset($this->lastItems[$record->$keyField])) { + continue; + } + yield $this->extractValue($record, $this->keyField) => $this->extractValue($record, $this->valueField); + } + + foreach($this->lastItems as $k => $v) { + yield $k => $v; + } + } + + /** + * Extracts a value from an item in the list, where the item is either an + * object or array. + * + * @param array|object $item + * @param string $key + * @return mixed + */ + protected function extractValue($item, $key) + { + if (is_object($item)) { + if (method_exists($item, 'hasMethod') && $item->hasMethod($key)) { + return $item->{$key}(); + } + return $item->{$key}; + } else { + if (array_key_exists($key, $item)) { + return $item[$key]; + } + } } /** diff --git a/src/ORM/Map_Iterator.php b/src/ORM/Map_Iterator.php deleted file mode 100644 index d645ca07d..000000000 --- a/src/ORM/Map_Iterator.php +++ /dev/null @@ -1,200 +0,0 @@ -items = $items; - $this->keyField = $keyField; - $this->titleField = $titleField; - $this->endItemIdx = null; - - if ($firstItems) { - foreach ($firstItems as $k => $v) { - $this->firstItems[] = [$k, $v]; - $this->excludedItems[] = $k; - } - } - - if ($lastItems) { - foreach ($lastItems as $k => $v) { - $this->lastItems[] = [$k, $v]; - $this->excludedItems[] = $k; - } - } - } - - /** - * Rewind the Iterator to the first element. - * - * @return mixed - */ - #[\ReturnTypeWillChange] - public function rewind() - { - $this->firstItemIdx = 0; - $this->endItemIdx = null; - - $rewoundItem = $this->items->rewind(); - - if (isset($this->firstItems[$this->firstItemIdx])) { - return $this->firstItems[$this->firstItemIdx][1]; - } else { - if ($rewoundItem) { - return $this->extractValue($rewoundItem, $this->titleField); - } else { - if (!$this->items->valid() && $this->lastItems) { - $this->endItemIdx = 0; - - return $this->lastItems[0][1]; - } - } - } - } - - /** - * Return the current element. - * - * @return mixed - */ - #[\ReturnTypeWillChange] - public function current() - { - if (($this->endItemIdx !== null) && isset($this->lastItems[$this->endItemIdx])) { - return $this->lastItems[$this->endItemIdx][1]; - } else { - if (isset($this->firstItems[$this->firstItemIdx])) { - return $this->firstItems[$this->firstItemIdx][1]; - } - } - return $this->extractValue($this->items->current(), $this->titleField); - } - - /** - * Extracts a value from an item in the list, where the item is either an - * object or array. - * - * @param array|object $item - * @param string $key - * @return mixed - */ - protected function extractValue($item, $key) - { - if (is_object($item)) { - if (method_exists($item, 'hasMethod') && $item->hasMethod($key)) { - return $item->{$key}(); - } - return $item->{$key}; - } else { - if (array_key_exists($key, $item ?? [])) { - return $item[$key]; - } - } - } - - /** - * Return the key of the current element. - * - * @return string - */ - #[\ReturnTypeWillChange] - public function key() - { - if (($this->endItemIdx !== null) && isset($this->lastItems[$this->endItemIdx])) { - return $this->lastItems[$this->endItemIdx][0]; - } else { - if (isset($this->firstItems[$this->firstItemIdx])) { - return $this->firstItems[$this->firstItemIdx][0]; - } else { - return $this->extractValue($this->items->current(), $this->keyField); - } - } - } - - /** - * Move forward to next element. - * - * @return mixed - */ - #[\ReturnTypeWillChange] - public function next() - { - $this->firstItemIdx++; - - if (isset($this->firstItems[$this->firstItemIdx])) { - return $this->firstItems[$this->firstItemIdx][1]; - } else { - if (!isset($this->firstItems[$this->firstItemIdx - 1])) { - $this->items->next(); - } - - if ($this->excludedItems) { - while (($c = $this->items->current()) && in_array($c->{$this->keyField}, $this->excludedItems ?? [], true)) { - $this->items->next(); - } - } - } - - if (!$this->items->valid()) { - // iterator has passed the preface items, off the end of the items - // list. Track through the end items to go through to the next - if ($this->endItemIdx === null) { - $this->endItemIdx = -1; - } - - $this->endItemIdx++; - - if (isset($this->lastItems[$this->endItemIdx])) { - return $this->lastItems[$this->endItemIdx]; - } - - return false; - } - } - - /** - * Checks if current position is valid. - * - * @return boolean - */ - #[\ReturnTypeWillChange] - public function valid() - { - return ( - (isset($this->firstItems[$this->firstItemIdx])) || - (($this->endItemIdx !== null) && isset($this->lastItems[$this->endItemIdx])) || - $this->items->valid() - ); - } -} From 6c136c9cf24223748db122c817b09e6cbd0e9472 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Tue, 17 Jan 2017 12:52:14 +1300 Subject: [PATCH 02/18] NEW: Iterate ArrayList via a generator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Using a generator here means that we don’t need to prepare a duplicate array in-memory before iterating. --- src/ORM/ArrayList.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/ORM/ArrayList.php b/src/ORM/ArrayList.php index 74a7cd600..eca2da26c 100644 --- a/src/ORM/ArrayList.php +++ b/src/ORM/ArrayList.php @@ -109,13 +109,13 @@ class ArrayList extends ViewableData implements SS_List, Filterable, Sortable, L #[\ReturnTypeWillChange] public function getIterator() { - $items = array_map( - function ($item) { - return is_array($item) ? new ArrayData($item) : $item; - }, - $this->items ?? [] - ); - return new ArrayIterator($items); + foreach ($this->items as $i => $item) { + if (is_array($item)) { + yield new ArrayData($item); + } else { + yield $item; + } + } } /** From 1efe2b46ffcd524cbe0e5cbc1593be47e2de589a Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Tue, 17 Jan 2017 14:21:11 +1300 Subject: [PATCH 03/18] FIX: Fix PaginatedList::toArray() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It wasn’t respecting pagination. --- src/ORM/PaginatedList.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/ORM/PaginatedList.php b/src/ORM/PaginatedList.php index f7289e9a8..2a0d6f07d 100644 --- a/src/ORM/PaginatedList.php +++ b/src/ORM/PaginatedList.php @@ -225,6 +225,21 @@ class PaginatedList extends ListDecorator } } + /** + * @return array + */ + public function toArray() + { + $result = []; + + // Use getIterator() + foreach($this as $record) { + $result[] = $record; + } + + return $result; + } + /** * Returns a set of links to all the pages in the list. This is useful for * basic pagination. From d8735633a765d19fcd8261a99310a1093d398506 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Tue, 17 Jan 2017 15:23:30 +1300 Subject: [PATCH 04/18] =?UTF-8?q?FIX:=20Don=E2=80=99t=20call=20PaginatedLi?= =?UTF-8?q?st::getIterator()=20directly.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s best for foreach() to call this for us. --- src/ORM/PaginatedList.php | 4 ++-- tests/php/ORM/PaginatedListTest.php | 21 +++++++++------------ 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/ORM/PaginatedList.php b/src/ORM/PaginatedList.php index 2a0d6f07d..1d0554e03 100644 --- a/src/ORM/PaginatedList.php +++ b/src/ORM/PaginatedList.php @@ -359,8 +359,8 @@ class PaginatedList extends ListDecorator $num = $i + 1; $emptyRange = $num != 1 && $num != $total && ( - $num == $left - 1 || $num == $right + 1 - ); + $num == $left - 1 || $num == $right + 1 + ); if ($emptyRange) { $result->push(new ArrayData([ diff --git a/tests/php/ORM/PaginatedListTest.php b/tests/php/ORM/PaginatedListTest.php index fb63403d6..e2ade0d48 100644 --- a/tests/php/ORM/PaginatedListTest.php +++ b/tests/php/ORM/PaginatedListTest.php @@ -90,7 +90,7 @@ class PaginatedListTest extends SapphireTest $this->assertEquals(1, $list->CurrentPage()); } - public function testGetIterator() + public function testIteration() { $list = new PaginatedList( new ArrayList([ @@ -105,26 +105,23 @@ class PaginatedListTest extends SapphireTest $this->assertListEquals( [['Num' => 1], ['Num' => 2]], - ArrayList::create($list->getIterator()->getInnerIterator()->getArrayCopy()) + $list ); $list->setCurrentPage(2); $this->assertListEquals( [['Num' => 3], ['Num' => 4]], - ArrayList::create($list->getIterator()->getInnerIterator()->getArrayCopy()) + $list ); $list->setCurrentPage(3); $this->assertListEquals( [['Num' => 5]], - ArrayList::create($list->getIterator()->getInnerIterator()->getArrayCopy()) + $list ); $list->setCurrentPage(999); - $this->assertListEquals( - [], - ArrayList::create($list->getIterator()->getInnerIterator()->getArrayCopy()) - ); + $this->assertListEquals([], $list); // Test disabled paging $list->setPageLength(0); @@ -137,14 +134,14 @@ class PaginatedListTest extends SapphireTest ['Num' => 4], ['Num' => 5], ], - ArrayList::create($list->getIterator()->getInnerIterator()->getArrayCopy()) + $list ); // Test with dataobjectset $players = Player::get(); $list = new PaginatedList($players); $list->setPageLength(1); - $list->getIterator(); + $list; $this->assertEquals( 4, $list->getTotalItems(), @@ -223,10 +220,10 @@ class PaginatedListTest extends SapphireTest $list = new PaginatedList($list); $list->setCurrentPage(3); - $this->assertCount(10, $list->getIterator()->getInnerIterator()); + $this->assertEquals(10, count($list->toArray())); $list->setLimitItems(false); - $this->assertCount(50, $list->getIterator()->getInnerIterator()); + $this->assertEquals(50, count($list->toArray())); } public function testCurrentPage() From 77c7552c3fc2c53b08cc774c3734e3243b07c4e4 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Tue, 17 Jan 2017 15:20:22 +1300 Subject: [PATCH 05/18] =?UTF-8?q?NEW:=20ORM=E2=80=99=20Query=20is=20a=20ge?= =?UTF-8?q?nerator-based=20IteratorAggregate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit API: Query no longer has iterator methods current(), first(), rewind(), next() Using generators reduces the amount of boilerplate needed for this code. Turning it into an IteratorAggregate means that the iterator can be re-created for each subsequent foreach call. This means that the rewind() and seek() functionality can be discarded. --- src/ORM/Connect/DBSchemaManager.php | 2 +- src/ORM/Connect/MySQLQuery.php | 44 ++++------ src/ORM/Connect/MySQLStatement.php | 80 ++++++++---------- src/ORM/Connect/Query.php | 124 ++-------------------------- src/ORM/ManyManyList.php | 2 +- tests/php/ORM/DatabaseTest.php | 2 +- 6 files changed, 59 insertions(+), 195 deletions(-) diff --git a/src/ORM/Connect/DBSchemaManager.php b/src/ORM/Connect/DBSchemaManager.php index 7f93148cb..7a10cef8e 100644 --- a/src/ORM/Connect/DBSchemaManager.php +++ b/src/ORM/Connect/DBSchemaManager.php @@ -378,7 +378,7 @@ abstract class DBSchemaManager if ($dbID && isset($options[$dbID])) { if (preg_match('/ENGINE=([^\s]*)/', $options[$dbID] ?? '', $alteredEngineMatches)) { $alteredEngine = $alteredEngineMatches[1]; - $tableStatus = $this->query(sprintf('SHOW TABLE STATUS LIKE \'%s\'', $table))->first(); + $tableStatus = $this->query(sprintf('SHOW TABLE STATUS LIKE \'%s\'', $table))->record(); $tableOptionsChanged = ($tableStatus['Engine'] != $alteredEngine); } } diff --git a/src/ORM/Connect/MySQLQuery.php b/src/ORM/Connect/MySQLQuery.php index c81138643..e42dddee8 100644 --- a/src/ORM/Connect/MySQLQuery.php +++ b/src/ORM/Connect/MySQLQuery.php @@ -45,16 +45,24 @@ class MySQLQuery extends Query } } - public function seek($row) + public function getIterator() { + $floatTypes = [MYSQLI_TYPE_FLOAT, MYSQLI_TYPE_DOUBLE, MYSQLI_TYPE_DECIMAL, MYSQLI_TYPE_NEWDECIMAL]; if (is_object($this->handle)) { - // Fix for https://github.com/silverstripe/silverstripe-framework/issues/9097 without breaking the seek() API - $this->handle->data_seek($row); - $result = $this->nextRecord(); - $this->handle->data_seek($row); - return $result; + while ($row = $this->handle->fetch_array(MYSQLI_NUM)) { + $data = []; + foreach ($row as $i => $value) { + if (!isset($this->columns[$i])) { + throw new DatabaseException("Can't get metadata for column $i"); + } + if (in_array($this->columns[$i]->type, $floatTypes ?? [])) { + $value = (float)$value; + } + $data[$this->columns[$i]->name] = $value; + } + yield $data; + } } - return null; } public function numRecords() @@ -62,27 +70,7 @@ class MySQLQuery extends Query if (is_object($this->handle)) { return $this->handle->num_rows; } + return null; } - - public function nextRecord() - { - $floatTypes = [MYSQLI_TYPE_FLOAT, MYSQLI_TYPE_DOUBLE, MYSQLI_TYPE_DECIMAL, MYSQLI_TYPE_NEWDECIMAL]; - - if (is_object($this->handle) && ($row = $this->handle->fetch_array(MYSQLI_NUM))) { - $data = []; - foreach ($row as $i => $value) { - if (!isset($this->columns[$i])) { - throw new DatabaseException("Can't get metadata for column $i"); - } - if (in_array($this->columns[$i]->type, $floatTypes ?? [])) { - $value = (float)$value; - } - $data[$this->columns[$i]->name] = $value; - } - return $data; - } else { - return false; - } - } } diff --git a/src/ORM/Connect/MySQLStatement.php b/src/ORM/Connect/MySQLStatement.php index 7bc54765f..b46390b8a 100644 --- a/src/ORM/Connect/MySQLStatement.php +++ b/src/ORM/Connect/MySQLStatement.php @@ -56,6 +56,26 @@ class MySQLStatement extends Query */ protected $boundValues = []; + /** + * Hook the result-set given into a Query class, suitable for use by SilverStripe. + * @param mysqli_stmt $statement The related statement, if present + * @param mysqli_result $metadata The metadata for this statement + */ + public function __construct($statement, $metadata) + { + $this->statement = $statement; + $this->metadata = $metadata; + + // Immediately bind and buffer + $this->bind(); + } + + public function __destruct() + { + $this->statement->close(); + $this->currentRecord = false; + } + /** * Binds this statement to the variables */ @@ -82,58 +102,24 @@ class MySQLStatement extends Query call_user_func_array([$this->statement, 'bind_result'], $variables ?? []); } - /** - * Hook the result-set given into a Query class, suitable for use by SilverStripe. - * @param mysqli_stmt $statement The related statement, if present - * @param mysqli_result $metadata The metadata for this statement - */ - public function __construct($statement, $metadata) + public function getIterator() { - $this->statement = $statement; - $this->metadata = $metadata; - - // Immediately bind and buffer - $this->bind(); - } - - public function __destruct() - { - $this->statement->close(); - $this->currentRecord = false; - } - - public function seek($row) - { - $this->rowNum = $row - 1; - - // Fix for https://github.com/silverstripe/silverstripe-framework/issues/9097 without breaking the seek() API - $this->statement->data_seek($row); - $result = $this->next(); - $this->statement->data_seek($row); - return $result; + while($this->statement->fetch()) { + // Dereferenced row + $row = []; + foreach ($this->boundValues as $key => $value) { + $floatTypes = [MYSQLI_TYPE_FLOAT, MYSQLI_TYPE_DOUBLE, MYSQLI_TYPE_DECIMAL, MYSQLI_TYPE_NEWDECIMAL]; + if (in_array($this->types[$key], $floatTypes ?? [])) { + $value = (float)$value; + } + $row[$key] = $value; + } + yield $row; + } } public function numRecords() { return $this->statement->num_rows(); } - - public function nextRecord() - { - // Skip data if out of data - if (!$this->statement->fetch()) { - return false; - } - - // Dereferenced row - $row = []; - foreach ($this->boundValues as $key => $value) { - $floatTypes = [MYSQLI_TYPE_FLOAT, MYSQLI_TYPE_DOUBLE, MYSQLI_TYPE_DECIMAL, MYSQLI_TYPE_NEWDECIMAL]; - if (in_array($this->types[$key], $floatTypes ?? [])) { - $value = (float)$value; - } - $row[$key] = $value; - } - return $row; - } } diff --git a/src/ORM/Connect/Query.php b/src/ORM/Connect/Query.php index af9e0c3a5..1d7497ffa 100644 --- a/src/ORM/Connect/Query.php +++ b/src/ORM/Connect/Query.php @@ -27,30 +27,9 @@ use Iterator; * on providing the specific data-access methods that are required: {@link nextRecord()}, {@link numRecords()} * and {@link seek()} */ -abstract class Query implements Iterator +abstract class Query implements \IteratorAggregate { - /** - * The current record in the iterator. - * - * @var array - */ - protected $currentRecord = null; - - /** - * The number of the current row in the iterator. - * - * @var int - */ - protected $rowNum = -1; - - /** - * Flag to keep track of whether iteration has begun, to prevent unnecessary seeks - * - * @var bool - */ - protected $queryHasBegun = false; - /** * Return an array containing all the values from a specific column. If no column is set, then the first will be * returned @@ -62,7 +41,7 @@ abstract class Query implements Iterator { $result = []; - while ($record = $this->next()) { + foreach ($this as $record) { if ($column) { $result[] = $record[$column]; } else { @@ -82,6 +61,7 @@ abstract class Query implements Iterator public function keyedColumn() { $column = []; + foreach ($this as $record) { $val = $record[key($record)]; $column[$val] = $val; @@ -106,13 +86,13 @@ abstract class Query implements Iterator } /** - * Returns the next record in the iterator. + * Returns the first record in the result * * @return array */ public function record() { - return $this->next(); + return $this->getIterator()->current(); } /** @@ -122,7 +102,7 @@ abstract class Query implements Iterator */ public function value() { - $record = $this->next(); + $record = $this->record(); if ($record) { return $record[key($record)]; } @@ -164,94 +144,12 @@ abstract class Query implements Iterator return $result; } - /** - * Iterator function implementation. Rewind the iterator to the first item and return it. - * Makes use of {@link seek()} and {@link numRecords()}, takes care of the plumbing. - * - * @return void - */ - #[\ReturnTypeWillChange] - public function rewind() - { - if ($this->queryHasBegun && $this->numRecords() > 0) { - $this->queryHasBegun = false; - $this->currentRecord = null; - $this->seek(0); - } - } - - /** - * Iterator function implementation. Return the current item of the iterator. - * - * @return array - */ - #[\ReturnTypeWillChange] - public function current() - { - if (!$this->currentRecord) { - return $this->next(); - } else { - return $this->currentRecord; - } - } - - /** - * Iterator function implementation. Return the first item of this iterator. - * - * @return array - */ - public function first() - { - $this->rewind(); - return $this->current(); - } - - /** - * Iterator function implementation. Return the row number of the current item. - * - * @return int - */ - #[\ReturnTypeWillChange] - public function key() - { - return $this->rowNum; - } - - /** - * Iterator function implementation. Return the next record in the iterator. - * Makes use of {@link nextRecord()}, takes care of the plumbing. - * - * @return array - */ - #[\ReturnTypeWillChange] - public function next() - { - $this->queryHasBegun = true; - $this->currentRecord = $this->nextRecord(); - $this->rowNum++; - return $this->currentRecord; - } - - /** - * Iterator function implementation. Check if the iterator is pointing to a valid item. - * - * @return bool - */ - #[\ReturnTypeWillChange] - public function valid() - { - if (!$this->queryHasBegun) { - $this->next(); - } - return $this->currentRecord !== false; - } - /** * Return the next record in the query result. * * @return array */ - abstract public function nextRecord(); + abstract public function getIterator(); /** * Return the total number of items in the query result. @@ -259,12 +157,4 @@ abstract class Query implements Iterator * @return int */ abstract public function numRecords(); - - /** - * Go to a specific row number in the query result and return the record. - * - * @param int $rowNum Row number to go to. - * @return array - */ - abstract public function seek($rowNum); } diff --git a/src/ORM/ManyManyList.php b/src/ORM/ManyManyList.php index 5ad69cd8f..ff841a9dc 100644 --- a/src/ORM/ManyManyList.php +++ b/src/ORM/ManyManyList.php @@ -521,7 +521,7 @@ class ManyManyList extends RelationList $query->addWhere([ "\"{$this->joinTable}\".\"{$this->localKey}\"" => $itemID ]); - $queryResult = $query->execute()->current(); + $queryResult = $query->execute()->record(); if ($queryResult) { foreach ($queryResult as $fieldName => $value) { $result[$fieldName] = $value; diff --git a/tests/php/ORM/DatabaseTest.php b/tests/php/ORM/DatabaseTest.php index 25fa3deed..33a45caaf 100644 --- a/tests/php/ORM/DatabaseTest.php +++ b/tests/php/ORM/DatabaseTest.php @@ -74,7 +74,7 @@ class DatabaseTest extends SapphireTest 'SHOW TABLE STATUS WHERE "Name" = \'%s\'', 'DatabaseTest_MyObject' ) - )->first(); + )->record(); $this->assertEquals( $ret['Engine'], 'InnoDB', From 1b8f601023a7a705b9fd22146e31f5addd15868a Mon Sep 17 00:00:00 2001 From: Loz Calver Date: Thu, 12 Jan 2017 14:56:11 +0000 Subject: [PATCH 06/18] NEW: Make DataList::getIterator a generator --- src/ORM/DataList.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php index 72dfaf02e..45fcf8b40 100644 --- a/src/ORM/DataList.php +++ b/src/ORM/DataList.php @@ -864,12 +864,14 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li * Returns an Iterator for this DataList. * This function allows you to use DataLists in foreach loops * - * @return ArrayIterator + * @return Generator */ #[\ReturnTypeWillChange] public function getIterator() { - return new ArrayIterator($this->toArray()); + foreach ($this->dataQuery->query()->execute() as $row) { + yield $this->createDataObject($row); + } } /** From 6ef5785fc5338eac1213af8feddf9f407e7ff43f Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Wed, 18 Jan 2017 09:24:12 +1300 Subject: [PATCH 07/18] FIX: pre-cache loop content within SSViewer. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SSViewer iterates on Iterators that it receives twice: first to get the total number of items, then to actually render each item. This necessitates a rewind. In order to make more use of generators, which are not rewindable, I’d like to remove the need for a rewind. I’ve done this by caching the content of the iterator as an array within SSViewer_Scope. Although this means a bit of memory usage, there are no cases in which code will get to this point without iterating on all items, which would use the memory anyway. It would only create onerous impacts in cases where you are iterating on very long iterators, which would mean you’re rendering a very large page anyway, and probably have other performance issues. --- src/View/SSViewer_Scope.php | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/View/SSViewer_Scope.php b/src/View/SSViewer_Scope.php index 224fd0753..69a4c4cd4 100644 --- a/src/View/SSViewer_Scope.php +++ b/src/View/SSViewer_Scope.php @@ -289,14 +289,20 @@ class SSViewer_Scope } if (!$this->itemIterator) { + // Turn the iterator into an array. This lets us get the count and iterate on it, even if it's a generator. if (is_array($this->item)) { - $this->itemIterator = new ArrayIterator($this->item); + $arrayVersion = $this->item; } else { - $this->itemIterator = $this->item->getIterator(); + $arrayVersion = []; + foreach ($this->item as $record) { + $arrayVersion[] = $record; + } } + $this->itemIterator = new ArrayIterator($arrayVersion); + $this->itemStack[$this->localIndex][SSViewer_Scope::ITEM_ITERATOR] = $this->itemIterator; - $this->itemIteratorTotal = iterator_count($this->itemIterator); // Count the total number of items + $this->itemIteratorTotal = count($arrayVersion); // Count the total number of items $this->itemStack[$this->localIndex][SSViewer_Scope::ITEM_ITERATOR_TOTAL] = $this->itemIteratorTotal; $this->itemIterator->rewind(); } else { From 749405170ccbcc4e8a36cd4c171a1d9991c6e5cb Mon Sep 17 00:00:00 2001 From: Loz Calver Date: Wed, 28 Jun 2017 12:22:45 +0100 Subject: [PATCH 08/18] Update MySQLDatabaseTest to work with new query iterators --- tests/php/ORM/MySQLDatabaseTest.php | 60 +++++++++++++---------------- 1 file changed, 26 insertions(+), 34 deletions(-) diff --git a/tests/php/ORM/MySQLDatabaseTest.php b/tests/php/ORM/MySQLDatabaseTest.php index 7b084b045..bd3d3f8bf 100644 --- a/tests/php/ORM/MySQLDatabaseTest.php +++ b/tests/php/ORM/MySQLDatabaseTest.php @@ -45,58 +45,50 @@ class MySQLDatabaseTest extends SapphireTest $this->assertInstanceOf(MySQLQuery::class, $result3); // Iterating one level should not buffer, but return the right result + $result1Array = []; + foreach($result1 as $record) { + $result1Array[] = $record; + } $this->assertEquals( [ - 'Sort' => 1, - 'Title' => 'First Item' + [ 'Sort' => 1, 'Title' => 'First Item' ], + [ 'Sort' => 2, 'Title' => 'Second Item' ], + [ 'Sort' => 3, 'Title' => 'Third Item' ], + [ 'Sort' => 4, 'Title' => 'Last Item' ], ], - $result1->next() - ); - $this->assertEquals( - [ - 'Sort' => 2, - 'Title' => 'Second Item' - ], - $result1->next() - ); - - // Test first - $this->assertEquals( - [ - 'Sort' => 1, - 'Title' => 'First Item' - ], - $result1->first() - ); - - // Test seek - $this->assertEquals( - [ - 'Sort' => 2, - 'Title' => 'Second Item' - ], - $result1->seek(1) + $result1Array ); // Test count $this->assertEquals(4, $result1->numRecords()); + // Test count + $this->assertEquals(4, $result1->numRecords()); + // Test second statement + $result2Array = []; + foreach($result2 as $record) { + $result2Array[] = $record; + break; + } $this->assertEquals( [ - 'Sort' => 3, - 'Title' => 'Third Item' + [ 'Sort' => 3, 'Title' => 'Third Item' ], ], - $result2->next() + $result2Array ); // Test non-prepared query + $result3Array = []; + foreach($result3 as $record) { + $result3Array[] = $record; + break; + } $this->assertEquals( [ - 'Sort' => 1, - 'Title' => 'First Item' + [ 'Sort' => 1, 'Title' => 'First Item' ], ], - $result3->next() + $result3Array ); } From 8e0e797b402b1b2ca8916594a9c9a27b22f6a6d1 Mon Sep 17 00:00:00 2001 From: Loz Calver Date: Wed, 28 Jun 2017 15:21:53 +0100 Subject: [PATCH 09/18] Fix code style --- src/ORM/Connect/MySQLStatement.php | 2 +- src/ORM/Map.php | 10 +++++----- src/ORM/PaginatedList.php | 2 +- tests/php/ORM/MySQLDatabaseTest.php | 6 +++--- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/ORM/Connect/MySQLStatement.php b/src/ORM/Connect/MySQLStatement.php index b46390b8a..50862de0b 100644 --- a/src/ORM/Connect/MySQLStatement.php +++ b/src/ORM/Connect/MySQLStatement.php @@ -104,7 +104,7 @@ class MySQLStatement extends Query public function getIterator() { - while($this->statement->fetch()) { + while ($this->statement->fetch()) { // Dereferenced row $row = []; foreach ($this->boundValues as $key => $value) { diff --git a/src/ORM/Map.php b/src/ORM/Map.php index a79177f8d..d062cd4c6 100644 --- a/src/ORM/Map.php +++ b/src/ORM/Map.php @@ -263,21 +263,21 @@ class Map implements ArrayAccess, Countable, IteratorAggregate $keyField = $this->keyField; $valueField = $this->valueField; - foreach($this->firstItems as $k => $v) { + foreach ($this->firstItems as $k => $v) { yield $k => $v; } - foreach($this->list as $record) { - if(isset($this->firstItems[$record->$keyField])) { + foreach ($this->list as $record) { + if (isset($this->firstItems[$record->$keyField])) { continue; } - if(isset($this->lastItems[$record->$keyField])) { + if (isset($this->lastItems[$record->$keyField])) { continue; } yield $this->extractValue($record, $this->keyField) => $this->extractValue($record, $this->valueField); } - foreach($this->lastItems as $k => $v) { + foreach ($this->lastItems as $k => $v) { yield $k => $v; } } diff --git a/src/ORM/PaginatedList.php b/src/ORM/PaginatedList.php index 1d0554e03..96a6fed9f 100644 --- a/src/ORM/PaginatedList.php +++ b/src/ORM/PaginatedList.php @@ -233,7 +233,7 @@ class PaginatedList extends ListDecorator $result = []; // Use getIterator() - foreach($this as $record) { + foreach ($this as $record) { $result[] = $record; } diff --git a/tests/php/ORM/MySQLDatabaseTest.php b/tests/php/ORM/MySQLDatabaseTest.php index bd3d3f8bf..44ff7be28 100644 --- a/tests/php/ORM/MySQLDatabaseTest.php +++ b/tests/php/ORM/MySQLDatabaseTest.php @@ -46,7 +46,7 @@ class MySQLDatabaseTest extends SapphireTest // Iterating one level should not buffer, but return the right result $result1Array = []; - foreach($result1 as $record) { + foreach ($result1 as $record) { $result1Array[] = $record; } $this->assertEquals( @@ -67,7 +67,7 @@ class MySQLDatabaseTest extends SapphireTest // Test second statement $result2Array = []; - foreach($result2 as $record) { + foreach ($result2 as $record) { $result2Array[] = $record; break; } @@ -80,7 +80,7 @@ class MySQLDatabaseTest extends SapphireTest // Test non-prepared query $result3Array = []; - foreach($result3 as $record) { + foreach ($result3 as $record) { $result3Array[] = $record; break; } From 850482138b721690ddd6104a8e1cbcae5e0d1ed9 Mon Sep 17 00:00:00 2001 From: Loz Calver Date: Wed, 28 Jun 2017 14:40:06 +0100 Subject: [PATCH 10/18] Proposed solution for caching template generator counts --- src/ORM/DataList.php | 56 +++++++++++++++++++++++++++++++++-- src/View/SSViewer_Scope.php | 29 +++++++++++------- src/View/TemplateIterator.php | 24 +++++++++++++++ 3 files changed, 97 insertions(+), 12 deletions(-) create mode 100644 src/View/TemplateIterator.php diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php index 45fcf8b40..af7b73d93 100644 --- a/src/ORM/DataList.php +++ b/src/ORM/DataList.php @@ -6,6 +6,7 @@ use SilverStripe\Core\Injector\Injector; use SilverStripe\Dev\Debug; use SilverStripe\ORM\Filters\SearchFilter; use SilverStripe\ORM\Queries\SQLConditionGroup; +use SilverStripe\View\TemplateIterator; use SilverStripe\View\ViewableData; use ArrayIterator; use Exception; @@ -32,7 +33,7 @@ use LogicException; * * Subclasses of DataList may add other methods that have the same effect. */ -class DataList extends ViewableData implements SS_List, Filterable, Sortable, Limitable +class DataList extends ViewableData implements SS_List, Filterable, Sortable, Limitable, TemplateIterator { /** @@ -49,6 +50,13 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li */ protected $dataQuery; + /** + * A cached Query to save repeated database calls. {@see DataList::getTemplateIteratorCount()} + * + * @var SilverStripe\ORM\Connect\Query + */ + protected $finalisedQuery; + /** * Create a new DataList. * No querying is done on construction, but the initial query schema is set up. @@ -79,6 +87,7 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li public function __clone() { $this->dataQuery = clone $this->dataQuery; + $this->finalisedQuery = null; } /** @@ -869,11 +878,45 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li #[\ReturnTypeWillChange] public function getIterator() { - foreach ($this->dataQuery->query()->execute() as $row) { + foreach ($this->getFinalisedQuery() as $row) { yield $this->createDataObject($row); } } + /** + * @return Generator|DataObject[] + */ + public function getTemplateIterator() + { + foreach ($this->getFinalisedQuery() as $row) { + yield $this->createDataObject($row); + } + } + + /** + * @return int + */ + public function getTemplateIteratorCount() + { + return $this->getFinalisedQuery()->numRecords(); + } + + /** + * Returns the Query result for this DataList. Repeated calls will return + * a cached result, unless the DataQuery underlying this list has been + * modified + * + * @return SilverStripe\ORM\Connect\Query + */ + protected function getFinalisedQuery() + { + if (!$this->finalisedQuery) { + $this->finalisedQuery = $this->dataQuery->query()->execute(); + } + + return $this->finalisedQuery; + } + /** * Return the number of items in this DataList * @@ -882,6 +925,10 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li #[\ReturnTypeWillChange] public function count() { + if ($this->finalisedQuery) { + return $this->finalisedQuery->numRecords(); + } + return $this->dataQuery->count(); } @@ -1029,6 +1076,11 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li */ public function column($colName = "ID") { + if ($this->finalisedQuery) { + $finalisedQuery = clone $this->finalisedQuery; + return $finalisedQuery->distinct(false)->column($colName); + } + $dataQuery = clone $this->dataQuery; return $dataQuery->distinct(false)->column($colName); } diff --git a/src/View/SSViewer_Scope.php b/src/View/SSViewer_Scope.php index 69a4c4cd4..8f7814c43 100644 --- a/src/View/SSViewer_Scope.php +++ b/src/View/SSViewer_Scope.php @@ -3,6 +3,7 @@ namespace SilverStripe\View; use ArrayIterator; +use Countable; use Iterator; /** @@ -289,22 +290,30 @@ class SSViewer_Scope } if (!$this->itemIterator) { - // Turn the iterator into an array. This lets us get the count and iterate on it, even if it's a generator. - if (is_array($this->item)) { - $arrayVersion = $this->item; + // TemplateIterator provides methods for extracting the count and iterator directly + if ($this->item instanceof TemplateIterator) { + $this->itemIterator = $this->item->getTemplateIterator(); + $this->itemIteratorTotal = $this->item->getTemplateIteratorCount(); } else { - $arrayVersion = []; - foreach ($this->item as $record) { - $arrayVersion[] = $record; + // Item may be an array or a regular IteratorAggregate + if (is_array($this->item)) { + $this->itemIterator = new ArrayIterator($this->item); + } else { + $this->itemIterator = $this->item->getIterator(); + } + + // If the item implements Countable, use that to fetch the count, otherwise we have to inspect the + // iterator and then rewind it + if ($this->item instanceof Countable) { + $this->itemIteratorTotal = count($this->item); + } else { + $this->itemIteratorTotal = iterator_count($this->itemIterator); + $this->itemIterator->rewind(); } } - $this->itemIterator = new ArrayIterator($arrayVersion); - $this->itemStack[$this->localIndex][SSViewer_Scope::ITEM_ITERATOR] = $this->itemIterator; - $this->itemIteratorTotal = count($arrayVersion); // Count the total number of items $this->itemStack[$this->localIndex][SSViewer_Scope::ITEM_ITERATOR_TOTAL] = $this->itemIteratorTotal; - $this->itemIterator->rewind(); } else { $this->itemIterator->next(); } diff --git a/src/View/TemplateIterator.php b/src/View/TemplateIterator.php new file mode 100644 index 000000000..bc076e55b --- /dev/null +++ b/src/View/TemplateIterator.php @@ -0,0 +1,24 @@ + Date: Thu, 29 Jun 2017 11:17:27 +1200 Subject: [PATCH 11/18] =?UTF-8?q?Further=20work=20on=20Loz=E2=80=99=20solu?= =?UTF-8?q?tion?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/ORM/DataList.php | 21 ++----------------- src/View/SSViewer_Scope.php | 39 ++++++++++++++++++----------------- src/View/TemplateIterator.php | 24 --------------------- 3 files changed, 22 insertions(+), 62 deletions(-) delete mode 100644 src/View/TemplateIterator.php diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php index af7b73d93..2a3437847 100644 --- a/src/ORM/DataList.php +++ b/src/ORM/DataList.php @@ -33,7 +33,7 @@ use LogicException; * * Subclasses of DataList may add other methods that have the same effect. */ -class DataList extends ViewableData implements SS_List, Filterable, Sortable, Limitable, TemplateIterator +class DataList extends ViewableData implements SS_List, Filterable, Sortable, Limitable { /** @@ -883,30 +883,13 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li } } - /** - * @return Generator|DataObject[] - */ - public function getTemplateIterator() - { - foreach ($this->getFinalisedQuery() as $row) { - yield $this->createDataObject($row); - } - } - - /** - * @return int - */ - public function getTemplateIteratorCount() - { - return $this->getFinalisedQuery()->numRecords(); - } - /** * Returns the Query result for this DataList. Repeated calls will return * a cached result, unless the DataQuery underlying this list has been * modified * * @return SilverStripe\ORM\Connect\Query + * @internal This API may change in minor releases */ protected function getFinalisedQuery() { diff --git a/src/View/SSViewer_Scope.php b/src/View/SSViewer_Scope.php index 8f7814c43..28d1b541f 100644 --- a/src/View/SSViewer_Scope.php +++ b/src/View/SSViewer_Scope.php @@ -290,26 +290,27 @@ class SSViewer_Scope } if (!$this->itemIterator) { - // TemplateIterator provides methods for extracting the count and iterator directly - if ($this->item instanceof TemplateIterator) { - $this->itemIterator = $this->item->getTemplateIterator(); - $this->itemIteratorTotal = $this->item->getTemplateIteratorCount(); - } else { - // Item may be an array or a regular IteratorAggregate - if (is_array($this->item)) { - $this->itemIterator = new ArrayIterator($this->item); - } else { - $this->itemIterator = $this->item->getIterator(); - } + // Note: it is important that getIterator() is called before count() as implemenations may rely on + // this to efficiency get both the number of records and an iterator (e.g. DataList does this) - // If the item implements Countable, use that to fetch the count, otherwise we have to inspect the - // iterator and then rewind it - if ($this->item instanceof Countable) { - $this->itemIteratorTotal = count($this->item); - } else { - $this->itemIteratorTotal = iterator_count($this->itemIterator); - $this->itemIterator->rewind(); - } + // Item may be an array or a regular IteratorAggregate + if (is_array($this->item)) { + $this->itemIterator = new ArrayIterator($this->item); + } else { + $this->itemIterator = $this->item->getIterator(); + + // This will execute code in a generator up to the first yield. For example, this ensures that + // DataList::getIterator() is called before Datalist::count() + $this->itemIterator->rewind(); + } + + // If the item implements Countable, use that to fetch the count, otherwise we have to inspect the + // iterator and then rewind it. + if ($this->item instanceof Countable) { + $this->itemIteratorTotal = count($this->item); + } else { + $this->itemIteratorTotal = iterator_count($this->itemIterator); + $this->itemIterator->rewind(); } $this->itemStack[$this->localIndex][SSViewer_Scope::ITEM_ITERATOR] = $this->itemIterator; diff --git a/src/View/TemplateIterator.php b/src/View/TemplateIterator.php deleted file mode 100644 index bc076e55b..000000000 --- a/src/View/TemplateIterator.php +++ /dev/null @@ -1,24 +0,0 @@ - Date: Thu, 29 Jun 2017 11:47:42 +1200 Subject: [PATCH 12/18] FIX: Re-set finalisedQuery to allow recreation of iterator. --- src/ORM/DataList.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php index 2a3437847..93cf301fb 100644 --- a/src/ORM/DataList.php +++ b/src/ORM/DataList.php @@ -881,6 +881,9 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li foreach ($this->getFinalisedQuery() as $row) { yield $this->createDataObject($row); } + + // Re-set the finaliseQuery so that it can be re-executed + $this->finalisedQuery = null; } /** From 81beddc161e7e1696b6f65b72df79617cd65dede Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Thu, 29 Jun 2017 11:49:22 +1200 Subject: [PATCH 13/18] Add deprecated method to make CMS tests work --- src/ORM/Connect/Query.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/ORM/Connect/Query.php b/src/ORM/Connect/Query.php index 1d7497ffa..6ad5e6353 100644 --- a/src/ORM/Connect/Query.php +++ b/src/ORM/Connect/Query.php @@ -95,6 +95,15 @@ abstract class Query implements \IteratorAggregate return $this->getIterator()->current(); } + /** + * @deprecated Use record() instead + * @return array + */ + public function first() + { + return $this->record(); + } + /** * Returns the first column of the first record. * From bf331072df0c9c3f3b941a4405c1c9de5f58315c Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Thu, 11 Aug 2022 15:04:33 +1200 Subject: [PATCH 14/18] FIX Don't try to call count() on an iterator --- src/ORM/DataList.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php index 93cf301fb..e68c42dea 100644 --- a/src/ORM/DataList.php +++ b/src/ORM/DataList.php @@ -1357,14 +1357,15 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li $currentChunk = 0; // Keep looping until we run out of chunks - while ($chunk = $this->limit($chunkSize, $chunkSize * $currentChunk)->getIterator()) { + while ($chunk = $this->limit($chunkSize, $chunkSize * $currentChunk)) { // Loop over all the item in our chunk + $count = 0; foreach ($chunk as $item) { + $count++; yield $item; } - - if ($chunk->count() < $chunkSize) { + if ($count < $chunkSize) { // If our last chunk had less item than our chunkSize, we've reach the end. break; } From a76fa32a390064b7571a2b318cda48357d918eb8 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Thu, 11 Aug 2022 15:04:52 +1200 Subject: [PATCH 15/18] API Remove unnecessary `getGenerator()` method. `getIterator()` now returns a generator by default. --- src/Forms/GridField/GridFieldExportButton.php | 4 ---- src/ORM/DataList.php | 16 +--------------- 2 files changed, 1 insertion(+), 19 deletions(-) diff --git a/src/Forms/GridField/GridFieldExportButton.php b/src/Forms/GridField/GridFieldExportButton.php index 9e56b76aa..9fc3ad3f3 100644 --- a/src/Forms/GridField/GridFieldExportButton.php +++ b/src/Forms/GridField/GridFieldExportButton.php @@ -223,10 +223,6 @@ class GridFieldExportButton extends AbstractGridFieldComponent implements GridFi // Remove limit as the list may be paginated, we want the full list for the export $items = $items->limit(null); - // Use Generator in applicable cases to reduce memory consumption - $items = $items instanceof DataList - ? $items->getGenerator() - : $items; /** @var DataObject $item */ foreach ($items as $item) { diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php index e68c42dea..e9f65b04d 100644 --- a/src/ORM/DataList.php +++ b/src/ORM/DataList.php @@ -790,20 +790,6 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li return $this; } - /** - * Returns a generator for this DataList - * - * @return \Generator&DataObject[] - */ - public function getGenerator() - { - $query = $this->dataQuery->query()->execute(); - - while ($row = $query->record()) { - yield $this->createDataObject($row); - } - } - public function debug() { $val = "

" . static::class . "

    "; @@ -1214,7 +1200,7 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li */ public function removeAll() { - foreach ($this->getGenerator() as $item) { + foreach ($this as $item) { $this->remove($item); } return $this; From d9be52579d79f38be861260d1e3f03923ee14447 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Thu, 11 Aug 2022 15:10:07 +1200 Subject: [PATCH 16/18] MNT Fix test --- tests/php/ORM/ListDecoratorTest.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/php/ORM/ListDecoratorTest.php b/tests/php/ORM/ListDecoratorTest.php index 2e85f50c9..0410bba76 100644 --- a/tests/php/ORM/ListDecoratorTest.php +++ b/tests/php/ORM/ListDecoratorTest.php @@ -2,6 +2,7 @@ namespace SilverStripe\ORM\Tests; +use ArrayIterator; use LogicException; use PHPUnit\Framework\MockObject\MockObject; use SilverStripe\Dev\SapphireTest; @@ -34,8 +35,9 @@ class ListDecoratorTest extends SapphireTest public function testGetIterator() { - $this->list->expects($this->once())->method('getIterator')->willReturn('mock'); - $this->assertSame('mock', $this->decorator->getIterator()); + $iterator = new ArrayIterator(); + $this->list->expects($this->once())->method('getIterator')->willReturn($iterator); + $this->assertSame($iterator, $this->decorator->getIterator()); } public function testCanSortBy() From 62ee63706f7d97bdd96faffed44db65b6090b280 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Fri, 9 Sep 2022 10:48:05 +1200 Subject: [PATCH 17/18] FIX PHP 8.1 compatability for iterators. Setting a proper return type for these will be done in a separate PR --- src/ORM/Connect/MySQLQuery.php | 1 + src/ORM/Connect/MySQLStatement.php | 1 + src/ORM/Connect/Query.php | 3 +-- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/ORM/Connect/MySQLQuery.php b/src/ORM/Connect/MySQLQuery.php index e42dddee8..56b9e0dbc 100644 --- a/src/ORM/Connect/MySQLQuery.php +++ b/src/ORM/Connect/MySQLQuery.php @@ -45,6 +45,7 @@ class MySQLQuery extends Query } } + #[\ReturnTypeWillChange] public function getIterator() { $floatTypes = [MYSQLI_TYPE_FLOAT, MYSQLI_TYPE_DOUBLE, MYSQLI_TYPE_DECIMAL, MYSQLI_TYPE_NEWDECIMAL]; diff --git a/src/ORM/Connect/MySQLStatement.php b/src/ORM/Connect/MySQLStatement.php index 50862de0b..da6878988 100644 --- a/src/ORM/Connect/MySQLStatement.php +++ b/src/ORM/Connect/MySQLStatement.php @@ -102,6 +102,7 @@ class MySQLStatement extends Query call_user_func_array([$this->statement, 'bind_result'], $variables ?? []); } + #[\ReturnTypeWillChange] public function getIterator() { while ($this->statement->fetch()) { diff --git a/src/ORM/Connect/Query.php b/src/ORM/Connect/Query.php index 6ad5e6353..afcf6d4f8 100644 --- a/src/ORM/Connect/Query.php +++ b/src/ORM/Connect/Query.php @@ -155,9 +155,8 @@ abstract class Query implements \IteratorAggregate /** * Return the next record in the query result. - * - * @return array */ + #[\ReturnTypeWillChange] abstract public function getIterator(); /** From e140c3786c96ac146127a92e08dee470c6c6682c Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Fri, 9 Sep 2022 15:45:12 +1200 Subject: [PATCH 18/18] FIX Ensure consistent behaviour with repeat iterations --- src/ORM/Connect/MySQLQuery.php | 5 ++++ tests/php/ORM/DataListTest.php | 1 - tests/php/ORM/DatabaseTest.php | 55 ++++++++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/src/ORM/Connect/MySQLQuery.php b/src/ORM/Connect/MySQLQuery.php index 56b9e0dbc..4f8db6993 100644 --- a/src/ORM/Connect/MySQLQuery.php +++ b/src/ORM/Connect/MySQLQuery.php @@ -63,6 +63,11 @@ class MySQLQuery extends Query } yield $data; } + // Check for the method first since $this->handle is a mixed type + if (method_exists($this->handle, 'data_seek')) { + // Reset so the query can be iterated over again + $this->handle->data_seek(0); + } } } diff --git a/tests/php/ORM/DataListTest.php b/tests/php/ORM/DataListTest.php index dc80aaf40..8ce1de790 100755 --- a/tests/php/ORM/DataListTest.php +++ b/tests/php/ORM/DataListTest.php @@ -42,7 +42,6 @@ class DataListTest extends SapphireTest ); } - public function testFilterDataObjectByCreatedDate() { // create an object to test with diff --git a/tests/php/ORM/DatabaseTest.php b/tests/php/ORM/DatabaseTest.php index 33a45caaf..706d25db3 100644 --- a/tests/php/ORM/DatabaseTest.php +++ b/tests/php/ORM/DatabaseTest.php @@ -7,6 +7,7 @@ use SilverStripe\ORM\Connect\MySQLDatabase; use SilverStripe\MSSQL\MSSQLDatabase; use SilverStripe\Dev\SapphireTest; use Exception; +use SilverStripe\ORM\Queries\SQLSelect; use SilverStripe\ORM\Tests\DatabaseTest\MyObject; /** @@ -293,4 +294,58 @@ class DatabaseTest extends SapphireTest $this->assertEquals($inputData, $query->map()); $this->assertEquals($inputData, $query->map()); } + + /** + * Test that repeated abstracted iteration of a query returns all records. + */ + public function testRepeatedIterationUsingAbstraction() + { + $inputData = ['one', 'two', 'three', 'four']; + + foreach ($inputData as $i => $text) { + $x = new MyObject(); + $x->MyField = $text; + $x->MyInt = $i; + $x->write(); + } + + $select = SQLSelect::create(['"MyInt"', '"MyField"'], '"DatabaseTest_MyObject"', orderby: ['"MyInt"']); + + $this->assertEquals($inputData, $select->execute()->map()); + $this->assertEquals($inputData, $select->execute()->map()); + } + + /** + * Test that stopping iteration part-way through produces predictable results + * on a subsequent iteration. + * This test is here to ensure consistency between implementations (e.g. mysql vs postgres, etc) + */ + public function testRepeatedPartialIteration() + { + $inputData = ['one', 'two', 'three', 'four']; + + foreach ($inputData as $i => $text) { + $x = new MyObject(); + $x->MyField = $text; + $x->MyInt = $i; + $x->write(); + } + + $query = DB::query('SELECT "MyInt", "MyField" FROM "DatabaseTest_MyObject" ORDER BY "MyInt"'); + + $i = 0; + foreach ($query as $record) { + $this->assertEquals($inputData[$i], $record['MyField']); + $i++; + if ($i > 1) { + break; + } + } + + // Continue from where we left off, since we're using a Generator + foreach ($query as $record) { + $this->assertEquals($inputData[$i], $record['MyField']); + $i++; + } + } }