Avoid loading Stripe script if Stripe gateway is not selected for a payment action. #2081
Conversation
|
Warning Rate limit exceeded@AbdiTolesa has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 46 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce a new static variable and several methods in the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
stripe/controllers/FrmStrpLiteActionsController.php(2 hunks)
🔇 Additional comments (3)
stripe/controllers/FrmStrpLiteActionsController.php (3)
13-13: LGTM: Well-structured static variable declaration.
The private static variable is appropriately used to track the Stripe script loading state, preventing duplicate loads.
442-459: LGTM: Well-implemented script loading optimization.
The changes improve performance by:
- Conditionally loading the Stripe script
- Properly preparing and localizing script variables
- Following WordPress script enqueuing best practices
468-486: LGTM: Well-structured helper methods.
The new helper methods are:
- Well-organized with clear responsibilities
- Include proper validation and error handling
- Follow good coding practices
Also applies to: 496-511, 519-522
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
stripe/controllers/FrmStrpLiteActionsController.php (1)
501-508: Enhance form ID validation.Consider adding explicit type checking and validation for the form ID parameter.
private static function should_load_stripe_script_for_form( $form_id ) { + $form_id = absint( $form_id ); + if ( ! $form_id ) { + return false; + } $action_status = array( 'post_status' => 'publish', );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
stripe/controllers/FrmStrpLiteActionsController.php(2 hunks)
🔇 Additional comments (4)
stripe/controllers/FrmStrpLiteActionsController.php (4)
13-18: LGTM! Well-documented static variable.
The new static variable is properly scoped and documented with PHPDoc standards.
447-464: LGTM! Secure script loading implementation.
The implementation includes proper security measures with nonce verification and URL escaping. The script parameters are comprehensive and well-structured.
466-491: LGTM! Well-structured conditional loading logic.
The method effectively manages the script loading state and follows single responsibility principle.
510-514:
Add type safety checks for gateway access.
The code assumes $action->post_content['gateway'] is always an array, which could lead to runtime errors.
AbdiTolesa
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
stripe/controllers/FrmStrpLiteActionsController.php (1)
466-491: Consider using early returns for better readability.While the logic is correct, the method's readability could be improved by restructuring with early returns.
Consider this refactoring:
private static function should_load_stripe_script() { if ( self::$has_loaded_stripe_script ) { return false; } global $frm_vars; - $form_ids = $frm_vars['forms_loaded']; - if ( ! is_array( $form_ids ) ) { + if ( empty( $frm_vars['forms_loaded'] ) || ! is_array( $frm_vars['forms_loaded'] ) ) { return false; } - $form_ids = array_unique( wp_list_pluck( $form_ids, 'id' ) ); + $form_ids = array_unique( wp_list_pluck( $frm_vars['forms_loaded'], 'id' ) ); foreach ( $form_ids as $form_id ) { if ( self::should_load_stripe_script_for_form( $form_id ) ) { self::$has_loaded_stripe_script = true; return true; } } return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
stripe/controllers/FrmStrpLiteActionsController.php(2 hunks)
🔇 Additional comments (4)
stripe/controllers/FrmStrpLiteActionsController.php (4)
13-18: LGTM! Well-documented static variable.
The static variable is properly declared and documented with PHPDoc comments.
Line range hint 518-564: LGTM! Well-structured script loading logic.
The method properly handles:
- Conditional script loading
- Dependencies management
- File existence checks
- Minified/unminified file variants
447-464: LGTM! Good separation of concerns.
The changes improve code organization by:
- Separating script loading logic into a dedicated method
- Maintaining proper order of operations (script loading before localization)
493-516:
Add type checking for gateway array access.
The array access at line 511 assumes $action->post_content['gateway'] is always an array, which could lead to runtime errors.
Apply the type checking as suggested in the past review:
foreach ( $payment_actions as $action ) {
- if ( in_array( 'stripe', $action->post_content['gateway'], true ) ) {
+ if ( isset( $action->post_content['gateway'] ) &&
+ ( ( is_array( $action->post_content['gateway'] ) && in_array( 'stripe', $action->post_content['gateway'], true ) ) ||
+ ( is_string( $action->post_content['gateway'] ) && 'stripe' === $action->post_content['gateway'] ) ) ) {
return true;
}
}
There was a problem hiding this comment.
@AbdiTolesa I tested this and it's working fine for me. Thank you!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
stripe/controllers/FrmStrpLiteActionsController.php (2)
447-464: Consider optimizing script loading order.The script variables are being localized even if the Stripe script is not loaded. Consider wrapping the variable preparation and localization within the same conditional check.
- self::maybe_load_stripe_script(); - - $action_settings = self::prepare_settings_for_js( $form_id ); - $style_settings = self::get_style_settings_for_form( $form_id ); - $stripe_vars = array( - 'publishable_key' => $publishable, - 'form_id' => $form_id, - 'nonce' => wp_create_nonce( 'frm_strp_ajax' ), - 'ajax' => esc_url_raw( FrmAppHelper::get_ajax_url() ), - 'settings' => $action_settings, - 'locale' => self::get_locale(), - 'baseFontSize' => $style_settings['field_font_size'], - 'appearanceRules' => self::get_appearance_rules( $style_settings ), - 'account_id' => FrmStrpLiteConnectHelper::get_account_id(), - ); - - wp_localize_script( 'formidable-stripe', 'frm_stripe_vars', $stripe_vars ); + if (self::maybe_load_stripe_script()) { + $action_settings = self::prepare_settings_for_js( $form_id ); + $style_settings = self::get_style_settings_for_form( $form_id ); + $stripe_vars = array( + 'publishable_key' => $publishable, + 'form_id' => $form_id, + 'nonce' => wp_create_nonce( 'frm_strp_ajax' ), + 'ajax' => esc_url_raw( FrmAppHelper::get_ajax_url() ), + 'settings' => $action_settings, + 'locale' => self::get_locale(), + 'baseFontSize' => $style_settings['field_font_size'], + 'appearanceRules' => self::get_appearance_rules( $style_settings ), + 'account_id' => FrmStrpLiteConnectHelper::get_account_id(), + ); + + wp_localize_script( 'formidable-stripe', 'frm_stripe_vars', $stripe_vars ); + }
524-527: Consider returning boolean from maybe_load_stripe_script.The method name suggests it might return a boolean indicating whether the script was loaded, which would be useful for the suggested optimization in the script loading order.
- private static function maybe_load_stripe_script() { + private static function maybe_load_stripe_script(): bool { if ( ! self::should_load_stripe_script() ) { - return; + return false; } + return true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
stripe/controllers/FrmStrpLiteActionsController.php(2 hunks)
🔇 Additional comments (2)
stripe/controllers/FrmStrpLiteActionsController.php (2)
13-18: LGTM! Well-documented static variable.
The new static variable is properly declared with clear PHPDoc documentation.
510-514:
Add type check before accessing array index.
As noted in the previous review, the code assumes $action->post_content['gateway'] is always an array. Add a type check to prevent potential errors.
- if ( in_array( 'stripe', $action->post_content['gateway'], true ) ) {
+ if ( isset( $action->post_content['gateway'] ) &&
+ ( ( is_array( $action->post_content['gateway'] ) && in_array( 'stripe', $action->post_content['gateway'], true ) ) ||
+ ( is_string( $action->post_content['gateway'] ) && 'stripe' === $action->post_content['gateway'] ) ) ) {| private static function should_load_stripe_script() { | ||
| if ( self::$has_loaded_stripe_script ) { | ||
| return false; | ||
| } | ||
| global $frm_vars; | ||
| $form_ids = $frm_vars['forms_loaded']; | ||
| if ( empty( $form_ids ) || ! is_array( $form_ids ) ) { | ||
| return false; | ||
| } | ||
| $form_ids = array_unique( wp_list_pluck( $form_ids, 'id' ) ); | ||
| foreach ( $form_ids as $form_id ) { | ||
| if ( self::should_load_stripe_script_for_form( $form_id ) ) { | ||
| self::$has_loaded_stripe_script = true; | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add early return and empty check for forms_loaded.
The function could be simplified with early returns, and the empty check for $frm_vars['forms_loaded'] should be moved before accessing it.
private static function should_load_stripe_script() {
if ( self::$has_loaded_stripe_script ) {
return false;
}
global $frm_vars;
- $form_ids = $frm_vars['forms_loaded'];
- if ( empty( $form_ids ) || ! is_array( $form_ids ) ) {
+ if ( empty( $frm_vars['forms_loaded'] ) || ! is_array( $frm_vars['forms_loaded'] ) ) {
return false;
}
+ $form_ids = $frm_vars['forms_loaded'];
$form_ids = array_unique( wp_list_pluck( $form_ids, 'id' ) );📝 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.
| private static function should_load_stripe_script() { | |
| if ( self::$has_loaded_stripe_script ) { | |
| return false; | |
| } | |
| global $frm_vars; | |
| $form_ids = $frm_vars['forms_loaded']; | |
| if ( empty( $form_ids ) || ! is_array( $form_ids ) ) { | |
| return false; | |
| } | |
| $form_ids = array_unique( wp_list_pluck( $form_ids, 'id' ) ); | |
| foreach ( $form_ids as $form_id ) { | |
| if ( self::should_load_stripe_script_for_form( $form_id ) ) { | |
| self::$has_loaded_stripe_script = true; | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| private static function should_load_stripe_script() { | |
| if ( self::$has_loaded_stripe_script ) { | |
| return false; | |
| } | |
| global $frm_vars; | |
| if ( empty( $frm_vars['forms_loaded'] ) || ! is_array( $frm_vars['forms_loaded'] ) ) { | |
| return false; | |
| } | |
| $form_ids = $frm_vars['forms_loaded']; | |
| $form_ids = array_unique( wp_list_pluck( $form_ids, 'id' ) ); | |
| foreach ( $form_ids as $form_id ) { | |
| if ( self::should_load_stripe_script_for_form( $form_id ) ) { | |
| self::$has_loaded_stripe_script = true; | |
| return true; | |
| } | |
| } | |
| return false; | |
| } |
Fix https://github.com/Strategy11/formidable-pro/issues/5004
Test steps
frmstrp.jsscript.