Merge pull request #5662 from kinglozzer/viewabledata-definemethods-pt2

FIX: ViewableData::setFailover() didn't remove cached methods

core/Object.php

@@ -800,6 +800,25 @@ abstract class Object {
 		}
 	}
 
+	/**
+	 * @param object $extension
+	 * @return array
+	 */
+	protected function findMethodsFromExtension($extension) {
+		if (method_exists($extension, 'allMethodNames')) {
+			if ($extension instanceof Extension) $extension->setOwner($this);
+			$methods = $extension->allMethodNames(true);
+			if ($extension instanceof Extension) $extension->clearOwner();
+		} else {
+			if (!isset(self::$built_in_methods[$extension->class])) {
+				self::$built_in_methods[$extension->class] = array_map('strtolower', get_class_methods($extension));
+			}
+			$methods = self::$built_in_methods[$extension->class];
+		}
+
+		return $methods;
+	}
+
 	/**
 	 * Add all the methods from an object property (which is an {@link Extension}) to this object.
 	 *
@@ -815,19 +834,8 @@ abstract class Object {
 			);
 		}
 
-		if(method_exists($extension, 'allMethodNames')) {
+		$methods = $this->findMethodsFromExtension($extension);
-			if ($extension instanceof Extension) $extension->setOwner($this);
+		if ($methods) {
-			$methods = $extension->allMethodNames(true);
-			if ($extension instanceof Extension) $extension->clearOwner();
-
-		} else {
-			if(!isset(self::$built_in_methods[$extension->class])) {
-				self::$built_in_methods[$extension->class] = array_map('strtolower', get_class_methods($extension));
-			}
-			$methods = self::$built_in_methods[$extension->class];
-		}
-
-		if($methods) {
 			$methodInfo = array(
 				'property' => $property,
 				'index'    => $index,
@@ -845,6 +853,37 @@ abstract class Object {
 		}
 	}
 
+	/**
+	 * Add all the methods from an object property (which is an {@link Extension}) to this object.
+	 *
+	 * @param string $property the property name
+	 * @param string|int $index an index to use if the property is an array
+	 */
+	protected function removeMethodsFrom($property, $index = null) {
+		$extension = ($index !== null) ? $this->{$property}[$index] : $this->$property;
+
+		if(!$extension) {
+			throw new InvalidArgumentException (
+				"Object->removeMethodsFrom(): could not remove methods from {$this->class}->{$property}[$index]"
+			);
+		}
+
+		$methods = $this->findMethodsFromExtension($extension);
+		if ($methods) {
+			foreach ($methods as $method) {
+				$methodInfo = self::$extra_methods[$this->class][$method];
+
+				if ($methodInfo['property'] === $property && $methodInfo['index'] === $index) {
+					unset(self::$extra_methods[$this->class][$method]);
+				}
+			}
+
+			if (empty(self::$extra_methods[$this->class])) {
+				unset(self::$extra_methods[$this->class]);
+			}
+		}
+	}
+
 	/**
 	 * Add a wrapper method - a method which points to another method with a different name. For example, Thumbnail(x)
 	 * can be wrapped to generateThumbnail(x)

tests/view/ViewableDataTest.php

@@ -187,6 +187,11 @@ class ViewableDataTest extends SapphireTest {
 		// Ensure that defined methods detected from the failover aren't cached when setting a new failover
 		$container->setFailover(new ViewableDataTest_Failover);
 		$this->assertTrue($container->hasMethod('testMethod'));
+
+		// Test the reverse - that defined methods previously detected in a failover are removed if they no longer exist
+		$container->setFailover($failover);
+		$this->assertSame($failover, $container->getFailover(), 'getFailover() returned a different object');
+		$this->assertFalse($container->hasMethod('testMethod'), 'testMethod() incorrectly reported as existing');
 	}
 
 }

view/ViewableData.php

@@ -134,6 +134,11 @@ class ViewableData extends Object implements IteratorAggregate {
 	 * @param ViewableData $failover
 	 */
 	public function setFailover(ViewableData $failover) {
+		// Ensure cached methods from previous failover are removed
+		if ($this->failover) {
+			$this->removeMethodsFrom('failover');
+		}
+
 		$this->failover = $failover;
 		$this->defineMethods();
 	}