Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Zend\Db\Resultset fix buffering #5308

Closed
wants to merge 3 commits into from
Closed

Zend\Db\Resultset fix buffering #5308

wants to merge 3 commits into from

Conversation

turrsis
Copy link
Contributor

@turrsis turrsis commented Oct 21, 2013

fix buffering
optimize some methods

@@ -56,6 +58,16 @@ public function setObjectPrototype($objectPrototype)
}

/**
* Get the row object prototype
*
* @return stdObject
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stdClass maybe ?

@turrsis
Copy link
Contributor Author

turrsis commented Oct 22, 2013

My local tests is ok - where i wrong?

@samsonasik
Copy link
Contributor

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

@ralphschindler
Copy link
Member

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

@turrsis
Copy link
Contributor Author

turrsis commented Oct 31, 2013

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
Copy link
Contributor Author

turrsis commented Oct 31, 2013

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

optimize some methods
fixes for added tests
@ralphschindler
Copy link
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
Copy link
Contributor Author

turrsis commented Nov 19, 2013

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

@ralphschindler
Copy link
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
Copy link
Contributor Author

turrsis commented Nov 19, 2013

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

@weierophinney
Copy link
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 milestones: 2.3.0, 2.2.6 Mar 4, 2014
@ralphschindler
Copy link
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
Copy link
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.

}

if (is_array($dataSource)) {
} elseif (is_array($dataSource)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for this to be turned into elseif?

@Ocramius
Copy link
Member

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

ralphschindler pushed a commit that referenced this pull request Mar 10, 2014
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
@ralphschindler
Copy link
Member

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

@turrsis turrsis deleted the hotfix/db-resultset-fix-buffering-and-refactor branch May 7, 2014 10:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants