Fix Callables for Extenders (#2423)

- Standardize signatures and variable names for extenders that take callbacks
- Adjust model extender docblock to clarify that default calue can't be an invokable class.
- Make invokable classes provided to Model->relationship
- Add integration tests to ensure Model->relationship and User->groupProcessor extenders accept callbacks
- Extract code for wrapping callbacks into central util

framework/core/src/Extend/Event.php

@@ -25,7 +25,7 @@ class Event implements ExtenderInterface
      *  - the class attribute of a class with a public `handle` method, which accepts an instance of the event as a parameter
      *
      * @param string $event
-     * @param callable $listener
+     * @param callable|string $listener
      */
     public function listen(string $event, $listener)
     {

framework/core/src/Extend/Formatter.php

@@ -11,6 +11,7 @@ namespace Flarum\Extend;
 
 use Flarum\Extension\Extension;
 use Flarum\Formatter\Formatter as ActualFormatter;
+use Flarum\Foundation\ContainerUtil;
 use Illuminate\Contracts\Container\Container;
 
 class Formatter implements ExtenderInterface, LifecycleInterface
@@ -83,27 +84,15 @@ class Formatter implements ExtenderInterface, LifecycleInterface
     {
         $container->extend('flarum.formatter', function ($formatter, $container) {
             foreach ($this->configurationCallbacks as $callback) {
-                if (is_string($callback)) {
-                    $callback = $container->make($callback);
-                }
-
-                $formatter->addConfigurationCallback($callback);
+                $formatter->addConfigurationCallback(ContainerUtil::wrapCallback($callback, $container));
             }
 
             foreach ($this->parsingCallbacks as $callback) {
-                if (is_string($callback)) {
-                    $callback = $container->make($callback);
-                }
-
-                $formatter->addParsingCallback($callback);
+                $formatter->addParsingCallback(ContainerUtil::wrapCallback($callback, $container));
             }
 
             foreach ($this->renderingCallbacks as $callback) {
-                if (is_string($callback)) {
-                    $callback = $container->make($callback);
-                }
-
-                $formatter->addRenderingCallback($callback);
+                $formatter->addRenderingCallback(ContainerUtil::wrapCallback($callback, $container));
             }
 
             return $formatter;

framework/core/src/Extend/Frontend.php

@@ -171,11 +171,7 @@ class Frontend implements ExtenderInterface
             "flarum.frontend.$this->frontend",
             function (ActualFrontend $frontend, Container $container) {
                 foreach ($this->content as $content) {
-                    if (is_string($content)) {
-                        $content = $container->make($content);
-                    }
-
-                    $frontend->content($content);
+                    $frontend->content(ContainerUtil::wrapCallback($content, $container));
                 }
             }
         );

framework/core/src/Extend/Model.php

@@ -11,12 +11,14 @@ namespace Flarum\Extend;
 
 use Flarum\Database\AbstractModel;
 use Flarum\Extension\Extension;
+use Flarum\Foundation\ContainerUtil;
 use Illuminate\Contracts\Container\Container;
 use Illuminate\Support\Arr;
 
 class Model implements ExtenderInterface
 {
     private $modelClass;
+    private $customRelations = [];
 
     /**
      * @param string $modelClass The ::class attribute of the model you are modifying.
@@ -48,7 +50,9 @@ class Model implements ExtenderInterface
     }
 
     /**
-     * Add a default value for a given attribute, which can be an explicit value, or a closure.
+     * Add a default value for a given attribute, which can be an explicit value, a closure,
+     * or an instance of an invokable class. Unlike with some other extenders,
+     * it CANNOT be the `::class` attribute of an invokable class.
      *
      * @param string $attribute
      * @param mixed $value
@@ -157,7 +161,7 @@ class Model implements ExtenderInterface
      * @param string $name: The name of the relation. This doesn't have to be anything in particular,
      *                      but has to be unique from other relation names for this model, and should
      *                      work as the name of a method.
-     * @param callable $callable
+     * @param callable|string $callback
      *
      * The callable can be a closure or invokable class, and should accept:
      * - $instance: An instance of this model.
@@ -168,15 +172,17 @@ class Model implements ExtenderInterface
      *
      * @return self
      */
-    public function relationship(string $name, callable $callable)
+    public function relationship(string $name, $callback)
     {
-        Arr::set(AbstractModel::$customRelations, "$this->modelClass.$name", $callable);
+        $this->customRelations[$name] = $callback;
 
         return $this;
     }
 
     public function extend(Container $container, Extension $extension = null)
     {
-        // Nothing needed here.
+        foreach ($this->customRelations as $name => $callback) {
+            Arr::set(AbstractModel::$customRelations, "$this->modelClass.$name", ContainerUtil::wrapCallback($callback, $container));
+        }
     }
 }

framework/core/src/Extend/User.php

@@ -35,7 +35,7 @@ class User implements ExtenderInterface
      * This can be used to give a user permissions for groups they aren't actually in, based on context.
      * It will not change the group badges displayed for the user.
      *
-     * @param callable $callable
+     * @param callable|string $callback
      *
      * The callable can be a closure or invokable class, and should accept:
      * - \Flarum\User\User $user: the user in question.
@@ -44,9 +44,9 @@ class User implements ExtenderInterface
      * The callable should return:
      * - array $groupIds: an array of ids for the groups the user belongs to.
      */
-    public function permissionGroups(callable $callable)
+    public function permissionGroups($callback)
     {
-        $this->groupProcessors[] = $callable;
+        $this->groupProcessors[] = $callback;
 
         return $this;
     }

framework/core/src/Extend/Validator.php

@@ -10,6 +10,7 @@
 namespace Flarum\Extend;
 
 use Flarum\Extension\Extension;
+use Flarum\Foundation\ContainerUtil;
 use Illuminate\Contracts\Container\Container;
 
 class Validator implements ExtenderInterface
@@ -47,11 +48,7 @@ class Validator implements ExtenderInterface
     {
         $container->resolving($this->validator, function ($validator, $container) {
             foreach ($this->configurationCallbacks as $callback) {
-                if (is_string($callback)) {
-                    $callback = $container->make($callback);
-                }
-
-                $validator->addConfiguration($callback);
+                $validator->addConfiguration(ContainerUtil::wrapCallback($callback, $container));
             }
         });
     }

framework/core/src/Foundation/ContainerUtil.php

@@ -0,0 +1,36 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Foundation;
+
+use Illuminate\Contracts\Container\Container;
+
+class ContainerUtil
+{
+    /**
+     * Wraps a callback so that string-based invokable classes get resolved only when actually used.
+     *
+     * @internal Backwards compatability not guaranteed.
+     *
+     * @param callable|string $callback: A callable, or a ::class attribute of an invokable class
+     * @param Container $container
+     */
+    public static function wrapCallback($callback, Container $container)
+    {
+        if (is_string($callback)) {
+            $callback = function () use ($container, $callback) {
+                $callback = $container->make($callback);
+
+                return call_user_func_array($callback, func_get_args());
+            };
+        }
+
+        return $callback;
+    }
+}

framework/core/src/User/UserServiceProvider.php

@@ -11,6 +11,7 @@ namespace Flarum\User;
 
 use Flarum\Event\ConfigureUserPreferences;
 use Flarum\Foundation\AbstractServiceProvider;
+use Flarum\Foundation\ContainerUtil;
 use Flarum\Settings\SettingsRepositoryInterface;
 use Flarum\User\DisplayName\DriverInterface;
 use Flarum\User\DisplayName\UsernameDriver;
@@ -77,11 +78,7 @@ class UserServiceProvider extends AbstractServiceProvider
     public function boot()
     {
         foreach ($this->app->make('flarum.user.group_processors') as $callback) {
-            if (is_string($callback)) {
-                $callback = $this->app->make($callback);
-            }
-
-            User::addGroupProcessor($callback);
+            User::addGroupProcessor(ContainerUtil::wrapCallback($callback, $this->app));
         }
 
         User::setHasher($this->app->make('hash'));

framework/core/tests/integration/extenders/ModelTest.php

@@ -134,6 +134,23 @@ class ModelTest extends TestCase
         $this->assertEquals([], $user->customRelation()->get()->toArray());
     }
 
+    /**
+     * @test
+     */
+    public function custom_relationship_can_be_invokable_class()
+    {
+        $this->extend(
+            (new Extend\Model(User::class))
+                ->relationship('customRelation', CustomRelationClass::class)
+        );
+
+        $this->prepDB();
+
+        $user = User::find(1);
+
+        $this->assertEquals([], $user->customRelation()->get()->toArray());
+    }
+
     /**
      * @test
      */
@@ -423,3 +440,11 @@ class ModelTestCustomPost extends AbstractEventPost
      */
     public static $type = 'customPost';
 }
+
+class CustomRelationClass
+{
+    public function __invoke(User $user)
+    {
+        return $user->hasMany(Discussion::class, 'user_id');
+    }
+}

framework/core/tests/integration/extenders/UserTest.php

@@ -88,6 +88,19 @@ class UserTest extends TestCase
 
         $this->assertNotContains('viewUserList', $user->getPermissions());
     }
+
+    /**
+     * @test
+     */
+    public function processor_can_be_invokable_class()
+    {
+        $this->extend((new Extend\User)->permissionGroups(CustomGroupProcessorClass::class));
+
+        $this->prepDb();
+        $user = User::find(2);
+
+        $this->assertNotContains('viewUserList', $user->getPermissions());
+    }
 }
 
 class CustomDisplayNameDriver implements DriverInterface
@@ -97,3 +110,13 @@ class CustomDisplayNameDriver implements DriverInterface
         return $user->email.'$$$suffix';
     }
 }
+
+class CustomGroupProcessorClass
+{
+    public function __invoke(User $user, array $groupIds)
+    {
+        return array_filter($groupIds, function ($id) {
+            return $id != 3;
+        });
+    }
+}