Zend\Db\Resultset fix buffering #5308

Closed
wants to merge 3 commits into
from

5 participants

@turrsis

fix buffering
optimize some methods

@samsonasik samsonasik commented on an outdated diff Oct 21, 2013
library/Zend/Db/ResultSet/HydratingResultSet.php
@@ -56,6 +58,16 @@ public function setObjectPrototype($objectPrototype)
}
/**
+ * Get the row object prototype
+ *
+ * @return stdObject
@samsonasik
samsonasik added a line comment Oct 21, 2013

stdClass maybe ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@turrsis

My local tests is ok - where i wrong?

@samsonasik

it seems the error is on ZendTest\Http\ClientTest::testUsageOfCustomResponseInvalidCode which not related with your commits.

@ralphschindler
Zend Framework member

Can you expand on the problems you're trying to solve in these commits?

@turrsis

ResultSet and HydratingResultSet not use the buffer for converted values (see current() method) and convert current data on each current() call.
Parameters for ResultSet::__construct is excess, because you can set only $arrayObjectPrototype and $returnType can be calculated
Сode simplification.

@turrsis

Sorry - only ResultSet not use the buffer for converted values (see current() method) and convert current data on each current() call.
HydratingResultSet (in current()) using code which already exist in AbstractResultSet

turrsis added some commits Sep 24, 2013
@turrsis turrsis fix buffering
optimize some methods
c3f3ac8
@turrsis turrsis added twice foreach tests
fixes for added tests
8612b40
@ralphschindler
Zend Framework member

Hey @turris,

I can't seem to duplicate your problem. Using my repo at Zend_Db-Example (https://github.com/ralphschindler/Zend_Db-Examples):

I've created the following script and witness that the buffered values are in fact utilized:

<?php

/** @var $adapter Zend\Db\Adapter\Adapter */
$adapter = include ((file_exists('bootstrap.php')) ? 'bootstrap.php' : 'bootstrap.dist.php');
refresh_data($adapter);

use Zend\Db\ResultSet\HydratingResultSet;
use Zend\Db\ResultSet\ResultSet;

$stmt = $adapter->query('SELECT * FROM album');
$result = $stmt->execute();

if (1) {
    class Foo extends ArrayObject { public function __clone() { echo 'cloning' . PHP_EOL; } }
    $hrs = new HydratingResultSet();
    $hrs->setObjectPrototype(new Foo);
    $hrs->initialize($result);
    $hrs->buffer();
    foreach ($hrs as $row) {
        var_dump($row['title']);
    }
    foreach ($hrs as $row) {
        var_dump($row['title']);
    }
} else {
    $rs = new ResultSet;
    $rs->initialize($result);
    $rs->buffer();
    foreach ($rs as $row) {
        var_dump($row['title']);
    }
    foreach ($rs as $row) {
        var_dump($row['title']);
    }
}

If you still find that not to be the case, how can I setup a reproduction of your problem?

@turrsis

@ralphschindler If merge last commit to develop, only HydratingResultSetTest::testSameCurrentForIteratorDataAndBuffering() is ok

@ralphschindler
Zend Framework member

So what you're saying is concurrent calls to current() without moving the internal pointer do not use the buffer. Ok, now I see what you're getting at. I'll have a look.

@turrsis

@ralphschindler you're right, sorry - this is my inaccurate description.

@weierophinney
Zend Framework member

@ralphschindler Can you update and specify a target milestone, please?

@ralphschindler ralphschindler added this to the 2.2.6 milestone Mar 4, 2014
@weierophinney weierophinney modified the milestone: 2.3.0, 2.2.6 Mar 4, 2014
@ralphschindler
Zend Framework member

Hey @turrsis, while I admit there is an issue with iteration and buffering, there is also too much unnecessary refactoring going on in this PR to take as is. I will work backwards from your unit tests and produce an alternative PR.

@ralphschindler
Zend Framework member

For the record, this would be the unit test to prove this is an issue:

namespace ZendTest\Db\ResultSet;

class AbstractResultSetIntegrationTest extends \PHPUnit_Framework_TestCase
{
    protected $resultSet;
    protected function setUp() {
        $this->resultSet = $this->getMockForAbstractClass('Zend\Db\ResultSet\AbstractResultSet');
    }

    public function testCurrentCallsDataSourceOnceWithoutBuffer()
    {
        $result = $this->getMock('Zend\Db\Adapter\Driver\ResultInterface');
        $this->resultSet->initialize($result);
        $result->expects($this->once())->method('current')->will($this->returnValue(array('foo' => 'bar')));
        $value1 = $this->resultSet->current();
        $value2 = $this->resultSet->current();
        $this->assertEquals($value1, $value2);
    }
}

That said, after thinking about this issue, I don't think this is a valid use case. I would expect that at the abstract level, without any buffering, that if I called current() twice, it should do just that with respect to the DataSource.

@Ocramius Ocramius commented on the diff Mar 10, 2014
library/Zend/Db/ResultSet/AbstractResultSet.php
@@ -73,9 +81,7 @@ public function initialize($dataSource)
$this->dataSource->rewind();
}
return $this;
- }
-
- if (is_array($dataSource)) {
+ } elseif (is_array($dataSource)) {
@Ocramius
Zend Framework member
Ocramius added a line comment Mar 10, 2014

Any reason for this to be turned into elseif?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Ocramius Ocramius commented on the diff Mar 10, 2014
library/Zend/Db/ResultSet/AbstractResultSet.php
@@ -49,6 +48,11 @@
protected $position = 0;
/**
+ * @var mixed
+ */
+ protected $current = null;
@Ocramius
Zend Framework member
Ocramius added a line comment Mar 10, 2014

= null is not needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Ocramius
Zend Framework member

@turrsis what happens if the data source is mutable? Shouldn't the value for current() change here?

@ralphschindler ralphschindler added a commit that referenced this pull request Mar 10, 2014
@ralphschindler ralphschindler Closes #5308 as Not An Issue
Merge branch 'hotfix/db-resultset-current-with-buffer-alt' into develop

* hotfix/db-resultset-current-with-buffer-alt:
  Added unit tests for Zend\Db\ResultSet current() handling
f964394
@ralphschindler
Zend Framework member

I've committed a few unit tests that describe the current behavior as the expected intended behavior.

@turrsis turrsis deleted the turrsis:hotfix/db-resultset-fix-buffering-and-refactor branch May 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment