Add redirect delay settings#2117
Conversation
WalkthroughThe pull request introduces enhancements to form submission and redirection logic across multiple files. Key updates include refined redirection handling in the Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 3
🧹 Outside diff range and nitpick comments (5)
classes/models/FrmOnSubmitAction.php (1)
56-59: Consider adding validation and documentation for the new redirect delay settings.The implementation looks good, but could be enhanced with the following improvements:
- Add validation for
redirect_delay_timeto ensure reasonable minimum/maximum values- Add PHPDoc comments to document the purpose and expected values of these settings
Consider applying this diff:
+ /** + * Whether to enable redirect delay + * @var string + */ 'redirect_delay' => '', - // Value in second. + /** + * Delay time in seconds before redirect + * @var int + */ 'redirect_delay_time' => 8, + + /** + * Message to show during redirect delay + * @var string + */ 'redirect_delay_msg' => FrmOnSubmitHelper::get_default_redirect_msg(),classes/helpers/FrmOnSubmitHelper.php (2)
131-134: Consider adding an upper limit for the redirect delay timeWhile there's a minimum validation of 1 second, there's no upper limit which could potentially lead to excessively long delays. Consider adding a reasonable upper limit (e.g., 60 seconds) to prevent abuse or poor user experience.
$delay_time = intval( $args['form_action']->post_content['redirect_delay_time'] ); -if ( $delay_time < 1 ) { +if ( $delay_time < 1 || $delay_time > 60 ) { $delay_time = 8; }
111-113: Consider using data attributes for all toggle parametersThe toggle functionality uses a single data attribute for the class. For better maintainability and consistency, consider adding additional data attributes for the initial state and target element.
'input_html' => array( - 'data-toggleclass' => 'frm_redirect_delay_settings', + 'data-toggle-class' => 'frm_redirect_delay_settings', + 'data-toggle-target' => '#frm_redirect_delay_settings', + 'data-toggle-initial-state' => $redirect_delay ? 'shown' : 'hidden', ),js/formidable.js (2)
736-750: Consider improving modularity and error handling of thedoRedirectfunction.The function implementation is logically sound but could be enhanced:
- Move the function outside
getFormErrorsscope to improve reusability.- Add validation for the response object properties.
- Add JSDoc documentation for better maintainability.
Consider refactoring to:
- const doRedirect = response => { + /** + * Handles form redirect after submission. + * + * @param {Object} response - The server response object + * @param {boolean} response.openInNewTab - Whether to open in new tab + * @param {string} response.redirect - The redirect URL + * @param {string} [response.fallbackMsg] - Message to show if new tab is blocked + * @param {number} [response.delay] - Delay in seconds before redirect + */ + function doRedirect( response ) { + if (!response || !response.redirect) { + console.error('Invalid redirect response'); + return; + } + jQuery( document ).trigger( 'frmBeforeFormRedirect', [ object, response ]); if ( ! response.openInNewTab ) { window.location = response.redirect; return; } const newTab = window.open( response.redirect, '_blank' ); if ( ! newTab && response.fallbackMsg && response.content ) { response.content = response.content.trim().replace( /(<\/div><\/div>)$/, ' ' + response.fallbackMsg + '</div></div>' ); } }
779-784: Add validation for redirect delay value.While the delay handling is implemented correctly, consider adding validation to ensure the delay value is reasonable and within acceptable limits.
Consider adding validation:
if ( response.delay ) { + // Ensure delay is a positive number and not too large + const delay = Math.min(Math.max(0, parseFloat(response.delay) || 0), 30); setTimeout( function() { doRedirect( response ); - }, 1000 * response.delay ); + }, 1000 * delay ); } else { doRedirect( response ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
classes/controllers/FrmFormsController.php(5 hunks)classes/helpers/FrmOnSubmitHelper.php(2 hunks)classes/models/FrmOnSubmitAction.php(1 hunks)js/formidable.js(2 hunks)
🔇 Additional comments (4)
classes/models/FrmOnSubmitAction.php (1)
59-59: Verify the existence of the default message helper method.
Let's ensure that the FrmOnSubmitHelper::get_default_redirect_msg() method exists and returns the expected default message.
✅ Verification successful
The helper method exists and returns a proper default message
The FrmOnSubmitHelper::get_default_redirect_msg() method is properly implemented in classes/helpers/FrmOnSubmitHelper.php. It returns a translatable string "Please wait while you are redirected." which is appropriate for a redirect delay message.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the helper method definition
ast-grep --pattern 'function get_default_redirect_msg() {
$$$
}'
# Alternatively, use ripgrep to find any references
rg "get_default_redirect_msg"
Length of output: 301
Script:
#!/bin/bash
# Let's check the implementation of the helper method
ast-grep --pattern 'function get_default_redirect_msg() {
$$$
}' classes/helpers/FrmOnSubmitHelper.php
Length of output: 106
Script:
#!/bin/bash
# Let's try with ripgrep to see the implementation with context
rg -A 5 "get_default_redirect_msg" classes/helpers/FrmOnSubmitHelper.php
Length of output: 250
classes/helpers/FrmOnSubmitHelper.php (1)
98-169: Verify the JavaScript implementation and migration strategy
The new redirect delay functionality requires JavaScript implementation and might affect existing forms. Let's verify:
- The JavaScript handling of these new fields
- The migration strategy for existing forms
classes/controllers/FrmFormsController.php (2)
2383-2386: Logic enhancement for redirect delay handling is correct
The added condition appropriately checks if there is no redirect_delay_time before proceeding with the redirection. This ensures that the redirection logic accounts for potential delays specified in the form options.
Line range hint 3063-3077: Assess the implications of changing delete_all method visibility from private to public
The method delete_all() in the FrmFormsController class has been changed from private to public. Making this method public could potentially expose it to external calls, which may lead to security risks such as unauthorized bulk deletion of forms.
Ensure that proper permission checks and nonce verification are implemented to prevent unauthorized access. Verify that changing the method's visibility does not introduce security vulnerabilities.
|
Hi @truongwp, I am testing this feature, and this is the issue I encountered:
|
|
@lauramekaj1 That's intentional. If the Delay time isn't correct, it will be set to |
|
I added some code to not allow negative value for that input. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
classes/controllers/FrmFormsController.php (2)
2777-2792: Improve code organization for new tab handlingThe code handling open in new tab functionality is mixed with content generation. Consider extracting this into a separate method for better maintainability.
Consider refactoring to:
- if ( empty( $response_data['content'] ) ) { - $args['message'] = FrmOnSubmitHelper::get_default_new_tab_msg(); - $args['form']->options['success_msg'] = $args['message']; - $args['form']->options['edit_msg'] = $args['message']; - if ( ! isset( $args['fields'] ) ) { - $args['fields'] = FrmField::get_all_for_form( $args['form']->id ); - } - $args['message'] = self::prepare_submit_message( $args['form'], $args['entry_id'], $args ); - ob_start(); - self::show_lone_success_message( $args ); - $response_data['content'] .= ob_get_clean(); - } + $response_data['content'] .= self::maybe_add_new_tab_content( $response_data, $args ); // Add new private method: private static function maybe_add_new_tab_content( $response_data, $args ) { if ( ! empty( $response_data['content'] ) ) { return ''; } $args['message'] = FrmOnSubmitHelper::get_default_new_tab_msg(); $args['form']->options['success_msg'] = $args['message']; $args['form']->options['edit_msg'] = $args['message']; if ( ! isset( $args['fields'] ) ) { $args['fields'] = FrmField::get_all_for_form( $args['form']->id ); } $args['message'] = self::prepare_submit_message( $args['form'], $args['entry_id'], $args ); ob_start(); self::show_lone_success_message( $args ); return ob_get_clean(); }
Line range hint
2808-2819: Consider adding validation for redirect delay timeThe code uses the redirect delay time directly without validation. Consider adding bounds checking to prevent unreasonable delay times.
Add validation:
+ // Add constant at class level + const MAX_REDIRECT_DELAY = 300; // 5 minutes in seconds + - $delay_time = apply_filters( 'frm_redirect_delay_time', 1000 * $args['form']->options['redirect_delay_time'] ); + $delay_seconds = min( (int) $args['form']->options['redirect_delay_time'], self::MAX_REDIRECT_DELAY ); + $delay_time = apply_filters( 'frm_redirect_delay_time', 1000 * $delay_seconds );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
classes/controllers/FrmFormsController.php(5 hunks)classes/helpers/FrmOnSubmitHelper.php(2 hunks)classes/models/FrmOnSubmitAction.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/models/FrmOnSubmitAction.php
🔇 Additional comments (5)
classes/helpers/FrmOnSubmitHelper.php (3)
159-168: Initialize redirect delay message with default value
The wp_editor for redirect delay message doesn't initialize with a default value when empty.
342-346: 🛠️ Refactor suggestion
Add validation when populating redirect delay data
The redirect delay settings are populated without validation. Consider adding validation to ensure consistent behavior.
- $form_options['redirect_delay'] = ! empty( $action->post_content['redirect_delay'] );
- $form_options['redirect_delay_time'] = $action->post_content['redirect_delay_time'];
- $form_options['redirect_delay_msg'] = $action->post_content['redirect_delay_msg'];
+ $form_options['redirect_delay'] = ! empty( $action->post_content['redirect_delay'] );
+ $delay_time = isset( $action->post_content['redirect_delay_time'] )
+ ? intval( $action->post_content['redirect_delay_time'] )
+ : self::DEFAULT_REDIRECT_DELAY;
+ $form_options['redirect_delay_time'] = $delay_time < 1 ? self::DEFAULT_REDIRECT_DELAY : $delay_time;
+ $form_options['redirect_delay_msg'] = ! empty( $action->post_content['redirect_delay_msg'] )
+ ? $action->post_content['redirect_delay_msg']
+ : self::get_default_redirect_msg();Likely invalid or redundant comment.
342-342: Verify URL validation for redirect
Ensure that the redirect URL is properly validated to prevent open redirect vulnerabilities.
classes/controllers/FrmFormsController.php (2)
2383-2384: Verify redirect delay time check
The condition checks if the redirect delay time is empty before proceeding with the redirect. This is a good defensive check, but we should ensure consistent handling of empty/zero values.
✅ Verification successful
Empty redirect delay time is handled consistently across the codebase
The codebase shows consistent handling of the redirect_delay_time value:
- Default value of 8 is set in
FrmOnSubmitAction.php - When the value is less than 1, it defaults to 8 in
FrmOnSubmitHelper.php - The empty check in
FrmFormsController.phpis appropriate as it triggers population of default values viaFrmOnSubmitHelper::populate_on_submit_data()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any other places where redirect_delay_time is used to ensure consistent handling
rg -A 2 "redirect_delay_time"
Length of output: 2298
2769-2772:
Ensure proper initialization of content variable
The code concatenates to $response_data['content'] but there's no guarantee that this variable is initialized. This could lead to PHP notices.
Apply this fix:
if ( ! empty( $args['form']->options['redirect_delay'] ) ) {
$response_data['delay'] = $args['form']->options['redirect_delay_time'];
+ if ( ! isset( $response_data['content'] ) ) {
+ $response_data['content'] = '';
+ }
$response_data['content'] .= self::get_redirect_message( $args['success_url'], $args['form']->options['redirect_delay_msg'], $args );
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
classes/controllers/FrmFormsController.php (1)
Line range hint
2808-2819: Review redirect delay timing implementationThe code implements redirect delay timing but could benefit from some improvements:
- The delay time should have a minimum value to prevent instant redirects
- The delay time should be validated as numeric
-$delay_time = apply_filters( 'frm_redirect_delay_time', 1000 * $args['form']->options['redirect_delay_time'] ); +$min_delay = 1; // 1 second minimum +$delay_time = max($min_delay, floatval($args['form']->options['redirect_delay_time'])); +$delay_time = apply_filters( 'frm_redirect_delay_time', 1000 * $delay_time );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
classes/controllers/FrmFormsController.php(5 hunks)
🔇 Additional comments (3)
classes/controllers/FrmFormsController.php (3)
2769-2772: LGTM: Redirect delay implementation
The code correctly handles redirect delay settings by adding delay time and message to the response data.
2777-2792: 🛠️ Refactor suggestion
Verify new tab handling edge cases
The code handles opening redirects in new tabs, but there are some edge cases to consider:
- Browser popup blockers may prevent the new tab from opening
- The fallback message should be shown when popup is blocked
Add popup blocker detection:
if ( ! empty( $args['form']->options['open_in_new_tab'] ) ) {
$response_data['openInNewTab'] = 1;
+ $response_data['popupBlockerMsg'] = __( 'Please allow popups to open the form in a new tab.', 'formidable' );2383-2383: Verify redirect delay condition
The condition checks if the redirect action has no delay time set. Consider adding a null check for $conf_method[0]->post_content to prevent potential PHP notices.
-if ( 'redirect' === FrmOnSubmitHelper::get_action_type( $conf_method[0] ) && empty( $conf_method[0]->post_content['redirect_delay'] ) ) {
+if ( 'redirect' === FrmOnSubmitHelper::get_action_type( $conf_method[0] ) && isset( $conf_method[0]->post_content ) && empty( $conf_method[0]->post_content['redirect_delay'] ) ) {
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
classes/views/frm-form-actions/on_submit_redirect_settings.php (2)
6-6: Replace version placeholder with actual version numberThe
@since x.xplaceholder should be replaced with the actual version number where this feature is being introduced.
13-21: Consider enhancing URL input field UXWhile the implementation is secure and accessible, consider these UX improvements:
- Add URL validation using
type="url"instead oftype="text"- Include a placeholder with example URL format
<input - type="text" + type="url" id="<?php echo esc_attr( $id_attr ); ?>" name="<?php echo esc_attr( $args['action_control']->get_field_name( 'success_url' ) ); ?>" value="<?php echo esc_attr( $args['form_action']->post_content['success_url'] ); ?>" + placeholder="https://example.com" />classes/helpers/FrmOnSubmitHelper.php (1)
86-86: Consider using path constants for file inclusionWhile moving view logic to a separate file is good practice, consider using WordPress path constants instead of string concatenation for better maintainability.
-include FrmAppHelper::plugin_path() . '/classes/views/frm-form-actions/on_submit_redirect_settings.php'; +include FrmAppHelper::plugin_path() . DIRECTORY_SEPARATOR . 'classes' . DIRECTORY_SEPARATOR . 'views' . + DIRECTORY_SEPARATOR . 'frm-form-actions' . DIRECTORY_SEPARATOR . 'on_submit_redirect_settings.php';classes/controllers/FrmFormsController.php (1)
2777-2792: Consider extracting message handling logicThe code block handles multiple operations including message setting, field retrieval, and content generation. Consider extracting this into a separate method for better maintainability.
Consider refactoring like this:
- if ( empty( $response_data['content'] ) ) { - $args['message'] = FrmOnSubmitHelper::get_default_new_tab_msg(); - $args['form']->options['success_msg'] = $args['message']; - $args['form']->options['edit_msg'] = $args['message']; - if ( ! isset( $args['fields'] ) ) { - $args['fields'] = FrmField::get_all_for_form( $args['form']->id ); - } - $args['message'] = self::prepare_submit_message( $args['form'], $args['entry_id'], $args ); - ob_start(); - self::show_lone_success_message( $args ); - $response_data['content'] .= ob_get_clean(); - } + if ( empty( $response_data['content'] ) ) { + $response_data['content'] .= self::get_new_tab_content( $args ); + } // Add new method: private static function get_new_tab_content( $args ) { $args['message'] = FrmOnSubmitHelper::get_default_new_tab_msg(); $args['form']->options['success_msg'] = $args['message']; $args['form']->options['edit_msg'] = $args['message']; if ( ! isset( $args['fields'] ) ) { $args['fields'] = FrmField::get_all_for_form( $args['form']->id ); } $args['message'] = self::prepare_submit_message( $args['form'], $args['entry_id'], $args ); ob_start(); self::show_lone_success_message( $args ); return ob_get_clean(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
classes/controllers/FrmFormsController.php(5 hunks)classes/helpers/FrmOnSubmitHelper.php(2 hunks)classes/views/frm-form-actions/on_submit_redirect_settings.php(1 hunks)
🔇 Additional comments (9)
classes/views/frm-form-actions/on_submit_redirect_settings.php (3)
23-45: LGTM!
The toggle implementation is well-structured, using proper helper methods, translations, and toggle behavior.
47-96: Implementation aligns well with PR requirements
The code successfully addresses the concerns raised in PR comments:
- Prevents negative values with
min="1" - Implements the intended default of 8 seconds
- Provides rich text editing for the delay message
98-117: Verify tooltip content availability
The implementation looks good, but let's verify the tooltip content is properly defined.
✅ Verification successful
Tooltip content is properly defined and implemented
The tooltip content for 'new_tab' is properly defined in FrmAppHelper class with a clear and helpful message: "This option will open the link in a new browser tab. Please note that some popup blockers may prevent this from happening, in which case the link will be displayed."
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for 'new_tab' tooltip definition
rg -A 2 "new_tab.*tooltip"
Length of output: 28
Script:
#!/bin/bash
# Let's try searching for tooltip content in different ways
# Search for 'new_tab' in tooltip definitions or translations
rg "new_tab" --type php
# Also search specifically in helper or tooltip-related files
fd "tooltip|helper" -e php --exec rg -l "new_tab" {}
Length of output: 3510
classes/helpers/FrmOnSubmitHelper.php (3)
235-239: 🛠️ Refactor suggestion
Extract magic numbers to class constants
The default delay time (8 seconds) and maximum delay time (60 seconds) should be defined as class constants.
Add these constants at the class level:
+/** @var int Default redirect delay time in seconds */
+private const DEFAULT_REDIRECT_DELAY = 8;
+
+/** @var int Maximum allowed redirect delay time in seconds */
+private const MAX_REDIRECT_DELAY = 60;Then use them in the validation code:
-$delay_time = isset( $action->post_content['redirect_delay_time'] ) ?
- absint( $action->post_content['redirect_delay_time'] ) : 8;
-$form_options['redirect_delay_time'] = $delay_time < 1 ? 8 : min($delay_time, 60);
+$delay_time = isset( $action->post_content['redirect_delay_time'] ) ?
+ absint( $action->post_content['redirect_delay_time'] ) : self::DEFAULT_REDIRECT_DELAY;
+$form_options['redirect_delay_time'] = $delay_time < 1 ?
+ self::DEFAULT_REDIRECT_DELAY : min($delay_time, self::MAX_REDIRECT_DELAY);Likely invalid or redundant comment.
235-239:
Add validation for redirect settings
The code lacks validation for the new redirect settings which could lead to security issues and unexpected behavior:
- No validation for redirect_delay_time (can accept negative values as reported)
- No default values for new fields
- No type checking
- Potential security risk with unvalidated redirect URL
Consider applying this validation:
-$form_options[ $opt . 'url' ] = isset( $action->post_content['success_url'] ) ? $action->post_content['success_url'] : '';
-$form_options['open_in_new_tab'] = ! empty( $action->post_content['open_in_new_tab'] );
-$form_options['redirect_delay'] = ! empty( $action->post_content['redirect_delay'] );
-$form_options['redirect_delay_time'] = $action->post_content['redirect_delay_time'];
-$form_options['redirect_delay_msg'] = $action->post_content['redirect_delay_msg'];
+$form_options[ $opt . 'url' ] = isset( $action->post_content['success_url'] ) ?
+ esc_url_raw( $action->post_content['success_url'] ) : '';
+$form_options['open_in_new_tab'] = ! empty( $action->post_content['open_in_new_tab'] );
+$form_options['redirect_delay'] = ! empty( $action->post_content['redirect_delay'] );
+$delay_time = isset( $action->post_content['redirect_delay_time'] ) ?
+ absint( $action->post_content['redirect_delay_time'] ) : 8;
+$form_options['redirect_delay_time'] = $delay_time < 1 ? 8 : min($delay_time, 60);
+$form_options['redirect_delay_msg'] = isset( $action->post_content['redirect_delay_msg'] ) ?
+ wp_kses_post( $action->post_content['redirect_delay_msg'] ) : self::get_default_redirect_msg();Likely invalid or redundant comment.
235-239: Verify redirect URL handling
The redirect URL handling needs verification to ensure it's properly sanitized and validated across the codebase.
✅ Verification successful
Based on the shell script results, I can provide a final response as the code shows proper URL handling:
URL handling is properly secured across the codebase
The redirect URL handling in the codebase follows WordPress security best practices:
- URLs are properly sanitized using
esc_url()andesc_url_raw()before output and database storage - The
frm_redirect_urlfilter is applied with proper validation throughFrmEntriesController::prepare_redirect_url() - URLs are encoded properly before redirects using
wp_redirect()withesc_url_raw() - The success URL is filtered through WordPress content filters and shortcode processing before use
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how redirect URLs are handled across the codebase
# Check for URL sanitization
echo "Checking URL sanitization..."
rg -A 2 "success_url|redirect_url"
# Check for potential XSS vectors in URL handling
echo "Checking URL output..."
ast-grep --pattern 'echo $$$url$$$'
# Check for URL validation
echo "Checking URL validation..."
rg -A 2 "esc_url|wp_http_validate_url"
Length of output: 96431
classes/controllers/FrmFormsController.php (3)
2383-2383: LGTM!
The condition correctly checks for a redirect action with no delay.
Line range hint 2808-2819: LGTM!
The code correctly:
- Prepares the redirect message
- Provides a filter hook for customizing delay time
- Converts seconds to milliseconds for JavaScript timing
2769-2772:
Initialize 'content' key before concatenation
The code appends to $response_data['content'] using .= operator without ensuring the 'content' key exists. This could trigger an undefined index notice.
Apply this diff to initialize the content key:
if ( ! empty( $args['form']->options['redirect_delay'] ) ) {
+ $response_data['content'] = '';
$response_data['delay'] = $args['form']->options['redirect_delay_time'];
$response_data['content'] .= self::get_redirect_message( $args['success_url'], $args['form']->options['redirect_delay_msg'], $args );
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
classes/controllers/FrmFormsController.php (1)
2777-2792: Consider extracting message preparation logicThe message preparation logic for new tab redirects is quite lengthy and could be extracted into a separate method for better maintainability.
- if ( empty( $response_data['content'] ) ) { - $args['message'] = FrmOnSubmitHelper::get_default_new_tab_msg(); - $args['form']->options['success_msg'] = $args['message']; - $args['form']->options['edit_msg'] = $args['message']; - if ( ! isset( $args['fields'] ) ) { - $args['fields'] = FrmField::get_all_for_form( $args['form']->id ); - } - $args['message'] = self::prepare_submit_message( $args['form'], $args['entry_id'], $args ); - ob_start(); - self::show_lone_success_message( $args ); - $response_data['content'] .= ob_get_clean(); - } + if ( empty( $response_data['content'] ) ) { + $response_data['content'] .= self::prepare_new_tab_message( $args ); + } // Add new method: + private static function prepare_new_tab_message( $args ) { + $args['message'] = FrmOnSubmitHelper::get_default_new_tab_msg(); + $args['form']->options['success_msg'] = $args['message']; + $args['form']->options['edit_msg'] = $args['message']; + if ( ! isset( $args['fields'] ) ) { + $args['fields'] = FrmField::get_all_for_form( $args['form']->id ); + } + $args['message'] = self::prepare_submit_message( $args['form'], $args['entry_id'], $args ); + ob_start(); + self::show_lone_success_message( $args ); + return ob_get_clean(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
classes/controllers/FrmFormsController.php(5 hunks)classes/views/frm-form-actions/on_submit_redirect_settings.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/views/frm-form-actions/on_submit_redirect_settings.php
🔇 Additional comments (3)
classes/controllers/FrmFormsController.php (3)
2769-2772: LGTM: Proper handling of redirect delay in AJAX response
The code correctly adds delay time and message to the AJAX response data when redirect delay is enabled.
Line range hint 2808-2819: Verify redirect delay time filter usage
The code adds a filter for customizing redirect delay time, which is good for flexibility. However, ensure the delay time is properly validated.
✅ Verification successful
Let me check if there's any validation of the delay time value in the codebase.
Redirect delay time is properly validated and has reasonable defaults
The codebase shows proper handling of the redirect delay time:
- Default value is set to 8 seconds in
FrmOnSubmitAction.php - Value is validated using
intval()in the form settings view - A minimum value of 1 second is enforced, defaulting to 8 seconds if less than 1
- The filter is applied after the value is already sanitized and validated
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any custom implementations of the filter
rg "frm_redirect_delay_time"
Length of output: 185
Script:
#!/bin/bash
# Check for validation or sanitization of redirect_delay_time
ast-grep --pattern 'redirect_delay_time'
# Also check the form options setup
rg "redirect_delay_time" -B 2 -A 2
Length of output: 3146
2383-2384: Verify redirect delay condition
The condition checks if the redirect action has no delay before proceeding with the redirect. This is a critical check to ensure proper redirect timing.
✅ Verification successful
Based on the gathered information, I can now provide the final response:
The redirect delay check is correctly implemented
The condition if ( 'redirect' === FrmOnSubmitHelper::get_action_type( $conf_method[0] ) && empty( $conf_method[0]->post_content['redirect_delay'] ) ) is properly checking for immediate redirects. The codebase shows:
- The redirect delay is a configurable option with proper default values in
FrmOnSubmitAction FrmOnSubmitHelper::get_action_type()correctly returns the action type from post_content- Different redirect behaviors are handled based on the delay setting:
- Immediate redirect when delay is empty
- Delayed redirect with configurable message and timing when enabled
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that redirect delay is properly handled across the codebase
rg -A 2 "redirect_delay['\"]?\s*=>"
Length of output: 236
Script:
#!/bin/bash
# Let's check how redirect_delay is used in the codebase
rg -A 5 "redirect_delay"
# Also check for any related methods in FrmOnSubmitHelper
ast-grep --pattern 'class FrmOnSubmitHelper {
$$$
static function get_action_type($_) {
$$$
}
$$$
}'
Length of output: 9828
Script:
#!/bin/bash
# Let's check the implementation of get_action_type method
ast-grep --pattern 'static function get_action_type($_) {
$$$
}'
# Also check for any related redirect handling in FrmOnSubmitHelper
ast-grep --pattern 'class FrmOnSubmitHelper {
$$$
static function current_event($_) {
$$$
}
$$$
}'
Length of output: 181
Script:
#!/bin/bash
# Let's try a different approach to find these methods
rg -A 5 "function get_action_type|function current_event" --type php
# Also check how redirect is handled in the codebase
rg "=== 'redirect'" --type php -A 2
Length of output: 1275
lauramekaj1
left a comment
lauramekaj1
left a comment
There was a problem hiding this comment.
@truongwp I tested it and it's working as expected. Thank you!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
css/frm_admin.css (1)
8231-8234: Use caution with "!important" overrides.While removing the border on child inputs is valid, "!important" overrides may cause conflicts with higher-level or third-party CSS. If you can avoid using "!important" by adjusting specificity, it may improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/views/frm-form-actions/on_submit_redirect_settings.php(1 hunks)css/frm_admin.css(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/views/frm-form-actions/on_submit_redirect_settings.php
🔇 Additional comments (2)
css/frm_admin.css (2)
8224-8229: Looks good.
These new styles establish a container for inputs that carry suffix elements, maintaining consistent padding and border. The approach seems correct, and no issues stand out here.
9360-9360: Match margins with surrounding layout.
Increasing the vertical margin to 22px helps with spacing, but ensure it aligns with adjacent elements so that the form maintains a visually consistent spacing system. Verify it doesn’t introduce unexpected layout shifts.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
classes/controllers/FrmFormsController.php (2)
2783-2801: Consider adding error event handling for new tab functionality.While the code handles the basic new tab functionality well, consider adding error event handling to detect if the new tab was blocked by a popup blocker.
if ( ! empty( $args['form']->options['open_in_new_tab'] ) ) { $response_data['openInNewTab'] = 1; + $response_data['onError'] = array( + 'event' => 'frm_new_tab_blocked', + 'message' => __( 'The new tab was blocked. Please allow popups for this site.', 'formidable' ) + ); // Only show open in new tab text if there is no delay. if ( empty( $response_data['content'] ) ) {
Line range hint
2814-2825: Add inline documentation for the delay time format.The code handles delay time well but could benefit from clearer documentation about the expected format of the delay time value.
/** * Filters the delay time before redirecting when On Submit Redirect action is delayed. * * @since 6.0 + * @param int $delay_time Delay time in milliseconds (converted from seconds set in form options). + * @return int Modified delay time in milliseconds. */ $delay_time = apply_filters( 'frm_redirect_delay_time', 1000 * $args['form']->options['redirect_delay_time'] );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/controllers/FrmFormsController.php(6 hunks)
🔇 Additional comments (2)
classes/controllers/FrmFormsController.php (2)
2389-2389: LGTM: Redirect delay check is properly implemented.
The condition correctly checks for empty redirect delay before proceeding with the redirect action.
2775-2779: LGTM: Redirect delay settings are properly handled in AJAX response.
The code correctly adds delay time and message to the AJAX response data structure.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
classes/helpers/FrmOnSubmitHelper.php (2)
86-86: Add file existence check before includeWhile moving the view logic to a separate file improves maintainability, we should verify the file exists to prevent PHP warnings in production.
-include FrmAppHelper::plugin_path() . '/classes/views/frm-form-actions/on_submit_redirect_settings.php'; +$view_file = FrmAppHelper::plugin_path() . '/classes/views/frm-form-actions/on_submit_redirect_settings.php'; +if ( file_exists( $view_file ) ) { + include $view_file; +} else { + wp_die( esc_html__( 'Error: Required view file is missing.', 'formidable' ) ); +}
Line range hint
1-1: Define a constant for the default redirect delay timeTo improve maintainability and consistency, the default delay time of 8 seconds should be defined as a class constant.
class FrmOnSubmitHelper { + /** + * Default redirect delay time in seconds + * + * @since 6.x + * @var int + */ + private const DEFAULT_REDIRECT_DELAY = 8;Then update the validation code to use this constant:
- $delay_time = isset( $action->post_content['redirect_delay_time'] ) ? absint( $action->post_content['redirect_delay_time'] ) : 8; - $form_options['redirect_delay_time'] = $delay_time < 1 ? 8 : min( $delay_time, 60 ); + $delay_time = isset( $action->post_content['redirect_delay_time'] ) ? absint( $action->post_content['redirect_delay_time'] ) : self::DEFAULT_REDIRECT_DELAY; + $form_options['redirect_delay_time'] = $delay_time < 1 ? self::DEFAULT_REDIRECT_DELAY : min( $delay_time, 60 );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/helpers/FrmOnSubmitHelper.php(3 hunks)
🔇 Additional comments (1)
classes/helpers/FrmOnSubmitHelper.php (1)
235-239:
Add validation and defaults for redirect delay settings
Based on the PR comments about negative values being accepted, we should add validation for the delay time and set appropriate defaults for all new fields.
- $form_options[ $opt . 'url' ] = isset( $action->post_content['success_url'] ) ? $action->post_content['success_url'] : '';
- $form_options['open_in_new_tab'] = ! empty( $action->post_content['open_in_new_tab'] );
- $form_options['redirect_delay'] = ! empty( $action->post_content['redirect_delay'] );
- $form_options['redirect_delay_time'] = $action->post_content['redirect_delay_time'];
- $form_options['redirect_delay_msg'] = $action->post_content['redirect_delay_msg'];
+ $form_options[ $opt . 'url' ] = isset( $action->post_content['success_url'] ) ? $action->post_content['success_url'] : '';
+ $form_options['open_in_new_tab'] = ! empty( $action->post_content['open_in_new_tab'] );
+ $form_options['redirect_delay'] = ! empty( $action->post_content['redirect_delay'] );
+ $delay_time = isset( $action->post_content['redirect_delay_time'] ) ? absint( $action->post_content['redirect_delay_time'] ) : 8;
+ $form_options['redirect_delay_time'] = $delay_time < 1 ? 8 : min( $delay_time, 60 ); // Cap at 60 seconds
+ $form_options['redirect_delay_msg'] = ! empty( $action->post_content['redirect_delay_msg'] )
+ ? $action->post_content['redirect_delay_msg']
+ : self::get_default_redirect_msg();Likely invalid or redundant comment.
3 participants
Handoff: https://linear.app/strategy11/document/feature-handoff-479d8bd5186c