From b642d8e06339c61b1ea673d831d3b630ec7bfd21 Mon Sep 17 00:00:00 2001 From: Sean Fisher Date: Wed, 13 Sep 2023 11:40:42 -0400 Subject: [PATCH] Ensure that attribute and action methods are deduplicated (#447) * Ensure that attribute and action methods are deduplicated * Remove a legacy attribute version check * Add light tests for typehinting events to ease my mind --- .../events/class-discover-events.php | 33 +++++----- .../support/attributes/class-action.php | 6 +- src/mantle/support/class-service-provider.php | 66 +++++++++++++------ .../test-wordpress-action-dispatcher.php | 20 ++++++ tests/support/test-service-provider.php | 47 ++++++++++--- 5 files changed, 122 insertions(+), 50 deletions(-) diff --git a/src/mantle/framework/events/class-discover-events.php b/src/mantle/framework/events/class-discover-events.php index 7dafadfeb..4c3b695fd 100644 --- a/src/mantle/framework/events/class-discover-events.php +++ b/src/mantle/framework/events/class-discover-events.php @@ -20,8 +20,6 @@ /** * Discover events within a specific directory. - * - * @todo Add support for WordPress hooks using attributes. */ class Discover_Events { /** @@ -80,23 +78,22 @@ protected static function get_listener_events( $listeners, string $base_path ): } foreach ( $listener->getMethods( ReflectionMethod::IS_PUBLIC ) as $method ) { - // Check for attribute support with PHP 8. - if ( version_compare( phpversion(), '8.0.0', '>=' ) ) { - // Check if the method has an attribute action. - $action_attributes = $method->getAttributes( Action::class ); - - if ( ! empty( $action_attributes ) ) { - foreach ( $action_attributes as $attribute ) { - $instance = $attribute->newInstance(); - - $listener_events[ $listener->name . '@' . $method->name ] = [ - [ $instance->action ], - $instance->priority, - ]; - } - - continue; + // Check if the method has an attribute action. + $action_attributes = $method->getAttributes( Action::class ); + + if ( ! empty( $action_attributes ) ) { + foreach ( $action_attributes as $attribute ) { + $instance = $attribute->newInstance(); + + $listener_events[ $listener->name . '@' . $method->name ] = [ + [ + $instance->hook_name, + ], + $instance->priority, + ]; } + + continue; } // Handle WordPress hooks being registered with a listener. diff --git a/src/mantle/support/attributes/class-action.php b/src/mantle/support/attributes/class-action.php index 57ab06e5f..ead14ada1 100644 --- a/src/mantle/support/attributes/class-action.php +++ b/src/mantle/support/attributes/class-action.php @@ -12,15 +12,15 @@ /** * Hook Action Attribute * - * Used to hook a method to an WordPress action at a specific priority. + * Used to hook a method to an WordPress hook at a specific priority. */ #[Attribute] class Action { /** * Constructor. * - * @param string $action Action name. + * @param string $hook_name Hook name. * @param int $priority Priority, defaults to 10. */ - public function __construct( public string $action, public int $priority = 10 ) {} + public function __construct( public string $hook_name, public int $priority = 10 ) {} } diff --git a/src/mantle/support/class-service-provider.php b/src/mantle/support/class-service-provider.php index 33b94a1d8..c6676f0e1 100644 --- a/src/mantle/support/class-service-provider.php +++ b/src/mantle/support/class-service-provider.php @@ -80,24 +80,36 @@ public function boot_provider() { } $this->boot_action_hooks(); - $this->boot_attribute_hooks(); $this->boot(); } /** - * Boot all actions on the service provider. + * Boot all actions and attribute methods on the service provider. * - * Allow methods in the 'on_{hook}_at_priority' and 'on_{hook}' format - * to automatically register WordPress hooks. + * Collects all of the `on_{hook}` and `on_{hook}_at_{priority}` methods as + * well as the attribute based `#[Action]` methods and registers them with + * the respective WordPress hooks. */ - protected function boot_action_hooks() { - collect( get_class_methods( static::class ) ) + protected function boot_action_hooks(): void { + $this->collect_action_methods() + ->merge( $this->collect_attribute_hooks() ) + ->unique() + ->each( + fn ( array $item ) => add_action( $item['hook'], [ $this, $item['method'] ], $item['priority'] ), + ); + } + + /** + * Collect all action methods from the service provider. + * + * @return Collection + */ + protected function collect_action_methods(): Collection { + return collect( get_class_methods( static::class ) ) ->filter( - function( string $method ) { - return Str::starts_with( $method, 'on_' ); - } + fn ( string $method ) => Str::starts_with( $method, 'on_' ) ) - ->each( + ->map( function( string $method ) { $hook = Str::after( $method, 'on_' ); $priority = 10; @@ -108,30 +120,42 @@ function( string $method ) { $hook = Str::before_last( $hook, '_at_' ); } - add_action( $hook, [ $this, $method ], $priority ); + return [ + 'hook' => $hook, + 'method' => $method, + 'priority' => $priority, + ]; } ); } /** - * Boot all attribute actions on the service provider. + * Collect all attribute actions on the service provider. + * + * Allow methods with the `#[Action]` attribute to automatically register + * WordPress hooks. + * + * @return Collection */ - protected function boot_attribute_hooks() { + protected function collect_attribute_hooks(): Collection { + $items = new Collection(); $class = new ReflectionClass( static::class ); foreach ( $class->getMethods() as $method ) { - $action_attributes = $method->getAttributes( Action::class ); - - if ( empty( $action_attributes ) ) { - continue; - } - - foreach ( $action_attributes as $attribute ) { + foreach ( $method->getAttributes( Action::class ) as $attribute ) { $instance = $attribute->newInstance(); - add_action( $instance->action, [ $this, $method->name ], $instance->priority ); + $items->push( + [ + 'hook' => $instance->hook_name, + 'method' => $method->getName(), + 'priority' => $instance->priority, + ] + ); } } + + return $items; } /** diff --git a/tests/events/test-wordpress-action-dispatcher.php b/tests/events/test-wordpress-action-dispatcher.php index 9abc88dc0..297e0ceaf 100644 --- a/tests/events/test-wordpress-action-dispatcher.php +++ b/tests/events/test-wordpress-action-dispatcher.php @@ -170,4 +170,24 @@ function() { do_action( 'test_action_to_fire' ); $this->assertTrue( $_SERVER['__test'] ); } + + public function test_typehint_action_argument() { + $_SERVER['__test'] = false; + + add_action( + Example_Typehint_Event::class, + function ( Example_Typehint_Event $event ): void { + $_SERVER['__test'] = $event; + } + ); + + $this->app['events']->dispatch( new Example_Typehint_Event( 'test' ) ); + + $this->assertInstanceOf( Example_Typehint_Event::class, $_SERVER['__test'] ); + $this->assertEquals( 'test', $_SERVER['__test']->example ); + } +} + +class Example_Typehint_Event { + public function __construct( public string $example ) {} } diff --git a/tests/support/test-service-provider.php b/tests/support/test-service-provider.php index 3ea82d499..8b42d9c85 100644 --- a/tests/support/test-service-provider.php +++ b/tests/support/test-service-provider.php @@ -4,6 +4,7 @@ use Mantle\Application\Application; use Mantle\Console\Command; use Mantle\Contracts\Providers as ProviderContracts; +use Mantle\Events\Dispatcher; use Mantle\Support\Service_Provider; use Mantle\Support\Attributes\Action; use Mockery as m; @@ -14,6 +15,7 @@ protected function setUp(): void { remove_all_actions( 'init' ); remove_all_filters( 'custom_filter' ); + remove_all_filters( 'custom_filter_dedupe' ); Service_Provider::$publishes = []; Service_Provider::$publish_tags = []; @@ -66,12 +68,6 @@ public function test_hook_method_filter() { } public function test_hook_attribute() { - // Abandon if we're not running PHP 8. - if ( version_compare( phpversion(), '8.0.0', '<' ) ) { - $this->markTestSkipped( 'Requires PHP 8.0.0 or greater.' ); - return; - } - $app = m::mock( Application::class )->makePartial(); $app->register( Provider_Test_Hook::class ); $app->boot(); @@ -81,6 +77,30 @@ public function test_hook_attribute() { $this->assertTrue( $_SERVER['__custom_hook_fired'] ?? false ); } + public function test_hook_attribute_deduplicate() { + $app = m::mock( Application::class )->makePartial(); + $app->register( Provider_Test_Hook::class ); + $app->boot(); + + $value = apply_filters( 'custom_filter_dedupe', 0 ); + + $this->assertEquals( 10, $value ); + } + + public function test_typehint_event() { + $_SERVER['__custom_event_fired'] = false; + + $app = m::mock( Application::class )->makePartial(); + $app->register( Provider_Test_Hook::class ); + $app->boot(); + + $app['events'] = new Dispatcher( $app ); + + $app['events']->dispatch( new Example_Service_Provider_Event() ); + + $this->assertInstanceOf( Example_Service_Provider_Event::class, $_SERVER['__custom_event_fired'] ); + } + public function test_publishable_service_providers() { $app = m::mock( Application::class )->makePartial(); $app->register( ServiceProviderForTestingOne::class ); @@ -248,8 +268,15 @@ public function handle_custom_hook() { $_SERVER['__custom_hook_fired'] = true; } - public function handle_custom_filter( $value ) { - return $value + 100; + // Assert that only a single action is registered for this hook. + #[Action('custom_filter_dedupe')] + public function on_custom_filter_dedupe( $value ) { + return $value + 10; + } + + #[Action(Example_Service_Provider_Event::class)] + public function handle_custom_event( Example_Service_Provider_Event $event ) { + $_SERVER['__custom_event_fired'] = $event; } } @@ -270,3 +297,7 @@ public function boot() { $this->publishes( [ 'source/tagged/two/b' => 'destination/tagged/two/b' ], 'some_tag' ); } } + +class Example_Service_Provider_Event { + +}