Fix data not being cached in ResultCache #286

Closed
wants to merge 1 commit into
from

4 participants

@Alan01252

The ResultCacheStatement has a small logic error, it tested for emptied
being true when it should have tested for emptied being false. The
comments also statement that the function returns a boolean, however the
function didn't return a boolean.

This is my first time committing to a larger open source project like dbal. Very worried I'll of done something wrong, please go easy / teach me.

unknown Fix data not being cached in ResultCache
The ResultCacheStatement has a small logic error, it tested for emptied
being true when it should have tested for emptied being false. The
comments also statement that the function returns a boolean, however the
function didn't return a boolean.
b733674
@doctrinebot

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-465

@stof stof commented on the diff Mar 19, 2013
lib/Doctrine/DBAL/Cache/ResultCacheStatement.php
$data = $this->resultCache->fetch($this->cacheKey);
if ( ! $data) {
$data = array();
}
$data[$this->realKey] = $this->data;
- $this->resultCache->save($this->cacheKey, $data, $this->lifetime);
+ if(!$this->resultCache->save($this->cacheKey, $data, $this->lifetime)){
+ return false;
+ }
unset($this->data);
@stof
Doctrine member
stof added a line comment Mar 19, 2013

this is not reached anymore

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

Please add a test to avoid regressions

@Alan01252

This might be a bit beyond me... I've looked at the first test that fails and it appears it's because the closeCursor function isn't called so the result doesn't ever get cached. I think the second test that fails is because it does use the closeCursor.

I'd like to see this pull request through, but I'll need to set up the test environment properly at home, I'm not able to do so at work. Might take me a little while.

Sorry!

@guilhermeblanco guilhermeblanco commented on the diff Mar 19, 2013
lib/Doctrine/DBAL/Cache/ResultCacheStatement.php
@@ -107,16 +107,19 @@ public function __construct(Statement $stmt, Cache $resultCache, $cacheKey, $rea
public function closeCursor()
{
$this->statement->closeCursor();
- if ($this->emptied && $this->data !== null) {
+ if (!$this->emptied && $this->data !== null) {
@guilhermeblanco
Doctrine member
guilhermeblanco added a line comment Mar 19, 2013

missing spaces around !.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on the diff Mar 19, 2013
lib/Doctrine/DBAL/Cache/ResultCacheStatement.php
$data = $this->resultCache->fetch($this->cacheKey);
if ( ! $data) {
$data = array();
}
$data[$this->realKey] = $this->data;
- $this->resultCache->save($this->cacheKey, $data, $this->lifetime);
+ if(!$this->resultCache->save($this->cacheKey, $data, $this->lifetime)){
@guilhermeblanco
Doctrine member
guilhermeblanco added a line comment Mar 19, 2013

missing spaces around !.

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

And you should set your email address in the git config.
https://help.github.com/articles/set-up-git

@Alan01252

Okay, it took me a little longer to set-up my machine to be able to run the tests than I thought it would..

Is anyone here knowledgeable on what the test:

ResultCacheTest.php . line 134 testDontFinishNoCache() function is meant to be testing? I don't understand the don't finish bit.

Thanks in advance

Alan

@Alan01252 Alan01252 closed this Mar 20, 2013
@Alan01252 Alan01252 reopened this Mar 20, 2013
@Alan01252

Closing pull request, didn't understand code properly, should never have submitted. Apologies!

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