Fix fetchColumn not caching #288

Closed
wants to merge 4 commits into
from

Projects

None yet

4 participants

@Alan01252

Column cache wasn't working because the emptied flag is only set
when fetch no longer returns a row. With fetch column the code
only calls fetch once and so emptied flag never gets set.

Created two test cases, one to test fetchColumn, and one to test
that the return value is false if the data doesn't get saved in the cache.

@Alan01252 Alan01252 Fix fetchColumn no cache
Column cache wasn't working because the emptied flag is only set
when fetch no longer returns a row. With fetch column the code
only calls fetch once and so emptied never gets set
2b37103
@doctrinebot
Collaborator

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DBAL-468

@stof stof commented on an outdated diff Mar 21, 2013
lib/Doctrine/DBAL/Cache/ResultCacheStatement.php
@@ -106,6 +106,7 @@ public function __construct(Statement $stmt, Cache $resultCache, $cacheKey, $rea
*/
public function closeCursor()
{
+ $savedInCache = true;
@stof
stof Mar 21, 2013 Doctrine member

wrong indentation

@stof stof commented on an outdated diff Mar 21, 2013
tests/Doctrine/Tests/DBAL/Functional/ResultCacheTest.php
@@ -74,7 +74,7 @@ public function testFetchColumn()
}
$this->assertCacheNonCacheSelectSameFetchModeAreEqual($expectedResult, \PDO::FETCH_COLUMN);
}
-
+
@stof
stof Mar 21, 2013 Doctrine member

Please don't add trailing whitespaces

@beberlei beberlei commented on the diff Apr 1, 2013
lib/Doctrine/DBAL/Cache/ResultCacheStatement.php
@@ -217,6 +218,7 @@ public function fetchColumn($columnIndex = 0)
// TODO: verify this is correct behavior
return false;
}
+ $this->atEndOfStatement = true;
@beberlei
beberlei Apr 1, 2013 Doctrine member

This is wrong, i can loop over statement and call fetchColumn() multiple times, your code makes this fail big time. Not sure how to proceed here, we have seem to found an edge case that looks rather messy to fix.

@Alan01252
Alan01252 Apr 1, 2013

Sorry, yes you're right I didn't think about that scenario at all.... I'll have another think about how to correct this.

@Alan01252
Alan01252 Apr 2, 2013

Thinking about this further.

I can't think of a scenario when you've explicitly closed the cursor and would not want to cache the data; even if the end of the statement hasn't been reached? So I'm wondering if the atEndOfStatement check is needed at all?

There's probably a scenario I haven't thought of / come across where this would be the case?

@stof
stof Apr 2, 2013 Doctrine member

@Alan01252 If you save in cache before reaching the end of the cursor, you will have incomplete data when using the cache the next time

@Alan01252
Alan01252 Apr 2, 2013

True, but you have to forcibly call closeCursor. This code assumes that the
desired result is to only cache at the end of the cursor but it could be
that the programmer wants the cursor closed early and expects that to be
cached.

I obviously could be completely wrong, just throwing thoughts around!
On 2 Apr 2013 17:51, "Christophe Coevoet" notifications@github.com wrote:

In lib/Doctrine/DBAL/Cache/ResultCacheStatement.php:

@@ -217,6 +218,7 @@ public function fetchColumn($columnIndex = 0)
// TODO: verify this is correct behavior
return false;
}

  •    $this->atEndOfStatement = true;
    

@Alan01252 https://github.com/Alan01252 If you save in cache before
reaching the end of the cursor, you will have incomplete data when using
the cache the next time


Reply to this email directly or view it on GitHubhttps://github.com/doctrine/dbal/pull/288/files#r3621776
.

@beberlei
Member

This bug exists, but this PR is not the correct solution, so closing this PR for now. I am unsure about the right kind of fix here.

@beberlei beberlei closed this Dec 20, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment