Fixes #6812 : Zend\Paginator\Adapter\DbSelect::getItems should return array

[samsonasik]

Fixes #6812

[Pittiplatsch]

Shouldn't you avoid to init the result set object at all, and use statement's result directly to fill the final array?

[samsonasik]

no

[Pittiplatsch]

Changing returned data from ResultSet to array is a BC.
Because old unit tests explicitly tested for ResultSet, it seems to have been a conscious decision rather than a bug to not return a plain array.
Note however that PHPDoc of old getItems() seems to be erroneous when stating @return array.

[samsonasik]

should the changed place is in Zend\Paginator\Paginator::getItem() instead ?

[Martin-P]

There is no need for any BC break and changing of tests. Updating Zend\Paginator\Paginator::getItemsByPage() to detect AbstractResultset is enough to fix the issue. That method already has an if clause to detect if $items is not an instance of Traversable:

if (!$items instanceof Traversable) {
    $items = new ArrayIterator($items);
}

If you prepend an if clause to detect if $items is an instance of AbstractResultSet, the issue is fixed:

if ($items instanceof AbstractResultSet) {
    $temp = array();
    foreach ($items as $key => $value) {
        $temp[$key] = $value;
    }

    $items = $temp;
    unset($temp);
}

The complete code of Zend\Paginator\Paginator::getItemsByPage() would be as follows:

/**
 * Returns the items for a given page.
 *
 * @param int $pageNumber
 * @return mixed
 */
public function getItemsByPage($pageNumber)
{
    $pageNumber = $this->normalizePageNumber($pageNumber);

    if ($this->cacheEnabled()) {
        $data = static::$cache->getItem($this->_getCacheId($pageNumber));
        if ($data) {
            return $data;
        }
    }

    $offset = ($pageNumber - 1) * $this->getItemCountPerPage();

    $items = $this->adapter->getItems($offset, $this->getItemCountPerPage());

    $filter = $this->getFilter();

    if ($filter !== null) {
        $items = $filter->filter($items);
    }

    if ($items instanceof AbstractResultSet) {
        $temp = array();
        foreach ($items as $key => $value) {
            $temp[$key] = $value;
        }

        $items = $temp;
        unset($temp);
    }

    if (!$items instanceof Traversable) {
        $items = new ArrayIterator($items);
    }

    if ($this->cacheEnabled()) {
        $cacheId = $this->_getCacheId($pageNumber);
        static::$cache->setItem($cacheId, $items);
        static::$cache->setTags($cacheId, array($this->_getCacheInternalId()));
    }

    return $items;
}

[samsonasik]

@Martin-P done ;). updated.

[samsonasik]

I've added unit test for it.

[Ocramius]

The AdapterInterface#getItems() defined return type is array, and it cannot be changed here.

[Martin-P]

DbSelect::getItems() never returned an array but always returned a ResultSet. The docs were wrong from the beginning.

See the current master: Zend\Paginator\Adapter\DbSelect line 90

[Ocramius]

I'd say it is a bug in DbSelect then, since it does not respect the interface specification.

[samsonasik]

@Ocramius Ok, I've updated back to @return array and DbSelect::getItems() logic instead to get return array.

[macnibblet]

Urk, this would be a perfect use-case of generators

[samsonasik]

updated loop resultset with ArrayUtils::iteratorToArray($resultSet);

[Ocramius]

Trailing whitespace here

[samsonasik]

@Ocramius done, trailing_spaces should be fixed and @group added.

[samsonasik]

if it is accepted, then zf2-documentation need to be updated for Album tutorial on loop as $album as object to $album as array.
before

 <?php foreach ($this->paginator as $album) : ?>
         <tr>
             <td><?php echo $this->escapeHtml($album->title);?></td>
              <!-- // ... 
        </tr>    
<?php endforeach; ?>

After

 <?php foreach ($this->paginator as $album) : ?>
         <tr>
             <td><?php echo $this->escapeHtml($album['title']);?></td>
              <!-- // ... 
        </tr>    
<?php endforeach; ?>

@Ocramius should I ?

[Ocramius]

@samsonasik I think ArrayUtils::iteratorToArray() is recursive, while iterator_to_array() is not. We may want to use iterator_to_array() only here.

[samsonasik]

@Ocramius done, I have changed to iterator_to_array()

[Ocramius]

@samsonasik the last change needs a test along the lines of (pseudo-code):

$resultset = new Resultset([new ArrayObject(['foo' => 'bar']), new ArrayObject(['foo' => 'bar'])]);

// ... 

foreach ($paginator as $item) {
    $this->assertInstanceOf('ArrayObject', $item);
}

This should prevent a regression, as it will fail with the previous ArrayUtils::iteratorToArray() approach

[samsonasik]

@Ocramius I can't reproduce it directly, but I've updated so it looks like :

$paginator = new Paginator\Paginator($dbSelect);
$this->assertInstanceOf('ArrayIterator', $paginator->getItemsByPage(1));

and to proof that the paginator items instanceof ArrayObject, I did pass $resultSet->getDatasource() to Iterator adapter:

$paginator = new Paginator\Paginator(new Paginator\Adapter\Iterator($resultSet->getDataSource()));
foreach($paginator as $item) {
     $this->assertInstanceOf('ArrayObject', $item);
 }

please let me know I need to do something, thanks ;)

[Ocramius]

@samsonasik that seems to work

[Ocramius]

@samsonasik merged, thanks!

develop: 9f1aeb9