From 76fc8f1596f5d9e82acbf5b81a5eebb2e868a5dd Mon Sep 17 00:00:00 2001
From: Damian Mooyman <damian@silverstripe.com>
Date: Fri, 12 Jan 2018 14:53:25 +1300
Subject: [PATCH 1/2] API Only expose public extension methods

API Enable `extend*` methods to handle ->extend()
API Remove Extensible::constructExtensions()
---
 src/Core/CustomMethods.php | 203 +++++++++++++++++++------------------
 src/Core/Extensible.php    |  63 ++++--------
 2 files changed, 127 insertions(+), 139 deletions(-)

diff --git a/src/Core/CustomMethods.php b/src/Core/CustomMethods.php
index ee6ea63ef..ed247b333 100644
--- a/src/Core/CustomMethods.php
+++ b/src/Core/CustomMethods.php
@@ -4,18 +4,20 @@ namespace SilverStripe\Core;
 
 use BadMethodCallException;
 use InvalidArgumentException;
-use SilverStripe\Dev\Deprecation;
+use ReflectionClass;
+use ReflectionMethod;
 
 /**
  * Allows an object to declare a set of custom methods
  */
 trait CustomMethods
 {
-
     /**
      * Custom method sources
      *
-     * @var array
+     * @var array Array of class names (lowercase) to list of methods.
+     * The list of methods will have lowercase keys. Each value in this array
+     * can be a callable, array, or string callback
      */
     protected static $extra_methods = [];
 
@@ -27,9 +29,10 @@ trait CustomMethods
     protected $extra_method_registers = [];
 
     /**
-     * Non-custom methods
+     * Non-custom public methods.
      *
-     * @var array
+     * @var array Array of class names (lowercase) to list of methods.
+     * The list of methods will have lowercase keys and correct-case values.
      */
     protected static $built_in_methods = [];
 
@@ -74,17 +77,16 @@ trait CustomMethods
                     );
                 }
 
-                // Call without setOwner
-                if (empty($config['callSetOwnerFirst'])) {
-                    return $obj->$method(...$arguments);
-                }
-
-                /** @var Extension $obj */
+                // Call on object
                 try {
-                    $obj->setOwner($this);
+                    if ($obj instanceof Extension) {
+                        $obj->setOwner($this);
+                    }
                     return $obj->$method(...$arguments);
                 } finally {
-                    $obj->clearOwner();
+                    if ($obj instanceof Extension) {
+                        $obj->clearOwner();
+                    }
                 }
             }
             case isset($config['wrap']): {
@@ -160,66 +162,87 @@ trait CustomMethods
             return null;
         }
         // Lazy define methods
-        if (!isset(self::$extra_methods[static::class])) {
+        $lowerClass = strtolower(static::class);
+        if (!isset(self::$extra_methods[$lowerClass])) {
             $this->defineMethods();
         }
 
-        if (isset(self::$extra_methods[static::class][strtolower($method)])) {
-            return self::$extra_methods[static::class][strtolower($method)];
-        }
-        return null;
+        return self::$extra_methods[$lowerClass][strtolower($method)] ?? null;
     }
 
     /**
      * Return the names of all the methods available on this object
      *
      * @param bool $custom include methods added dynamically at runtime
-     * @return array
+     * @return array Map of method names with lowercase keys
      */
     public function allMethodNames($custom = false)
     {
-        $class = static::class;
-        if (!isset(self::$built_in_methods[$class])) {
-            self::$built_in_methods[$class] = array_map('strtolower', get_class_methods($this ?? ''));
-        }
+        $methods = static::findBuiltInMethods();
 
-        if ($custom && isset(self::$extra_methods[$class])) {
-            return array_merge(self::$built_in_methods[$class], array_keys(self::$extra_methods[$class] ?? []));
-        } else {
-            return self::$built_in_methods[$class];
-        }
-    }
-
-    /**
-     * @param object $extension
-     * @return array
-     */
-    protected function findMethodsFromExtension($extension)
-    {
-        if (method_exists($extension, 'allMethodNames')) {
-            if ($extension instanceof Extension) {
-                try {
-                    $extension->setOwner($this);
-                    $methods = $extension->allMethodNames(true);
-                } finally {
-                    $extension->clearOwner();
-                }
-            } else {
-                $methods = $extension->allMethodNames(true);
-            }
-        } else {
-            $class = get_class($extension);
-            if (!isset(self::$built_in_methods[$class])) {
-                self::$built_in_methods[$class] = array_map('strtolower', get_class_methods($extension ?? ''));
-            }
-            $methods = self::$built_in_methods[$class];
+        // Query extra methods
+        $lowerClass = strtolower(static::class);
+        if ($custom && isset(self::$extra_methods[$lowerClass])) {
+            $methods = array_merge(self::$extra_methods[$lowerClass], $methods);
         }
 
         return $methods;
     }
 
     /**
-     * Add all the methods from an object property (which is an {@link Extension}) to this object.
+     * Get all public built in methods for this class
+     *
+     * @param string|object $class Class or instance to query methods from (defaults to static::class)
+     * @return array Map of methods with lowercase key name
+     */
+    protected static function findBuiltInMethods($class = null)
+    {
+        $class = is_object($class) ? get_class($class) : ($class ?: static::class);
+        $lowerClass = strtolower($class);
+        if (isset(self::$built_in_methods[$lowerClass])) {
+            return self::$built_in_methods[$lowerClass];
+        }
+
+        // Build new list
+        $reflection = new ReflectionClass($class);
+        $methods = $reflection->getMethods(ReflectionMethod::IS_PUBLIC);
+        self::$built_in_methods[$lowerClass] = [];
+        foreach ($methods as $method) {
+            $name = $method->getName();
+            self::$built_in_methods[$lowerClass][strtolower($name)] = $name;
+        }
+        return self::$built_in_methods[$lowerClass];
+    }
+
+    /**
+     * Find all methods on the given object.
+     *
+     * @param object $object
+     * @return array
+     */
+    protected function findMethodsFrom($object)
+    {
+        // Respect "allMethodNames"
+        if (method_exists($object, 'allMethodNames')) {
+            if ($object instanceof Extension) {
+                try {
+                    $object->setOwner($this);
+                    $methods = $object->allMethodNames(true);
+                } finally {
+                    $object->clearOwner();
+                }
+            } else {
+                $methods = $object->allMethodNames(true);
+            }
+            return $methods;
+        }
+
+        // Get methods
+        return static::findBuiltInMethods($object);
+    }
+
+    /**
+     * Add all the methods from an object property.
      *
      * @param string $property the property name
      * @param string|int $index an index to use if the property is an array
@@ -228,37 +251,31 @@ trait CustomMethods
     protected function addMethodsFrom($property, $index = null)
     {
         $class = static::class;
-        $extension = ($index !== null) ? $this->{$property}[$index] : $this->$property;
+        $object = ($index !== null) ? $this->{$property}[$index] : $this->$property;
 
-        if (!$extension) {
+        if (!$object) {
             throw new InvalidArgumentException(
                 "Object->addMethodsFrom(): could not add methods from {$class}->{$property}[$index]"
             );
         }
 
-        $methods = $this->findMethodsFromExtension($extension);
-        if ($methods) {
-            if ($extension instanceof Extension) {
-                Deprecation::notice(
-                    '5.0',
-                    'Register custom methods from extensions with addCallbackMethod.'
-                    . ' callSetOwnerFirst will be removed in 5.0'
-                );
-            }
-            $methodInfo = [
-                'property' => $property,
-                'index' => $index,
-                'callSetOwnerFirst' => $extension instanceof Extension,
-            ];
+        $methods = $this->findMethodsFrom($object);
+        if (!$methods) {
+            return;
+        }
+        $methodInfo = [
+            'property' => $property,
+            'index' => $index,
+        ];
 
-            $newMethods = array_fill_keys($methods ?? [], $methodInfo);
+        $newMethods = array_fill_keys(array_keys($methods), $methodInfo);
 
-            if (isset(self::$extra_methods[$class])) {
-                self::$extra_methods[$class] =
-                    array_merge(self::$extra_methods[$class], $newMethods);
-            } else {
-                self::$extra_methods[$class] = $newMethods;
-            }
+        // Merge with extra_methods
+        $lowerClass = strtolower($class);
+        if (isset(self::$extra_methods[$lowerClass])) {
+            self::$extra_methods[$lowerClass] = array_merge(self::$extra_methods[$lowerClass], $newMethods);
+        } else {
+            self::$extra_methods[$lowerClass] = $newMethods;
         }
     }
 
@@ -279,26 +296,18 @@ trait CustomMethods
             );
         }
 
-        $methods = $this->findMethodsFromExtension($extension);
-        if ($methods) {
-            foreach ($methods as $method) {
-                if (!isset(self::$extra_methods[$class][$method])) {
-                    continue;
-                }
+        $lowerClass = strtolower($class);
+        if (!isset(self::$extra_methods[$lowerClass])) {
+            return;
+        }
+        $methods = $this->findMethodsFrom($extension);
 
-                $methodInfo = self::$extra_methods[$class][$method];
+        // Unset by key
+        self::$extra_methods[$lowerClass] = array_diff_key(self::$extra_methods[$lowerClass], $methods);
 
-                // always check for property, AND
-                // check for index only if provided
-                if ((isset($methodInfo['property']) && $methodInfo['property'] === $property) &&
-                    (!$index || ($index && isset($methodInfo['index']) && $methodInfo['index'] === $index))) {
-                    unset(self::$extra_methods[$class][$method]);
-                }
-            }
-
-            if (empty(self::$extra_methods[$class])) {
-                unset(self::$extra_methods[$class]);
-            }
+        // Clear empty list
+        if (empty(self::$extra_methods[$lowerClass])) {
+            unset(self::$extra_methods[$lowerClass]);
         }
     }
 
@@ -311,7 +320,7 @@ trait CustomMethods
      */
     protected function addWrapperMethod($method, $wrap)
     {
-        self::$extra_methods[static::class][strtolower($method)] = [
+        self::$extra_methods[strtolower(static::class)][strtolower($method)] = [
             'wrap' => $wrap,
             'method' => $method
         ];
@@ -326,7 +335,7 @@ trait CustomMethods
      */
     protected function addCallbackMethod($method, $callback)
     {
-        self::$extra_methods[static::class][strtolower($method)] = [
+        self::$extra_methods[strtolower(static::class)][strtolower($method)] = [
             'callback' => $callback,
         ];
     }
diff --git a/src/Core/Extensible.php b/src/Core/Extensible.php
index 37e79a185..c9b59e1c4 100644
--- a/src/Core/Extensible.php
+++ b/src/Core/Extensible.php
@@ -104,14 +104,6 @@ trait Extensible
         $this->afterExtendCallbacks[$method][] = $callback;
     }
 
-    /**
-     * @deprecated 4.0.0:5.0.0 Extensions and methods are now lazy-loaded
-     */
-    protected function constructExtensions()
-    {
-        Deprecation::notice('5.0', 'constructExtensions does not need to be invoked and will be removed in 5.0');
-    }
-
     protected function defineMethods()
     {
         $this->defineMethodsCustom();
@@ -131,7 +123,7 @@ trait Extensible
     {
         $extensions = $this->getExtensionInstances();
         foreach ($extensions as $extensionClass => $extensionInstance) {
-            foreach ($this->findMethodsFromExtension($extensionInstance) as $method) {
+            foreach ($this->findMethodsFrom($extensionInstance) as $method) {
                 $this->addCallbackMethod($method, function ($inst, $args) use ($method, $extensionClass) {
                     /** @var Extensible $inst */
                     $extension = $inst->getExtensionInstance($extensionClass);
@@ -199,11 +191,8 @@ trait Extensible
         // unset some caches
         $subclasses = ClassInfo::subclassesFor($class);
         $subclasses[] = $class;
-
-        if ($subclasses) {
-            foreach ($subclasses as $subclass) {
-                unset(self::$extra_methods[$subclass]);
-            }
+        foreach ($subclasses as $subclass) {
+            unset(self::$extra_methods[strtolower($subclass)]);
         }
 
         Config::modify()
@@ -261,10 +250,8 @@ trait Extensible
         // unset some caches
         $subclasses = ClassInfo::subclassesFor($class);
         $subclasses[] = $class;
-        if ($subclasses) {
-            foreach ($subclasses as $subclass) {
-                unset(self::$extra_methods[$subclass]);
-            }
+        foreach ($subclasses as $subclass) {
+            unset(self::$extra_methods[strtolower($subclass)]);
         }
     }
 
@@ -403,25 +390,19 @@ trait Extensible
      * all results into an array
      *
      * @param string $method the method name to call
-     * @param mixed $a1
-     * @param mixed $a2
-     * @param mixed $a3
-     * @param mixed $a4
-     * @param mixed $a5
-     * @param mixed $a6
-     * @param mixed $a7
+     * @param mixed ...$arguments List of arguments
      * @return array List of results with nulls filtered out
      */
-    public function invokeWithExtensions($method, &$a1 = null, &$a2 = null, &$a3 = null, &$a4 = null, &$a5 = null, &$a6 = null, &$a7 = null)
+    public function invokeWithExtensions($method, &...$arguments)
     {
         $result = [];
         if (method_exists($this, $method ?? '')) {
-            $thisResult = $this->$method($a1, $a2, $a3, $a4, $a5, $a6, $a7);
+            $thisResult = $this->$method(...$arguments);
             if ($thisResult !== null) {
                 $result[] = $thisResult;
             }
         }
-        $extras = $this->extend($method, $a1, $a2, $a3, $a4, $a5, $a6, $a7);
+        $extras = $this->extend($method, ...$arguments);
 
         return $extras ? array_merge($result, $extras) : $result;
     }
@@ -438,22 +419,16 @@ trait Extensible
      * The extension methods are defined during {@link __construct()} in {@link defineMethods()}.
      *
      * @param string $method the name of the method to call on each extension
-     * @param mixed $a1
-     * @param mixed $a2
-     * @param mixed $a3
-     * @param mixed $a4
-     * @param mixed $a5
-     * @param mixed $a6
-     * @param mixed $a7
+     * @param mixed ...$arguments
      * @return array
      */
-    public function extend($method, &$a1 = null, &$a2 = null, &$a3 = null, &$a4 = null, &$a5 = null, &$a6 = null, &$a7 = null)
+    public function extend($method, &...$arguments)
     {
         $values = [];
 
         if (!empty($this->beforeExtendCallbacks[$method])) {
-            foreach (array_reverse($this->beforeExtendCallbacks[$method] ?? []) as $callback) {
-                $value = call_user_func_array($callback, [&$a1, &$a2, &$a3, &$a4, &$a5, &$a6, &$a7]);
+            foreach (array_reverse($this->beforeExtendCallbacks[$method ?? '']) as $callback) {
+                $value = call_user_func_array($callback, $arguments);
                 if ($value !== null) {
                     $values[] = $value;
                 }
@@ -462,10 +437,14 @@ trait Extensible
         }
 
         foreach ($this->getExtensionInstances() as $instance) {
-            if (method_exists($instance, $method ?? '')) {
+            // Prefer `extend` prefixed methods
+            $instanceMethod = method_exists($instance, "extend{$method}")
+                ? "extend{$method}"
+                : (method_exists($instance, $method) ? $method : null);
+            if ($instanceMethod) {
                 try {
                     $instance->setOwner($this);
-                    $value = $instance->$method($a1, $a2, $a3, $a4, $a5, $a6, $a7);
+                    $value = $instance->$instanceMethod(...$arguments);
                 } finally {
                     $instance->clearOwner();
                 }
@@ -476,8 +455,8 @@ trait Extensible
         }
 
         if (!empty($this->afterExtendCallbacks[$method])) {
-            foreach (array_reverse($this->afterExtendCallbacks[$method] ?? []) as $callback) {
-                $value = call_user_func_array($callback, [&$a1, &$a2, &$a3, &$a4, &$a5, &$a6, &$a7]);
+            foreach (array_reverse($this->afterExtendCallbacks[$method ?? '']) as $callback) {
+                $value = call_user_func_array($callback, $arguments);
                 if ($value !== null) {
                     $values[] = $value;
                 }

From f2211d690f292886a29e1a5d4117cd9cdc7fe840 Mon Sep 17 00:00:00 2001
From: Damian Mooyman <damian@silverstripe.com>
Date: Tue, 20 Feb 2018 13:48:20 +1300
Subject: [PATCH 2/2] BUG Fix extend() failing on protected extend-prefixed
 methods

---
 src/Core/Extensible.php | 18 ++++--------------
 src/Core/Extension.php  | 27 +++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/src/Core/Extensible.php b/src/Core/Extensible.php
index c9b59e1c4..526bd4101 100644
--- a/src/Core/Extensible.php
+++ b/src/Core/Extensible.php
@@ -419,7 +419,7 @@ trait Extensible
      * The extension methods are defined during {@link __construct()} in {@link defineMethods()}.
      *
      * @param string $method the name of the method to call on each extension
-     * @param mixed ...$arguments
+     * @param mixed &...$arguments
      * @return array
      */
     public function extend($method, &...$arguments)
@@ -438,19 +438,9 @@ trait Extensible
 
         foreach ($this->getExtensionInstances() as $instance) {
             // Prefer `extend` prefixed methods
-            $instanceMethod = method_exists($instance, "extend{$method}")
-                ? "extend{$method}"
-                : (method_exists($instance, $method) ? $method : null);
-            if ($instanceMethod) {
-                try {
-                    $instance->setOwner($this);
-                    $value = $instance->$instanceMethod(...$arguments);
-                } finally {
-                    $instance->clearOwner();
-                }
-                if ($value !== null) {
-                    $values[] = $value;
-                }
+            $value = $instance->invokeExtension($this, $method, ...$arguments);
+            if ($value !== null) {
+                $values[] = $value;
             }
         }
 
diff --git a/src/Core/Extension.php b/src/Core/Extension.php
index 3963bbdc0..6cf12c79d 100644
--- a/src/Core/Extension.php
+++ b/src/Core/Extension.php
@@ -115,4 +115,31 @@ abstract class Extension
         // Split out both args and service name
         return strtok(strtok($extensionStr ?? '', '(') ?? '', '.');
     }
+
+    /**
+     * Invoke extension point. This will prefer explicit `extend` prefixed
+     * methods.
+     *
+     * @param object $owner
+     * @param string $method
+     * @param array &...$arguments
+     * @return mixed
+     */
+    public function invokeExtension($owner, $method, &...$arguments)
+    {
+        // Prefer `extend` prefixed methods
+        $instanceMethod = method_exists($this, "extend{$method}")
+            ? "extend{$method}"
+            : (method_exists($this, $method) ? $method : null);
+        if (!$instanceMethod) {
+            return null;
+        }
+
+        try {
+            $this->setOwner($owner);
+            return $this->$instanceMethod(...$arguments);
+        } finally {
+            $this->clearOwner();
+        }
+    }
 }