Provider registration actions should fire once or not at all when provider is inactive

changelog/fix-container-registration-actions

@@ -0,0 +1,4 @@
+Significance: minor
+Type: fix
+
+Providers will fire their registration action only once and only if they are active. [TCMN-178]

src/Common/Contracts/Container.php

@@ -4,6 +4,7 @@
 
 use TEC\Common\StellarWP\ContainerContract\ContainerInterface;
 use TEC\Common\Exceptions\Not_Bound_Exception;
+use TEC\Common\Contracts\Provider\Controller;
 
 use TEC\Common\lucatume\DI52\Container as DI52_Container;
 
@@ -32,28 +33,42 @@ public function get( $id ) {
 	 * Overrides the parent method to fire an action when a service provider is registered.
 	 *
 	 * @since 5.1.4
+	 * @since TBD - Ensure registration actions are fired only once and ONLY for active controllers.
 	 *
-	 * @param string $serviceProviderClass The service provider class name.
-	 * @param string ...$alias             Optional. The alias(es) to register the service provider with.
+	 * @param string $service_provider_class The service provider class name.
+	 * @param string ...$alias               Optional. The alias(es) to register the service provider with.
 	 *
 	 * @return void
 	 *
 	 * @throws \TEC\Common\lucatume\DI52\ContainerException If the provider class is marked as deferred but
 	 *                                                      does not provide a set of deferred registrations.
 	 */
-	public function register( $serviceProviderClass, ...$alias ) {
+	public function register( $service_provider_class, ...$alias ) {
+		// Function is_a can be used with strings but instanceof only with objects!
+		$is_controller = is_a( $service_provider_class, Controller::class, true ); // phpcs:ignore StellarWP.PHP.IsAFunction.Found
+
+		if ( $is_controller && $this->getVar( $service_provider_class . '_registered', false ) ) {
+			// If the controller is already registered, bail.
+			return;
+		}
+
 		// Register the provider with the parent container.
-		parent::register( $serviceProviderClass, ...$alias );
+		parent::register( $service_provider_class, ...$alias );
+
+		if ( $is_controller && ! $this->getVar( $service_provider_class . '_registered', false ) ) {
+			// If the controller did not register, DO NOT fire registration actions AT ALL. Instead silently return.
+			return;
+		}
 
 		/**
 		 * Fires when a service provider is registered by the container.
 		 *
 		 * @since 5.1.4
 		 *
-		 * @param string        $serviceProviderClass The service provider class name.
-		 * @param array<string> $alias                The alias(es) the service provider was registered with.
+		 * @param string        $service_provider_class The service provider class name.
+		 * @param array<string> $alias                  The alias(es) the service provider was registered with.
 		 */
-		do_action( 'tec_container_registered_provider', $serviceProviderClass, $alias );
+		do_action( 'tec_container_registered_provider', $service_provider_class, $alias );
 
 		/**
 		 * Fires a class-specific action when a service provider is registered by the container.
@@ -62,13 +77,13 @@ public function register( $serviceProviderClass, ...$alias ) {
 		 *
 		 * @param array<string> $alias The alias(es) the service provider was registered with.
 		 */
-		do_action( 'tec_container_registered_provider_' . $serviceProviderClass, $alias );
+		do_action( 'tec_container_registered_provider_' . $service_provider_class, $alias );
 
 		if (
 			// Back compat with older definition of Service Provider.
-			! property_exists( $serviceProviderClass, 'registration_action' )
+			! property_exists( $service_provider_class, 'registration_action' )
 			// New definition of Service Provider: default action is empty.
-			|| empty( $serviceProviderClass::$registration_action )
+			|| empty( $service_provider_class::$registration_action )
 		) {
 			return;
 		}
@@ -78,7 +93,7 @@ public function register( $serviceProviderClass, ...$alias ) {
 		 *
 		 * @since 5.1.4
 		 */
-		do_action( $serviceProviderClass::$registration_action, $serviceProviderClass, $alias );
+		do_action( $service_provider_class::$registration_action, $service_provider_class, $alias );
 	}
 
 	/**

tests/wpunit/Common/Contracts/Container_Test.php

@@ -0,0 +1,130 @@
+<?php
+
+namespace TEC\Common\Contracts;
+
+use Codeception\TestCase\WPTestCase;
+use TEC\Common\Contracts\Provider\AlreadyRegisteredException;
+use TEC\Common\Contracts\Provider\ControllerInactiveException;
+use TEC\Common\Contracts\Provider\Controller as Controller_Contract;
+use Tribe\Tests\Traits\With_Uopz;
+
+class Container_Test extends WPTestCase {
+	use With_Uopz;
+
+	/**
+	 * The cloned container.
+	 *
+	 * @return ?Container
+	 */
+	protected $cloned_container = null;
+
+	/**
+	 * @before
+	 */
+	public function prepare() {
+		$this->cloned_container = clone tribe();
+		$this->assertNotSame( $this->cloned_container, tribe() );
+
+		$localized = $this->cloned_container;
+		$this->set_fn_return( 'tribe', fn( $service = null ) => $service ? $localized->get( $service ) : $localized, true );
+
+		$this->assertSame( $this->cloned_container, tribe() );
+	}
+
+	/**
+	 * @test
+	 */
+	public function it_should_fire_registration_actions_only_once() {
+		$provider = new class( $this->cloned_container ) extends Controller_Contract {
+			public static string $registration_action = 'foo_test_action';
+
+			protected function do_register(): void {}
+
+			public function unregister(): void {}
+
+			public function is_active(): bool {
+				return true;
+			}
+		};
+
+		$provider_class_name = get_class( $provider );
+
+		$this->assertEquals( 0, did_action( 'tec_container_registered_provider_' . $provider_class_name ) );
+		$this->assertEquals( 0, did_action( $provider::$registration_action ) );
+
+		$this->cloned_container->register( $provider_class_name );
+
+		$this->assertEquals( 1, did_action( 'tec_container_registered_provider_' . $provider_class_name ) );
+		$this->assertEquals( 1, did_action( $provider::$registration_action ) );
+
+		$this->cloned_container->register( $provider_class_name );
+
+		$this->assertEquals( 1, did_action( 'tec_container_registered_provider_' . $provider_class_name ) );
+		$this->assertEquals( 1, did_action( $provider::$registration_action ) );
+	}
+
+	/**
+	 * @test
+	 */
+	public function it_should_fire_registration_actions_only_once_even_after_unregistration() {
+		$provider = new class( $this->cloned_container ) extends Controller_Contract {
+			public static string $registration_action = 'foo_test_action';
+
+			protected function do_register(): void {}
+
+			public function unregister(): void {}
+
+			public function is_active(): bool {
+				return true;
+			}
+		};
+
+		$provider_class_name = get_class( $provider );
+
+		$this->assertEquals( 0, did_action( 'tec_container_registered_provider_' . $provider_class_name ) );
+		$this->assertEquals( 0, did_action( $provider::$registration_action ) );
+
+		$this->cloned_container->register( $provider_class_name );
+
+		$this->assertEquals( 1, did_action( 'tec_container_registered_provider_' . $provider_class_name ) );
+		$this->assertEquals( 1, did_action( $provider::$registration_action ) );
+
+		$provider->unregister();
+		$this->cloned_container->register( $provider_class_name );
+
+		$this->assertEquals( 1, did_action( 'tec_container_registered_provider_' . $provider_class_name ) );
+		$this->assertEquals( 1, did_action( $provider::$registration_action ) );
+	}
+
+	/**
+	 * @test
+	 */
+	public function it_should_not_fire_registration_actions_for_inactive() {
+		$provider = new class( $this->cloned_container ) extends Controller_Contract {
+			public static string $registration_action = 'foo_test_action';
+
+			protected function do_register(): void {}
+
+			public function unregister(): void {}
+
+			public function is_active(): bool {
+				return false;
+			}
+		};
+
+		$provider_class_name = get_class( $provider );
+
+		$this->assertEquals( 0, did_action( 'tec_container_registered_provider_' . $provider_class_name ) );
+		$this->assertEquals( 0, did_action( $provider::$registration_action ) );
+
+		$this->cloned_container->register( $provider_class_name );
+
+		$this->assertEquals( 0, did_action( 'tec_container_registered_provider_' . $provider_class_name ) );
+		$this->assertEquals( 0, did_action( $provider::$registration_action ) );
+
+		$this->cloned_container->register( $provider_class_name );
+
+		$this->assertEquals( 0, did_action( 'tec_container_registered_provider_' . $provider_class_name ) );
+		$this->assertEquals( 0, did_action( $provider::$registration_action ) );
+	}
+}