Merge pull request #8871 from google/dont-add-action-inside-action-8814

Ensure hooks for conversion tracking classes can be registered.

Author: kuasha420

includes/Core/Conversion_Tracking/Conversion_Event_Providers/WooCommerce.php

@@ -66,24 +66,20 @@ public function register_script() {
 
 		$script->register( $this->context );
 
-		$this->add_purchase_event_hook();
-
 		return $script;
 	}
 
 	/**
 	 * Adds a hook for a purchase event.
+	 *
+	 * @since 1.129.0
 	 */
-	public function add_purchase_event_hook() {
+	public function register_hooks() {
+		$input = $this->context->input();
+
 		add_action(
 			'woocommerce_thankyou',
-			function ( $order_id ) {
-				// Don't output this script tag if conversion tracking is
-				// disabled.
-				if ( ! $this->is_active() ) {
-					return;
-				}
-
+			function ( $order_id ) use ( $input ) {
 				$order = wc_get_order( $order_id );
 
 				// If there isn't a valid order for this ID, or if this order
@@ -95,7 +91,6 @@ function ( $order_id ) {
 
 				// Ensure the order key in the query param is valid for this
 				// order.
-				$input     = $this->context->input();
 				$order_key = $input->filter( INPUT_GET, 'key' );
 
 				// Don't output the script tag if the order key is invalid.
@@ -115,5 +110,4 @@ function ( $order_id ) {
 			1
 		);
 	}
-
 }

includes/Core/Conversion_Tracking/Conversion_Events_Provider.php

@@ -62,6 +62,16 @@ public function is_active() {
 	 */
 	abstract public function get_event_names();
 
+	/**
+	 * Registers any actions/hooks for this provider.
+	 *
+	 * @since 1.129.0
+	 */
+	public function register_hooks() {
+		// No-op by default, but left here so subclasses can implement
+		// their own `add_action`/hook calls.
+	}
+
 	/**
 	 * Registers the script for the provider.
 	 *

includes/Core/Conversion_Tracking/Conversion_Tracking.php

@@ -93,6 +93,15 @@ public function register() {
 		$this->rest_conversion_tracking_controller->register();
 
 		add_action( 'wp_enqueue_scripts', fn () => $this->maybe_enqueue_scripts(), 30 );
+
+		$active_providers = $this->get_active_providers();
+
+		array_walk(
+			$active_providers,
+			function( Conversion_Events_Provider $active_provider ) {
+				$active_provider->register_hooks();
+			}
+		);
 	}
 
 	/**

tests/phpunit/includes/Core/Conversion_Tracking/Conversion_Event_Providers/FakeConversionEventProvider.php

@@ -50,4 +50,14 @@ public function register_script() {
 		return $script_asset;
 	}
 
+	/**
+	 * Registers any actions/hooks for this provider.
+	 *
+	 * @since 1.129.0
+	 */
+	public function register_hooks() {
+		// Register a fake action.
+		add_action( 'fake_provider_action', '__return_true' );
+	}
+
 }

tests/phpunit/integration/Core/Conversion_Tracking/Conversion_Event_Providers/WooCommerceTest.php

@@ -49,4 +49,10 @@ public function test_register_script() {
 		$this->assertTrue( wp_script_is( $handle, 'registered' ) );
 	}
 
+	public function test_register_hooks() {
+		$this->assertFalse( has_action( 'woocommerce_thankyou' ) );
+		$this->woocommerce->register_hooks();
+		$this->assertTrue( has_action( 'woocommerce_thankyou' ) );
+	}
+
 }

tests/phpunit/integration/Core/Conversion_Tracking/Conversion_TrackingTest.php

@@ -82,13 +82,17 @@ public function test_register__not_enqueued_when_tracking_disabled() {
 	 * @dataProvider data_modules
 	 */
 	public function test_register__enqueued_when_snippet_inserted( $module_slug ) {
+		$this->assertFalse( has_action( 'fake_provider_action' ) );
+
 		$this->conversion_tracking->register();
 
 		do_action( "googlesitekit_{$module_slug}_init_tag" );
 		do_action( 'wp_enqueue_scripts' );
 
 		$this->assertTrue( wp_script_is( 'gsk-cep-' . FakeConversionEventProvider_Active::CONVERSION_EVENT_PROVIDER_SLUG ) );
 		$this->assertFalse( wp_script_is( 'gsk-cep-' . FakeConversionEventProvider::CONVERSION_EVENT_PROVIDER_SLUG ) );
+
+		$this->assertTrue( has_action( 'fake_provider_action' ) );
 	}
 
 	public function data_modules() {