Skip to content

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

Open
gabriel-fuentes wants to merge 1 commit into
developfrom
fix/GOOWOO-4-assets-handler-allow-script-style-same-handle
Open

Fix GOOWOO-4: allow scripts and styles to share an asset handle#3477
gabriel-fuentes wants to merge 1 commit into
developfrom
fix/GOOWOO-4-assets-handler-allow-script-style-same-handle

Conversation

@gabriel-fuentes

Copy link
Copy Markdown

Changes proposed in this Pull Request:

Closes #129.

AssetsHandler stored assets in a flat handle -> Asset map and threw InvalidAsset on any duplicate handle. WordPress core keeps separate registries for scripts and styles so same-handle pairs are normal there. The original report describes that case, and Zendesk #11272914 confirms a real fatal in production when GfW is active alongside Enhanced Media Library (~70K installs): two code paths register the same handle within a single request, the second one throws, and the customer hits a white screen on wp-admin/options-general.php?page=media.

Change AssetsHandler::$assets to array<string, Asset[]> so a handle can hold multiple assets of different classes. Same handle + same class is now a silent no-op (idempotent), matching WordPress's own wp_register_script / wp_register_style behavior. AssetsHandlerInterface is unchanged, so existing callers see no signature change.

Screenshots:

n/a (no UI change).

Detailed test instructions:

PHPUnit (covers the AC + idempotency):

  1. composer install
  2. vendor/bin/phpunit --bootstrap vendor/autoload.php --no-configuration tests/Unit/Assets/AssetsHandlerTest.php
  3. Expect 9 passing tests, 19 assertions.

Manual repro of Zendesk #11272914:

  1. WP 6.7+, WC 10.7+, GfW (this branch), Enhanced Media Library 2.9.4 all active.
  2. Visit wp-admin/options-general.php?page=media as an admin.
  3. The page loads without a fatal. Without this fix, the same setup throws InvalidAsset: The asset handle "google-listings-and-ads" already exists.

The AC scenario (registering a ScriptAsset and a StyleAsset under the same handle) is covered by test_allows_script_and_style_with_same_handle and friends.

Additional details:

The fix also lets future GfW code register a StyleAsset under the same handle as its matching script, which is what the issue's original AC asks for. The current google-listings-and-ads script + google-listings-and-ads-css style split was a workaround for this limitation.

Changelog entry

Fix - Allow scripts and styles to share an asset handle, preventing fatal errors when multiple components register an asset under the same handle.


Created during the Woo Bug Blitz with Claude Code via team Ceres' /bug-blitz skill.

Drafted with AI; reviewed by me.

AssetsHandler stored assets in a flat handle -> Asset map and threw
InvalidAsset on any duplicate handle. WordPress core keeps separate
registries for scripts and styles so same-handle pairs are normal
there, and the original report calls out registering a ScriptAsset and
StyleAsset under the same handle. The strict throw also caused a fatal
in production when multiple code paths registered the same handle
within a single request (Zendesk #11272914 with Enhanced Media Library
active).

Change AssetsHandler::$assets to array<string, Asset[]> so a handle can
hold multiple assets of different classes. Same handle + same class is
now a silent no-op, matching wp_register_script / wp_register_style.
The public interface (AssetsHandlerInterface) is unchanged. Adds
PHPUnit coverage for the new behavior and the idempotency case.

Fixes: https://linear.app/a8c/issue/GOOWOO-4
@github-actions github-actions Bot added type: bug The issue is a confirmed bug. changelog: fix Took care of something that wasn't working. labels Jun 3, 2026
@codecov

codecov Bot commented Jun 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.5%. Comparing base (5f2c6bf) to head (a50ba86).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             develop   #3477     +/-   ##
===========================================
+ Coverage       66.7%   67.5%   +0.9%     
- Complexity      5514    5520      +6     
===========================================
  Files            923     531    -392     
  Lines          28735   22058   -6677     
  Branches        1694       0   -1694     
===========================================
- Hits           19164   14899   -4265     
+ Misses          9319    7159   -2160     
+ Partials         252       0    -252     
Flag Coverage Δ
js-unit-tests ?
php-unit-tests 67.5% <100.0%> (+0.1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/Assets/AssetsHandler.php 79.5% <100.0%> (+79.5%) ⬆️

... and 393 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gabriel-fuentes gabriel-fuentes marked this pull request as ready for review June 3, 2026 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog: fix Took care of something that wasn't working. type: bug The issue is a confirmed bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AssetHandler doesn't allow script and style assets to have the same name

1 participant