Update script handles to WooCommerce standards #2966
Conversation
Refactored script registration and enqueuing to use dynamic handler names for 'jquery-blockui' and 'jquery-tiptip' based on WooCommerce version. Introduced Assets::get_wc_handler() to map legacy handlers to new ones, ensuring compatibility with WooCommerce 10.3.0 and above.
kzamanbd
added
Needs: Testing
WalkthroughThis PR refactors legacy WC script handle usage by introducing a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
includes/Admin/SetupWizard.php(2 hunks)includes/Admin/SetupWizardNoWC.php(2 hunks)includes/Assets.php(4 hunks)includes/Vendor/SetupWizard.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
includes/Vendor/SetupWizard.php (1)
includes/Assets.php (2)
Assets(12-1406)get_wc_handler(32-42)
includes/Admin/SetupWizard.php (1)
includes/Assets.php (2)
Assets(12-1406)get_wc_handler(32-42)
includes/Admin/SetupWizardNoWC.php (1)
includes/Assets.php (2)
Assets(12-1406)get_wc_handler(32-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: api tests (1, 1)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: e2e tests (3, 3)
🔇 Additional comments (4)
includes/Assets.php (1)
391-391: LGTM with dependency on get_wc_handler fix.The usage of the new
get_wc_handler()method is implemented correctly:
- Handler is computed once and stored in a variable (line 391)
- Consistently used in dependency arrays (line 509)
- Properly used in conditional registration logic (lines 600-605)
Once the critical bug in
get_wc_handler()is fixed, this implementation will work correctly for all WooCommerce versions.Also applies to: 509-509, 600-605
includes/Vendor/SetupWizard.php (1)
7-7: LGTM! Clean implementation of dynamic WC asset handling.The vendor setup wizard correctly:
- Imports the Assets class (line 7)
- Retrieves dynamic handles for both WooCommerce scripts (lines 104-105)
- Enqueues scripts using the dynamic handles (lines 109-110)
This ensures compatibility with both legacy and modern WooCommerce versions.
Also applies to: 104-110
includes/Admin/SetupWizardNoWC.php (1)
6-6: LGTM! Consistent dynamic asset handling.The no-WC setup wizard properly:
- Imports the Assets class (line 6)
- Retrieves the dynamic jquery-blockui handle (line 46)
- Registers the script using the dynamic handle (line 47)
- Uses it in the wc-setup script dependencies (line 51)
The implementation is clean and follows the same pattern as other setup wizards.
Also applies to: 46-47, 51-51
includes/Admin/SetupWizard.php (1)
6-6: LGTM! Thorough implementation with proper conditional checks.The admin setup wizard correctly:
- Imports the Assets class (line 6)
- Retrieves dynamic handles for both WooCommerce scripts (lines 155-156)
- Conditionally registers jquery-tiptip only if not already registered (lines 158-160)
- Uses both dynamic handles in the wc-setup script dependencies (line 162)
The conditional registration check is a nice defensive pattern that prevents duplicate registrations.
Also applies to: 155-162
| public static function get_wc_handler( $handler ): string { | ||
| // map legacy handlers to new ones | ||
| $handlers = [ | ||
| 'jquery-blockui' => 'jquery-blockui', | ||
| 'jquery-tiptip' => 'jquery-tiptip', | ||
| ]; | ||
| if ( version_compare( WC()->version, '10.3.0', '>=' ) ) { | ||
| return 'wc-' . $handlers[ $handler ]; | ||
| } | ||
| return $handlers[ $handler ] ?? $handler; | ||
| } |
There was a problem hiding this comment.
Critical: Fix undefined array key access and refactor handler mapping.
The current implementation has a critical bug when an unknown handler is passed with WooCommerce >= 10.3.0. On line 39, $handlers[$handler] will trigger an undefined array key warning if the handler isn't in the mapping, and concatenating with 'wc-' will produce an invalid handle like 'wc-' (empty suffix).
Additionally, the mapping array is redundant since keys and values are identical—it's not actually "mapping" anything, just whitelisting known handlers.
Apply this refactored implementation:
public static function get_wc_handler( $handler ): string {
- // map legacy handlers to new ones
- $handlers = [
+ // Whitelist of WC handlers that require version-specific prefixing
+ $supported_handlers = [
'jquery-blockui' => 'jquery-blockui',
'jquery-tiptip' => 'jquery-tiptip',
];
+
+ // Return original handler if not in our supported list
+ if ( ! isset( $supported_handlers[ $handler ] ) ) {
+ return $handler;
+ }
+
+ // For WC 10.3.0+, use 'wc-' prefix
if ( version_compare( WC()->version, '10.3.0', '>=' ) ) {
- return 'wc-' . $handlers[ $handler ];
+ return 'wc-' . $supported_handlers[ $handler ];
}
- return $handlers[ $handler ] ?? $handler;
+
+ return $supported_handlers[ $handler ];
}Alternatively, simplify further by using an array of strings:
public static function get_wc_handler( $handler ): string {
- // map legacy handlers to new ones
- $handlers = [
- 'jquery-blockui' => 'jquery-blockui',
- 'jquery-tiptip' => 'jquery-tiptip',
- ];
+ // Whitelist of WC handlers that require version-specific prefixing
+ $supported_handlers = [ 'jquery-blockui', 'jquery-tiptip' ];
+
+ // Return original handler if not in our supported list
+ if ( ! in_array( $handler, $supported_handlers, true ) ) {
+ return $handler;
+ }
+
+ // For WC 10.3.0+, use 'wc-' prefix
if ( version_compare( WC()->version, '10.3.0', '>=' ) ) {
- return 'wc-' . $handlers[ $handler ];
+ return 'wc-' . $handler;
}
- return $handlers[ $handler ] ?? $handler;
+
+ return $handler;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static function get_wc_handler( $handler ): string { | |
| // map legacy handlers to new ones | |
| $handlers = [ | |
| 'jquery-blockui' => 'jquery-blockui', | |
| 'jquery-tiptip' => 'jquery-tiptip', | |
| ]; | |
| if ( version_compare( WC()->version, '10.3.0', '>=' ) ) { | |
| return 'wc-' . $handlers[ $handler ]; | |
| } | |
| return $handlers[ $handler ] ?? $handler; | |
| } | |
| public static function get_wc_handler( $handler ): string { | |
| // Whitelist of WC handlers that require version-specific prefixing | |
| $supported_handlers = [ | |
| 'jquery-blockui' => 'jquery-blockui', | |
| 'jquery-tiptip' => 'jquery-tiptip', | |
| ]; | |
| // Return original handler if not in our supported list | |
| if ( ! isset( $supported_handlers[ $handler ] ) ) { | |
| return $handler; | |
| } | |
| // For WC 10.3.0+, use 'wc-' prefix | |
| if ( version_compare( WC()->version, '10.3.0', '>=' ) ) { | |
| return 'wc-' . $supported_handlers[ $handler ]; | |
| } | |
| return $supported_handlers[ $handler ]; | |
| } |
🤖 Prompt for AI Agents
In includes/Assets.php around lines 32-42, replace the current associative
$handlers usage that accesses $handlers[$handler] (causing undefined key
warnings) with a whitelist array of handler names (simple strings) and guard the
WC version branch with an existence check; specifically, create an array like
$allowed = ['jquery-blockui','jquery-tiptip']; then if
version_compare(WC()->version,'10.3.0','>=' ) return 'wc-'.$handler only when
in_array($handler,$allowed,true) else return $handler; for older versions just
return $handler (or return $handler when not in the whitelist) so no undefined
array key access occurs and the mapping is simplified.
| if ( version_compare( WC()->version, '10.3.0', '>=' ) ) { | ||
| return 'wc-' . $handlers[ $handler ]; | ||
| } |
There was a problem hiding this comment.
We did not check the $handlers[ $handler ]; isset or not.
Pls check the coderabbit suggestion.
mrabbani
added
Needs: Author Reply
and removed
Needs: Dev Review
Refactored script registration and enqueuing to use dynamic handler names for 'jquery-blockui' and 'jquery-tiptip' based on WooCommerce version. Introduced Assets::get_wc_handler() to map legacy handlers to new ones, ensuring compatibility with WooCommerce 10.3.0 and above.
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
Title
Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.
Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit