From 47d2eee9ce199b691ebda77143a6db5270d0e5bc Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov <38059171+askvortsov1@users.noreply.github.com> Date: Sun, 8 Nov 2020 21:36:38 -0500 Subject: [PATCH] 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 --- src/Extend/Event.php | 2 +- src/Extend/Formatter.php | 19 +++--------- src/Extend/Frontend.php | 6 +--- src/Extend/Model.php | 16 ++++++---- src/Extend/User.php | 6 ++-- src/Extend/Validator.php | 7 ++--- src/Foundation/ContainerUtil.php | 36 +++++++++++++++++++++++ src/User/UserServiceProvider.php | 7 ++--- tests/integration/extenders/ModelTest.php | 25 ++++++++++++++++ tests/integration/extenders/UserTest.php | 23 +++++++++++++++ 10 files changed, 108 insertions(+), 39 deletions(-) create mode 100644 src/Foundation/ContainerUtil.php diff --git a/src/Extend/Event.php b/src/Extend/Event.php index 8cb90e59a..152ed71ec 100644 --- a/src/Extend/Event.php +++ b/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) { diff --git a/src/Extend/Formatter.php b/src/Extend/Formatter.php index 98041514c..5a2dd62bd 100644 --- a/src/Extend/Formatter.php +++ b/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; diff --git a/src/Extend/Frontend.php b/src/Extend/Frontend.php index 3fd5e8554..f1680d2aa 100644 --- a/src/Extend/Frontend.php +++ b/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)); } } ); diff --git a/src/Extend/Model.php b/src/Extend/Model.php index aae9e8762..b0912fded 100644 --- a/src/Extend/Model.php +++ b/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)); + } } } diff --git a/src/Extend/User.php b/src/Extend/User.php index 130f19249..58f5cc9ff 100644 --- a/src/Extend/User.php +++ b/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; } diff --git a/src/Extend/Validator.php b/src/Extend/Validator.php index 62301e93c..4b8d2a619 100644 --- a/src/Extend/Validator.php +++ b/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)); } }); } diff --git a/src/Foundation/ContainerUtil.php b/src/Foundation/ContainerUtil.php new file mode 100644 index 000000000..bc566802d --- /dev/null +++ b/src/Foundation/ContainerUtil.php @@ -0,0 +1,36 @@ +make($callback); + + return call_user_func_array($callback, func_get_args()); + }; + } + + return $callback; + } +} diff --git a/src/User/UserServiceProvider.php b/src/User/UserServiceProvider.php index 5cb6d8bde..ae5658dab 100644 --- a/src/User/UserServiceProvider.php +++ b/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')); diff --git a/tests/integration/extenders/ModelTest.php b/tests/integration/extenders/ModelTest.php index 608c7aadc..6f7959e13 100644 --- a/tests/integration/extenders/ModelTest.php +++ b/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'); + } +} diff --git a/tests/integration/extenders/UserTest.php b/tests/integration/extenders/UserTest.php index 1a897c719..7e00b7d26 100644 --- a/tests/integration/extenders/UserTest.php +++ b/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; + }); + } +}