Fix GOOWOO-4: allow scripts and styles to share an asset handle

src/Assets/AssetsHandler.php

@@ -13,20 +13,31 @@
 final class AssetsHandler implements AssetsHandlerInterface {
 
 	/**
-	 * Assets known to this asset handler.
+	 * Assets known to this asset handler, keyed by handle.
 	 *
-	 * @var Asset[]
+	 * A single handle may hold multiple assets of different classes (e.g. a
+	 * script and a style paired under the same handle).
+	 *
+	 * @var array<string, Asset[]>
 	 */
 	private $assets = [];
 
 	/**
 	 * Register a single asset.
 	 *
+	 * Multiple assets of different classes may share a handle (e.g. a script
+	 * and a style). Re-registering the same handle + class combination is a
+	 * silent no-op, matching WordPress's own `wp_register_script` /
+	 * `wp_register_style` behavior.
+	 *
 	 * @param Asset $asset Asset to register.
 	 */
 	public function register( Asset $asset ): void {
-		$this->validate_handle_not_exists( $asset->get_handle() );
-		$this->assets[ $asset->get_handle() ] = $asset;
+		if ( $this->is_asset_registered( $asset ) ) {
+			return;
+		}
+
+		$this->assets[ $asset->get_handle() ][] = $asset;
 		$asset->register();
 	}
 
@@ -72,15 +83,19 @@ public function enqueue_many( array $assets ): void {
 	}
 
 	/**
-	 * Enqueue a single asset based on its handle.
+	 * Enqueue every asset registered under a given handle.
+	 *
+	 * When a handle has both a script and a style, both are enqueued.
 	 *
-	 * @param string $handle Handle of the asset to enqueue.
+	 * @param string $handle Handle of the assets to enqueue.
 	 *
-	 * @throws InvalidAsset If the passed-in asset handle is not valid.
+	 * @throws InvalidAsset If the handle is unknown to this handler.
 	 */
 	public function enqueue_handle( string $handle ): void {
 		$this->validate_handle_exists( $handle );
-		$this->assets[ $handle ]->enqueue();
+		foreach ( $this->assets[ $handle ] as $asset ) {
+			$asset->enqueue();
+		}
 	}
 
 	/**
@@ -97,23 +112,29 @@ public function enqueue_many_handles( array $handles ): void {
 	}
 
 	/**
-	 * Dequeue a single asset based on its handle.
+	 * Dequeue every asset registered under a given handle.
+	 *
+	 * When a handle has both a script and a style, both are dequeued.
 	 *
-	 * @param string $handle Handle of the asset to enqueue.
+	 * @param string $handle Handle of the assets to dequeue.
 	 *
-	 * @throws InvalidAsset If the passed-in asset handle is not valid.
+	 * @throws InvalidAsset If the handle is unknown to this handler.
 	 */
 	public function dequeue_handle( string $handle ): void {
 		$this->validate_handle_exists( $handle );
-		$this->assets[ $handle ]->dequeue();
+		foreach ( $this->assets[ $handle ] as $asset ) {
+			$asset->dequeue();
+		}
 	}
 
 	/**
 	 * Enqueue all assets known to this asset handler.
 	 */
 	public function enqueue_all(): void {
-		foreach ( $this->assets as $asset_object ) {
-			$asset_object->enqueue();
+		foreach ( $this->assets as $handle_assets ) {
+			foreach ( $handle_assets as $asset ) {
+				$asset->enqueue();
+			}
 		}
 	}
 
@@ -125,21 +146,28 @@ public function enqueue_all(): void {
 	 * @throws InvalidAsset When the asset handle is unknown to the object.
 	 */
 	protected function validate_handle_exists( string $handle ): void {
-		if ( ! array_key_exists( $handle, $this->assets ) ) {
+		if ( empty( $this->assets[ $handle ] ) ) {
 			throw InvalidAsset::invalid_handle( $handle );
 		}
 	}
 
 	/**
-	 * Validate that a given asset handle does not already exist.
+	 * Whether an asset of the same class is already registered under its handle.
 	 *
-	 * @param string $handle
+	 * @param Asset $asset
 	 *
-	 * @throws InvalidAsset When the handle exists.
+	 * @return bool
 	 */
-	protected function validate_handle_not_exists( string $handle ): void {
-		if ( array_key_exists( $handle, $this->assets ) ) {
-			throw InvalidAsset::handle_exists( $handle );
+	private function is_asset_registered( Asset $asset ): bool {
+		$handle = $asset->get_handle();
+		if ( empty( $this->assets[ $handle ] ) ) {
+			return false;
+		}
+		foreach ( $this->assets[ $handle ] as $existing ) {
+			if ( get_class( $existing ) === get_class( $asset ) ) {
+				return true;
+			}
 		}
+		return false;
 	}
 }

tests/Unit/Assets/AssetsHandlerTest.php

@@ -0,0 +1,150 @@
+<?php
+declare( strict_types=1 );
+
+namespace Automattic\WooCommerce\GoogleListingsAndAds\Tests\Unit\Assets;
+
+use Automattic\WooCommerce\GoogleListingsAndAds\Assets\Asset;
+use Automattic\WooCommerce\GoogleListingsAndAds\Assets\AssetsHandler;
+use Automattic\WooCommerce\GoogleListingsAndAds\Assets\ScriptAsset;
+use Automattic\WooCommerce\GoogleListingsAndAds\Assets\StyleAsset;
+use Automattic\WooCommerce\GoogleListingsAndAds\Exception\InvalidAsset;
+use PHPUnit\Framework\TestCase;
+
+class AssetsHandlerTest extends TestCase {
+
+	/**
+	 * Build an Asset mock with no WP side effects.
+	 *
+	 * @param string      $handle      Handle to expose via get_handle().
+	 * @param string|null $asset_class Concrete class to mock (defaults to ScriptAsset).
+	 * @return Asset
+	 */
+	private function asset( string $handle, ?string $asset_class = null ): Asset {
+		$asset_class = $asset_class ?? ScriptAsset::class;
+
+		$mock = $this->getMockBuilder( $asset_class )
+			->disableOriginalConstructor()
+			->onlyMethods( [ 'get_handle', 'register', 'enqueue', 'dequeue' ] )
+			->getMock();
+		$mock->method( 'get_handle' )->willReturn( $handle );
+
+		return $mock;
+	}
+
+	public function test_registers_a_single_asset() {
+		$handler = new AssetsHandler();
+		$asset   = $this->asset( 'gla-foo' );
+		$asset->expects( $this->once() )->method( 'register' );
+		$asset->expects( $this->once() )->method( 'enqueue' );
+
+		$handler->register( $asset );
+		$handler->enqueue_handle( 'gla-foo' );
+	}
+
+	public function test_allows_script_and_style_with_same_handle() {
+		$handler = new AssetsHandler();
+		$script  = $this->asset( 'gla-foo', ScriptAsset::class );
+		$style   = $this->asset( 'gla-foo', StyleAsset::class );
+
+		$script->expects( $this->once() )->method( 'register' );
+		$style->expects( $this->once() )->method( 'register' );
+
+		$handler->register( $script );
+		$handler->register( $style );
+
+		// Reaching this point without an InvalidAsset exception is the assertion.
+		$this->addToAssertionCount( 1 );
+	}
+
+	public function test_enqueue_handle_enqueues_all_assets_under_handle() {
+		$handler = new AssetsHandler();
+		$script  = $this->asset( 'gla-foo', ScriptAsset::class );
+		$style   = $this->asset( 'gla-foo', StyleAsset::class );
+		$handler->register( $script );
+		$handler->register( $style );
+
+		$script->expects( $this->once() )->method( 'enqueue' );
+		$style->expects( $this->once() )->method( 'enqueue' );
+
+		$handler->enqueue_handle( 'gla-foo' );
+	}
+
+	public function test_dequeue_handle_dequeues_all_assets_under_handle() {
+		$handler = new AssetsHandler();
+		$script  = $this->asset( 'gla-foo', ScriptAsset::class );
+		$style   = $this->asset( 'gla-foo', StyleAsset::class );
+		$handler->register( $script );
+		$handler->register( $style );
+
+		$script->expects( $this->once() )->method( 'dequeue' );
+		$style->expects( $this->once() )->method( 'dequeue' );
+
+		$handler->dequeue_handle( 'gla-foo' );
+	}
+
+	public function test_enqueue_all_enqueues_every_asset() {
+		$handler = new AssetsHandler();
+		$a       = $this->asset( 'gla-foo', ScriptAsset::class );
+		$b       = $this->asset( 'gla-foo', StyleAsset::class );
+		$c       = $this->asset( 'gla-bar', ScriptAsset::class );
+
+		$handler->register( $a );
+		$handler->register( $b );
+		$handler->register( $c );
+
+		$a->expects( $this->once() )->method( 'enqueue' );
+		$b->expects( $this->once() )->method( 'enqueue' );
+		$c->expects( $this->once() )->method( 'enqueue' );
+
+		$handler->enqueue_all();
+	}
+
+	public function test_register_is_idempotent_for_same_handle_and_class() {
+		$handler = new AssetsHandler();
+		$first   = $this->asset( 'gla-foo', ScriptAsset::class );
+		$second  = $this->asset( 'gla-foo', ScriptAsset::class );
+
+		$first->expects( $this->once() )->method( 'register' );
+		$second->expects( $this->never() )->method( 'register' );
+
+		$handler->register( $first );
+		// Re-registering the same handle + same class should silently no-op, matching
+		// WordPress's own wp_register_script behavior.
+		$handler->register( $second );
+
+		$first->expects( $this->once() )->method( 'enqueue' );
+		$second->expects( $this->never() )->method( 'enqueue' );
+
+		$handler->enqueue_handle( 'gla-foo' );
+	}
+
+	public function test_enqueue_handle_with_unknown_handle_throws() {
+		$handler = new AssetsHandler();
+
+		$this->expectException( InvalidAsset::class );
+		$handler->enqueue_handle( 'gla-unknown' );
+	}
+
+	public function test_dequeue_handle_with_unknown_handle_throws() {
+		$handler = new AssetsHandler();
+
+		$this->expectException( InvalidAsset::class );
+		$handler->dequeue_handle( 'gla-unknown' );
+	}
+
+	public function test_register_many_handles_mixed_classes() {
+		$handler = new AssetsHandler();
+		$assets  = [
+			$this->asset( 'gla-foo', ScriptAsset::class ),
+			$this->asset( 'gla-foo', StyleAsset::class ),
+			$this->asset( 'gla-bar', ScriptAsset::class ),
+		];
+
+		foreach ( $assets as $asset ) {
+			$asset->expects( $this->once() )->method( 'register' );
+		}
+
+		$handler->register_many( $assets );
+		$this->addToAssertionCount( 1 );
+	}
+}