[OJS.de] Fix memory leak in hook registry

ojsde · May 1, 2013 · 7bc6547 · 7bc6547
1 parent cf0e8b1
commit 7bc6547
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 11 deletions.
diff --git a/classes/plugins/HookRegistry.inc.php b/classes/plugins/HookRegistry.inc.php
@@ -68,11 +68,16 @@ static function register($hookName, $callback) {
 	 * @return mixed
 	 */
 	static function call($hookName, $args = null) {
-		// Remember the called hooks for testing.
-		$calledHooks =& HookRegistry::getCalledHooks();
-		$calledHooks[] = array(
-			$hookName, $args
-		);
+		// For testing only.
+		// The implementation is a bit quirky as this has to work when
+		// executed statically.
+		if (self::rememberCalledHooks(true)) {
+			// Remember the called hooks for testing.
+			$calledHooks =& HookRegistry::getCalledHooks();
+			$calledHooks[] = array(
+				$hookName, $args
+			);
+		}
 
 		$hooks =& HookRegistry::getHooks();
 		if (!isset($hooks[$hookName])) {
@@ -92,13 +97,42 @@ static function call($hookName, $args = null) {
 	//
 	// Methods required for testing only.
 	//
-	static function resetCalledHooks() {
+	/**
+	 * Set/query the flag that triggers storing of
+	 * called hooks.
+	 * @param $askOnly boolean When set to true, the flag will not
+	 *   be changed but only returned.
+	 * @param $updateTo boolean When $askOnly is set to 'true' then
+	 *   this parameter defines the value of the flag.
+	 * @return boolean The current value of the flag.
+	 */
+	static function rememberCalledHooks($askOnly = false, $updateTo = true) {
+		static $rememberCalledHooks = false;
+		if (!$askOnly) {
+			$rememberCalledHooks = $updateTo;
+		}
+		return $rememberCalledHooks;
+	}
+
+	/**
+	 * Switch off the function to store hooks and delete all stored hooks.
+	 * Always call this after using otherwise we get a severe memory.
+	 * @param $leaveAlive boolean Set this to true if you only want to
+	 *   delete hooks stored so far but if you want to record future
+	 *   hook calls, too.
+	 */
+	static function resetCalledHooks($leaveAlive = false) {
+		if (!$leaveAlive) HookRegistry::rememberCalledHooks(false, false);
 		$calledHooks =& HookRegistry::getCalledHooks();
 		$calledHooks = array();
 	}
 
+	/**
+	 * Return a reference to the stored hooks.
+	 * @return array
+	 */
 	static function &getCalledHooks() {
-		static $calledHooks;
+		static $calledHooks = array();
 		return $calledHooks;
 	}
 }

diff --git a/tests/classes/core/PKPRequestTest.php b/tests/classes/core/PKPRequestTest.php
@@ -25,6 +25,7 @@ class PKPRequestTest extends PKPTestCase {
 
 	public function setUp() {
 		parent::setUp();
+		HookRegistry::rememberCalledHooks();
 		$this->request = new PKPRequest();
 	}
 

diff --git a/tests/classes/core/PKPRouterTestCase.inc.php b/tests/classes/core/PKPRouterTestCase.inc.php
@@ -32,9 +32,15 @@ class PKPRouterTestCase extends PKPTestCase {
 
 	protected function setUp() {
 		parent::setUp();
+		HookRegistry::rememberCalledHooks();
 		$this->router = new PKPRouter();
 	}
 
+	protected function tearDown() {
+		parent::tearDown();
+		HookRegistry::resetCalledHooks(true);
+	}
+
 	/**
 	 * @covers PKPRouter::getApplication
 	 * @covers PKPRouter::setApplication
@@ -106,7 +112,7 @@ public function testGetRequestedContextPathWithEmptyPathInfo() {
 	 */
 	public function testGetRequestedContextPathWithFullPathInfo() {
 		$this->_setUpMockEnvironment(self::PATHINFO_ENABLED);
-		HookRegistry::resetCalledHooks();
+		HookRegistry::resetCalledHooks(true);
 		$_SERVER['PATH_INFO'] = '/context1/context2/other/path/vars';
 		self::assertEquals(array('context1', 'context2'),
 				$this->router->getRequestedContextPaths($this->request));
@@ -157,7 +163,7 @@ public function testGetRequestedContextPathWithEmptyContextParameters() {
 	 */
 	public function testGetRequestedContextPathWithFullContextParameters() {
 		$this->_setUpMockEnvironment(self::PATHINFO_DISABLED);
-		HookRegistry::resetCalledHooks();
+		HookRegistry::resetCalledHooks(true);
 		$_GET['firstContext'] = 'context1';
 		$_GET['secondContext'] = 'context2';
 		self::assertEquals(array('context1', 'context2'),
@@ -240,7 +246,7 @@ public function testGetIndexUrl() {
 			'HOSTNAME' => 'mydomain.org',
 			'SCRIPT_NAME' => '/base/index.php'
 		);
-		HookRegistry::resetCalledHooks();
+		HookRegistry::resetCalledHooks(true);
 
 		self::assertEquals('http://mydomain.org/base/index.php', $this->router->getIndexUrl($this->request));
 
@@ -258,7 +264,7 @@ public function testGetIndexUrl() {
 
 		// Calling getIndexUrl() twice should return the same
 		// result without triggering the hooks again.
-		HookRegistry::resetCalledHooks();
+		HookRegistry::resetCalledHooks(true);
 		self::assertEquals('http://mydomain.org/base/index.php', $this->router->getIndexUrl($this->request));
 		self::assertEquals(
 			array(),