New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: CDbCacheDependency with reuseDependentData did not invalidate cache when getting cache across different requests #2290

Merged
merged 13 commits into from Apr 12, 2013

Make $reusableData protected instead of private, so we can reset in s…

…ubclass.
  • Loading branch information...
marcovtwout committed Apr 8, 2013
commit 00119b13e5c782672ffd979e313a3d911f71e861
@@ -39,7 +39,7 @@ class CCacheDependency extends CComponent implements ICacheDependency
* @var array cached data for reusable dependencies.
* @since 1.1.11
*/
private static $_reusableData=array();
protected static $reusableData=array();

This comment has been minimized.

@klimov-paul

klimov-paul Apr 9, 2013

Member

Do not make field protected!
Add method "resetReusableData" to the CCacheDependency itself.

@klimov-paul

klimov-paul Apr 9, 2013

Member

Do not make field protected!
Add method "resetReusableData" to the CCacheDependency itself.

This comment has been minimized.

@marcovtwout

marcovtwout Apr 9, 2013

Contributor

Why not? The method is available just for the unit test, right? Making it protected only allows subclasses to deal with it, instead of public.

@marcovtwout

marcovtwout Apr 9, 2013

Contributor

Why not? The method is available just for the unit test, right? Making it protected only allows subclasses to deal with it, instead of public.

This comment has been minimized.

@klimov-paul

klimov-paul Apr 9, 2013

Member

There should NOT be a protected properties inside the framework!
This will make the core code harder to support!

The method "resetReusableData" is actually missing. Each private property should have some interface for set/get or, like in this case, reset its value.

Yes we are using it for the unit test, but that does not mean, there is none who may need it in the other place.

@klimov-paul

klimov-paul Apr 9, 2013

Member

There should NOT be a protected properties inside the framework!
This will make the core code harder to support!

The method "resetReusableData" is actually missing. Each private property should have some interface for set/get or, like in this case, reset its value.

Yes we are using it for the unit test, but that does not mean, there is none who may need it in the other place.

This comment has been minimized.

@marcovtwout

marcovtwout Apr 9, 2013

Contributor

Alrighty, I don't want to discuss the pros/cons of private/protected in Yii framework, I will revert it.

@marcovtwout

marcovtwout Apr 9, 2013

Contributor

Alrighty, I don't want to discuss the pros/cons of private/protected in Yii framework, I will revert it.

private $_hash;
private $_data;
@@ -53,9 +53,9 @@ public function evaluateDependency()
if ($this->reuseDependentData)
{
$hash=$this->getHash();
if (!isset(self::$_reusableData[$hash]['dependentData']))
self::$_reusableData[$hash]['dependentData']=$this->generateDependentData();
$this->_data=self::$_reusableData[$hash]['dependentData'];
if (!isset(self::$reusableData[$hash]['dependentData']))

This comment has been minimized.

@klimov-paul

klimov-paul Apr 9, 2013

Member

What happened here?
Why this is shown as a diff?

@klimov-paul

klimov-paul Apr 9, 2013

Member

What happened here?
Why this is shown as a diff?

This comment has been minimized.

@marcovtwout

marcovtwout Apr 9, 2013

Contributor

$_ -> $ (following the coding standard)

@marcovtwout

marcovtwout Apr 9, 2013

Contributor

$_ -> $ (following the coding standard)

This comment has been minimized.

@klimov-paul

klimov-paul Apr 9, 2013

Member

Code standard says:

Property names MUST start with an initial underscore if they are private.

@klimov-paul

klimov-paul Apr 9, 2013

Member

Code standard says:

Property names MUST start with an initial underscore if they are private.

This comment has been minimized.

@marcovtwout

marcovtwout Apr 9, 2013

Contributor

Yes, but it is a protected variable now. According to the example code, that means no underscore.

@marcovtwout

marcovtwout Apr 9, 2013

Contributor

Yes, but it is a protected variable now. According to the example code, that means no underscore.

self::$reusableData[$hash]['dependentData']=$this->generateDependentData();
$this->_data=self::$reusableData[$hash]['dependentData'];
}
else
$this->_data=$this->generateDependentData();
@@ -69,9 +69,9 @@ public function getHasChanged()
if ($this->reuseDependentData)
{
$hash=$this->getHash();
if (!isset(self::$_reusableData[$hash]['dependentData']))
self::$_reusableData[$hash]['dependentData']=$this->generateDependentData();
return self::$_reusableData[$hash]['dependentData']!=$this->_data;
if (!isset(self::$reusableData[$hash]['dependentData']))
self::$reusableData[$hash]['dependentData']=$this->generateDependentData();
return self::$reusableData[$hash]['dependentData']!=$this->_data;
}
else
return $this->generateDependentData()!=$this->_data;
@@ -86,14 +86,6 @@ public function getDependentData()
return $this->_data;
}
/**
* @see CCacheDependencyTest
*/
public static function resetReusableData()
{
self::$_reusableData=array();
}
/**
* Generates the data needed to determine if dependency has been changed.
* Derived classes should override this method to generate actual dependent data.
@@ -23,7 +23,7 @@ public function testReuseDependentData()
$dependency2=new MockCacheDependency();
$dependency2->reuseDependentData = true;
CCacheDependency::resetReusableData();
MockCacheDependency::resetReusableData();
$this->setCacheDependentData('start');
$dependency1->evaluateDependency();
$dependency2->evaluateDependency();
@@ -32,21 +32,21 @@ public function testReuseDependentData()
$this->assertEquals(1,MockCacheDependency::$generateDependentDataCalled,'Extra invokations of "generateDependentData()"!');
// New request:
CCacheDependency::resetReusableData();
MockCacheDependency::resetReusableData();
MockCacheDependency::$generateDependentDataCalled=0;
$this->assertFalse($dependency1->getHasChanged(),'Dependency1 changed for new request!');
$this->assertFalse($dependency2->getHasChanged(),'Dependency2 changed for new request!');
$this->assertEquals(1,MockCacheDependency::$generateDependentDataCalled,'Extra invokations of "generateDependentData()"!');
// New request:
CCacheDependency::resetReusableData();
MockCacheDependency::resetReusableData();
MockCacheDependency::$generateDependentDataCalled=0;
$this->setCacheDependentData('change1');
$this->assertTrue($dependency1->getHasChanged(),'Dependency1 is not changed after source change!');
$dependency1->evaluateDependency();
// New request:
CCacheDependency::resetReusableData();
MockCacheDependency::resetReusableData();
MockCacheDependency::$generateDependentDataCalled=0;
$this->assertFalse($dependency1->getHasChanged(),'Dependency1 has been changed!');
$this->assertTrue($dependency2->getHasChanged(),'Dependency2 has not been changed!');
@@ -64,4 +64,9 @@ public function generateDependentData()
self::$generateDependentDataCalled++;
return call_user_func(self::$generateDependentDataCallback);
}
public static function resetReusableData()
{
self::$reusableData=array();
}
}
ProTip! Use n and p to navigate between commits in a pull request.