From 7837481d21f37e6de9876124c014c05620bb6f3b Mon Sep 17 00:00:00 2001 From: Ian Morland Date: Wed, 11 Oct 2023 08:42:49 +0100 Subject: [PATCH 01/12] chore: create tests to highlight the conditional instantiation problem --- .../integration/extenders/ConditionalTest.php | 47 ++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/framework/core/tests/integration/extenders/ConditionalTest.php b/framework/core/tests/integration/extenders/ConditionalTest.php index ec96423e1f..bed5f6988d 100644 --- a/framework/core/tests/integration/extenders/ConditionalTest.php +++ b/framework/core/tests/integration/extenders/ConditionalTest.php @@ -12,9 +12,12 @@ use Exception; use Flarum\Api\Serializer\ForumSerializer; use Flarum\Extend; +use Flarum\Extend\ExtenderInterface; +use Flarum\Extension\Extension; use Flarum\Extension\ExtensionManager; use Flarum\Testing\integration\RetrievesAuthorizedUsers; use Flarum\Testing\integration\TestCase; +use Illuminate\Contracts\Container\Container; class ConditionalTest extends TestCase { @@ -144,7 +147,7 @@ public function conditional_injects_dependencies_to_condition_callable() $this->extend( (new Extend\Conditional()) ->when(function (?ExtensionManager $extensions) { - if (! $extensions) { + if (!$extensions) { throw new Exception('ExtensionManager not injected'); } }, [ @@ -159,4 +162,46 @@ public function conditional_injects_dependencies_to_condition_callable() $this->app(); } + + /** @test */ + public function conditional_does_not_instantiate_extender_if_condition_is_false() + { + $this->extend( + (new Extend\Conditional()) + ->when(false, [ + new TestExtender() + ]) + ); + + $this->app(); + } + + /** @test */ + public function conditional_does_instantiate_extender_if_condition_is_true() + { + $this->expectException(Exception::class); + $this->expectExceptionMessage('TestExtender was instantiated!'); + + $this->extend( + (new Extend\Conditional()) + ->when(true, [ + new TestExtender() + ]) + ); + + $this->app(); + } +} + +class TestExtender implements ExtenderInterface +{ + public function __construct() + { + throw new Exception('TestExtender was instantiated!'); + } + + public function extend(Container $container, Extension $extension = null) + { + // This method can be left empty for this test. + } } From 79493e44d8955e5d5654ff2c5211eaa041cf180f Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Wed, 11 Oct 2023 07:43:25 +0000 Subject: [PATCH 02/12] Apply fixes from StyleCI --- framework/core/tests/integration/extenders/ConditionalTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/framework/core/tests/integration/extenders/ConditionalTest.php b/framework/core/tests/integration/extenders/ConditionalTest.php index bed5f6988d..e68472d2c5 100644 --- a/framework/core/tests/integration/extenders/ConditionalTest.php +++ b/framework/core/tests/integration/extenders/ConditionalTest.php @@ -147,7 +147,7 @@ public function conditional_injects_dependencies_to_condition_callable() $this->extend( (new Extend\Conditional()) ->when(function (?ExtensionManager $extensions) { - if (!$extensions) { + if (! $extensions) { throw new Exception('ExtensionManager not injected'); } }, [ From 198229b0a99473e8a519e6a5451567692b6f6258 Mon Sep 17 00:00:00 2001 From: Ian Morland Date: Wed, 11 Oct 2023 12:14:59 +0100 Subject: [PATCH 03/12] add callback and invokable class + tests --- framework/core/src/Extend/Conditional.php | 50 ++++++- .../integration/extenders/ConditionalTest.php | 133 +++++++++++++++--- 2 files changed, 156 insertions(+), 27 deletions(-) diff --git a/framework/core/src/Extend/Conditional.php b/framework/core/src/Extend/Conditional.php index b9bbc764af..26202666c2 100644 --- a/framework/core/src/Extend/Conditional.php +++ b/framework/core/src/Extend/Conditional.php @@ -13,17 +13,36 @@ use Flarum\Extension\ExtensionManager; use Illuminate\Contracts\Container\Container; +/** + * The Conditional extender allows developers to conditionally apply other extenders + * based on either boolean values or results from callable functions. + * + * This is useful for applying extenders only if certain conditions are met, + * such as the presence of an enabled extension or a specific configuration setting. + * + * @package Flarum\Extend + */ class Conditional implements ExtenderInterface { /** + * An array of conditions and their associated extenders. + * + * Each entry should have: + * - 'condition': a boolean or callable that should return a boolean. + * - 'extenders': an array of extenders, a callable returning an array of extenders, or an invokable class string. + * * @var array */ protected $conditions = []; /** - * @param ExtenderInterface[] $extenders + * Apply extenders only if a specific extension is enabled. + * + * @param string $extensionId The ID of the extension. + * @param ExtenderInterface[]|callable|string $extenders An array of extenders, a callable returning an array of extenders, or an invokable class string. + * @return self */ - public function whenExtensionEnabled(string $extensionId, array $extenders): self + public function whenExtensionEnabled(string $extensionId, $extenders): self { return $this->when(function (ExtensionManager $extensions) use ($extensionId) { return $extensions->isEnabled($extensionId); @@ -31,10 +50,14 @@ public function whenExtensionEnabled(string $extensionId, array $extenders): sel } /** - * @param bool|callable $condition - * @param ExtenderInterface[] $extenders + * Apply extenders based on a condition. + * + * @param bool|callable $condition A boolean or callable that should return a boolean. + * If this evaluates to true, the extenders will be applied. + * @param ExtenderInterface[]|callable|string $extenders An array of extenders, a callable returning an array of extenders, or an invokable class string. + * @return self */ - public function when($condition, array $extenders): self + public function when($condition, $extenders): self { $this->conditions[] = [ 'condition' => $condition, @@ -44,6 +67,13 @@ public function when($condition, array $extenders): self return $this; } + /** + * Iterates over the conditions and applies the associated extenders if the conditions are met. + * + * @param Container $container + * @param Extension|null $extension + * @return void + */ public function extend(Container $container, Extension $extension = null) { foreach ($this->conditions as $condition) { @@ -52,7 +82,15 @@ public function extend(Container $container, Extension $extension = null) } if ($condition['condition']) { - foreach ($condition['extenders'] as $extender) { + $extenders = $condition['extenders']; + + if (is_string($extenders)) { + $extenders = $container->call($extenders); + } elseif (is_callable($extenders)) { + $extenders = $container->call($extenders); + } + + foreach ($extenders as $extender) { $extender->extend($container, $extension); } } diff --git a/framework/core/tests/integration/extenders/ConditionalTest.php b/framework/core/tests/integration/extenders/ConditionalTest.php index e68472d2c5..ce5291b80b 100644 --- a/framework/core/tests/integration/extenders/ConditionalTest.php +++ b/framework/core/tests/integration/extenders/ConditionalTest.php @@ -12,12 +12,9 @@ use Exception; use Flarum\Api\Serializer\ForumSerializer; use Flarum\Extend; -use Flarum\Extend\ExtenderInterface; -use Flarum\Extension\Extension; use Flarum\Extension\ExtensionManager; use Flarum\Testing\integration\RetrievesAuthorizedUsers; use Flarum\Testing\integration\TestCase; -use Illuminate\Contracts\Container\Container; class ConditionalTest extends TestCase { @@ -147,7 +144,7 @@ public function conditional_injects_dependencies_to_condition_callable() $this->extend( (new Extend\Conditional()) ->when(function (?ExtensionManager $extensions) { - if (! $extensions) { + if (!$extensions) { throw new Exception('ExtensionManager not injected'); } }, [ @@ -164,44 +161,138 @@ public function conditional_injects_dependencies_to_condition_callable() } /** @test */ - public function conditional_does_not_instantiate_extender_if_condition_is_false() + public function conditional_does_not_instantiate_extender_if_condition_is_false_using_callable() { $this->extend( (new Extend\Conditional()) - ->when(false, [ - new TestExtender() - ]) + ->when(false, TestExtender::class) ); $this->app(); + + $response = $this->send( + $this->request('GET', '/api', [ + 'authenticatedAs' => 1, + ]) + ); + + $payload = json_decode($response->getBody()->getContents(), true); + + $this->assertArrayNotHasKey('customConditionalAttribute', $payload['data']['attributes']); } /** @test */ - public function conditional_does_instantiate_extender_if_condition_is_true() + public function conditional_does_instantiate_extender_if_condition_is_true_using_callable() { - $this->expectException(Exception::class); - $this->expectExceptionMessage('TestExtender was instantiated!'); + $this->extend( + (new Extend\Conditional()) + ->when(true, TestExtender::class) + ); + + $this->app(); + + $response = $this->send( + $this->request('GET', '/api', [ + 'authenticatedAs' => 1, + ]) + ); + + $payload = json_decode($response->getBody()->getContents(), true); + $this->assertArrayHasKey('customConditionalAttribute', $payload['data']['attributes']); + } + + /** @test */ + public function conditional_does_not_instantiate_extender_if_condition_is_false_using_callback() + { $this->extend( (new Extend\Conditional()) - ->when(true, [ - new TestExtender() - ]) + ->when(false, function (): array { + return [ + (new Extend\ApiSerializer(ForumSerializer::class)) + ->attributes(function () { + return [ + 'customConditionalAttribute' => true + ]; + }) + ]; + }) ); $this->app(); + + $response = $this->send( + $this->request('GET', '/api', [ + 'authenticatedAs' => 1, + ]) + ); + + $payload = json_decode($response->getBody()->getContents(), true); + + $this->assertArrayNotHasKey('customConditionalAttribute', $payload['data']['attributes']); } -} -class TestExtender implements ExtenderInterface -{ - public function __construct() + /** @test */ + public function conditional_does_instantiate_extender_if_condition_is_true_using_callback() { - throw new Exception('TestExtender was instantiated!'); + $this->extend( + (new Extend\Conditional()) + ->when(true, function (): array { + return [ + (new Extend\ApiSerializer(ForumSerializer::class)) + ->attributes(function () { + return [ + 'customConditionalAttribute' => true + ]; + }) + ]; + }) + ); + + $this->app(); + + $response = $this->send( + $this->request('GET', '/api', [ + 'authenticatedAs' => 1, + ]) + ); + + $payload = json_decode($response->getBody()->getContents(), true); + + $this->assertArrayHasKey('customConditionalAttribute', $payload['data']['attributes']); } - public function extend(Container $container, Extension $extension = null) + /** @test */ + public function conditional_does_not_work_if_extension_is_disabled() + { + $this->extend( + (new Extend\Conditional()) + ->whenExtensionEnabled('dummy-extension-id', TestExtender::class) + ); + + $response = $this->send( + $this->request('GET', '/api', [ + 'authenticatedAs' => 1, + ]) + ); + + $payload = json_decode($response->getBody()->getContents(), true); + + $this->assertArrayNotHasKey('customConditionalAttribute', $payload['data']['attributes']); + } +} + +class TestExtender +{ + public function __invoke(): array { - // This method can be left empty for this test. + return [ + (new Extend\ApiSerializer(ForumSerializer::class)) + ->attributes(function () { + return [ + 'customConditionalAttribute' => true + ]; + }) + ]; } } From 3aa604eb254a4e35d333635e024a8fa9bf6b7803 Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Wed, 11 Oct 2023 11:15:11 +0000 Subject: [PATCH 04/12] Apply fixes from StyleCI --- framework/core/src/Extend/Conditional.php | 2 -- framework/core/tests/integration/extenders/ConditionalTest.php | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/framework/core/src/Extend/Conditional.php b/framework/core/src/Extend/Conditional.php index 26202666c2..55584beebe 100644 --- a/framework/core/src/Extend/Conditional.php +++ b/framework/core/src/Extend/Conditional.php @@ -19,8 +19,6 @@ * * This is useful for applying extenders only if certain conditions are met, * such as the presence of an enabled extension or a specific configuration setting. - * - * @package Flarum\Extend */ class Conditional implements ExtenderInterface { diff --git a/framework/core/tests/integration/extenders/ConditionalTest.php b/framework/core/tests/integration/extenders/ConditionalTest.php index ce5291b80b..2ee1512073 100644 --- a/framework/core/tests/integration/extenders/ConditionalTest.php +++ b/framework/core/tests/integration/extenders/ConditionalTest.php @@ -144,7 +144,7 @@ public function conditional_injects_dependencies_to_condition_callable() $this->extend( (new Extend\Conditional()) ->when(function (?ExtensionManager $extensions) { - if (!$extensions) { + if (! $extensions) { throw new Exception('ExtensionManager not injected'); } }, [ From 1fc2c8801a2b75f462251be8ae266e1699497f95 Mon Sep 17 00:00:00 2001 From: Ian Morland Date: Wed, 11 Oct 2023 12:32:31 +0100 Subject: [PATCH 05/12] address stan issue on php 8.2 --- framework/core/src/Extend/Conditional.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/framework/core/src/Extend/Conditional.php b/framework/core/src/Extend/Conditional.php index 55584beebe..f9098430a0 100644 --- a/framework/core/src/Extend/Conditional.php +++ b/framework/core/src/Extend/Conditional.php @@ -82,12 +82,15 @@ public function extend(Container $container, Extension $extension = null) if ($condition['condition']) { $extenders = $condition['extenders']; - if (is_string($extenders)) { - $extenders = $container->call($extenders); + if (is_string($extenders) && class_exists($extenders) && method_exists($extenders, '__invoke')) { + $result = $container->call($extenders); + $extenders = is_array($result) ? $result : [$result]; } elseif (is_callable($extenders)) { $extenders = $container->call($extenders); } + assert(is_array($extenders), 'Extenders should be an array after resolution.'); + foreach ($extenders as $extender) { $extender->extend($container, $extension); } From 79350732976b94ca41cf1c4a0c38fd8650e09a6a Mon Sep 17 00:00:00 2001 From: Ian Morland Date: Wed, 11 Oct 2023 16:51:31 +0100 Subject: [PATCH 06/12] Revert "address stan issue on php 8.2" This reverts commit 1fc2c8801a2b75f462251be8ae266e1699497f95. --- framework/core/src/Extend/Conditional.php | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/framework/core/src/Extend/Conditional.php b/framework/core/src/Extend/Conditional.php index f9098430a0..55584beebe 100644 --- a/framework/core/src/Extend/Conditional.php +++ b/framework/core/src/Extend/Conditional.php @@ -82,15 +82,12 @@ public function extend(Container $container, Extension $extension = null) if ($condition['condition']) { $extenders = $condition['extenders']; - if (is_string($extenders) && class_exists($extenders) && method_exists($extenders, '__invoke')) { - $result = $container->call($extenders); - $extenders = is_array($result) ? $result : [$result]; + if (is_string($extenders)) { + $extenders = $container->call($extenders); } elseif (is_callable($extenders)) { $extenders = $container->call($extenders); } - assert(is_array($extenders), 'Extenders should be an array after resolution.'); - foreach ($extenders as $extender) { $extender->extend($container, $extension); } From 1cc327bb3b1cc52273b18488f9cc926f43d8858e Mon Sep 17 00:00:00 2001 From: Ian Morland Date: Wed, 11 Oct 2023 16:57:16 +0100 Subject: [PATCH 07/12] attempt to make stan happy --- framework/core/src/Extend/Conditional.php | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/framework/core/src/Extend/Conditional.php b/framework/core/src/Extend/Conditional.php index 55584beebe..b74ce9c851 100644 --- a/framework/core/src/Extend/Conditional.php +++ b/framework/core/src/Extend/Conditional.php @@ -81,14 +81,15 @@ public function extend(Container $container, Extension $extension = null) if ($condition['condition']) { $extenders = $condition['extenders']; + $resolvedExtenders = []; - if (is_string($extenders)) { - $extenders = $container->call($extenders); - } elseif (is_callable($extenders)) { - $extenders = $container->call($extenders); + if (is_string($extenders) || is_callable($extenders)) { + $resolvedExtenders = $container->call($extenders); + } else { + $resolvedExtenders = $extenders; } - foreach ($extenders as $extender) { + foreach ($resolvedExtenders as $extender) { $extender->extend($container, $extension); } } From 4eba41a0bcbbd050b82b90e53b3b7bf2c848ca62 Mon Sep 17 00:00:00 2001 From: Ian Morland Date: Wed, 11 Oct 2023 17:11:55 +0100 Subject: [PATCH 08/12] Revert "attempt to make stan happy" This reverts commit 1cc327bb3b1cc52273b18488f9cc926f43d8858e. --- framework/core/src/Extend/Conditional.php | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/framework/core/src/Extend/Conditional.php b/framework/core/src/Extend/Conditional.php index b74ce9c851..55584beebe 100644 --- a/framework/core/src/Extend/Conditional.php +++ b/framework/core/src/Extend/Conditional.php @@ -81,15 +81,14 @@ public function extend(Container $container, Extension $extension = null) if ($condition['condition']) { $extenders = $condition['extenders']; - $resolvedExtenders = []; - if (is_string($extenders) || is_callable($extenders)) { - $resolvedExtenders = $container->call($extenders); - } else { - $resolvedExtenders = $extenders; + if (is_string($extenders)) { + $extenders = $container->call($extenders); + } elseif (is_callable($extenders)) { + $extenders = $container->call($extenders); } - foreach ($resolvedExtenders as $extender) { + foreach ($extenders as $extender) { $extender->extend($container, $extension); } } From 2006755cf17157e653c610e222377cac5f127e50 Mon Sep 17 00:00:00 2001 From: Ian Morland Date: Wed, 11 Oct 2023 17:16:51 +0100 Subject: [PATCH 09/12] is it really that simple? --- framework/core/src/Extend/Conditional.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/framework/core/src/Extend/Conditional.php b/framework/core/src/Extend/Conditional.php index 55584beebe..2d9caaab21 100644 --- a/framework/core/src/Extend/Conditional.php +++ b/framework/core/src/Extend/Conditional.php @@ -29,7 +29,7 @@ class Conditional implements ExtenderInterface * - 'condition': a boolean or callable that should return a boolean. * - 'extenders': an array of extenders, a callable returning an array of extenders, or an invokable class string. * - * @var array + * @var array */ protected $conditions = []; From 9d8b15ece4e5122567a7508af3f62f908567884f Mon Sep 17 00:00:00 2001 From: Ian Morland Date: Wed, 11 Oct 2023 17:19:34 +0100 Subject: [PATCH 10/12] Revert "is it really that simple?" This reverts commit 2006755cf17157e653c610e222377cac5f127e50. --- framework/core/src/Extend/Conditional.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/framework/core/src/Extend/Conditional.php b/framework/core/src/Extend/Conditional.php index 2d9caaab21..55584beebe 100644 --- a/framework/core/src/Extend/Conditional.php +++ b/framework/core/src/Extend/Conditional.php @@ -29,7 +29,7 @@ class Conditional implements ExtenderInterface * - 'condition': a boolean or callable that should return a boolean. * - 'extenders': an array of extenders, a callable returning an array of extenders, or an invokable class string. * - * @var array + * @var array */ protected $conditions = []; From 42136f03677d52ad8fae4aafd77a2e755e708066 Mon Sep 17 00:00:00 2001 From: Ian Morland Date: Wed, 11 Oct 2023 17:22:17 +0100 Subject: [PATCH 11/12] let's try this --- framework/core/src/Extend/Conditional.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/framework/core/src/Extend/Conditional.php b/framework/core/src/Extend/Conditional.php index 55584beebe..4241cf476e 100644 --- a/framework/core/src/Extend/Conditional.php +++ b/framework/core/src/Extend/Conditional.php @@ -29,7 +29,7 @@ class Conditional implements ExtenderInterface * - 'condition': a boolean or callable that should return a boolean. * - 'extenders': an array of extenders, a callable returning an array of extenders, or an invokable class string. * - * @var array + * @var array */ protected $conditions = []; From 9af214ce151c4429bb041d49a1107d67447602a6 Mon Sep 17 00:00:00 2001 From: IanM <16573496+imorland@users.noreply.github.com> Date: Thu, 12 Oct 2023 19:09:23 +0100 Subject: [PATCH 12/12] Update framework/core/src/Extend/Conditional.php Co-authored-by: Sami Mazouz --- framework/core/src/Extend/Conditional.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/framework/core/src/Extend/Conditional.php b/framework/core/src/Extend/Conditional.php index 4241cf476e..d0c1d699fe 100644 --- a/framework/core/src/Extend/Conditional.php +++ b/framework/core/src/Extend/Conditional.php @@ -82,9 +82,7 @@ public function extend(Container $container, Extension $extension = null) if ($condition['condition']) { $extenders = $condition['extenders']; - if (is_string($extenders)) { - $extenders = $container->call($extenders); - } elseif (is_callable($extenders)) { + if (is_string($extenders) || is_callable($extenders)) { $extenders = $container->call($extenders); }