Remove buttons settings tab#2115
Conversation
WalkthroughThe pull request introduces several modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 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: 0
🧹 Outside diff range and nitpick comments (3)
classes/views/frm-forms/settings-buttons.php (2)
6-7: Enhance the deprecation check comment.The current comment could be more descriptive about the purpose and conditions of the deprecation check.
Consider this improved version:
-// Do not deprecate if the Pro version still use these hooks. +// Only deprecate hooks if Formidable Pro >= 6.16.2 is active, as older versions still use these hooks.
Line range hint
24-29: Enhance the settings migration notice.Consider providing more specific guidance on how to access the moved settings.
-<?php esc_html_e( 'Submit button settings were moved to the Submit button in the form builder.', 'formidable' ); ?> +<?php esc_html_e( 'Submit button settings were moved to the Submit button in the form builder. Click the submit button in the form preview to access these settings.', 'formidable' ); ?>classes/controllers/FrmFormsController.php (1)
Line range hint
952-991: Improve error handling increate_page_with_shortcodemethodIn the
create_page_with_shortcode()method,die( 0 );is used for error handling when permissions are insufficient or required parameters are missing. Usingdie()is not recommended in WordPress AJAX handlers. Consider usingwp_send_json_error()orwp_die()with appropriate HTTP status codes and messages to provide better feedback and adhere to WordPress best practices.Apply this diff to improve error handling:
@@ -954,7 +954,7 @@ public static function create_page_with_shortcode() { if ( ! current_user_can( 'publish_posts' ) ) { - die( 0 ); + wp_send_json_error( __( 'You do not have permission to publish posts.', 'formidable' ), 403 ); } check_ajax_referer( 'frm_ajax', 'nonce' ); @@ -960,7 +960,7 @@ $type = FrmAppHelper::get_post_param( 'type', '', 'sanitize_text_field' ); if ( ! $type || ! in_array( $type, array( 'form', 'view' ), true ) ) { - die( 0 ); + wp_send_json_error( __( 'Invalid type parameter.', 'formidable' ), 400 ); } $object_id = FrmAppHelper::get_post_param( 'object_id', '', 'absint' ); @@ -964,7 +964,7 @@ if ( ! $object_id ) { - die( 0 ); + wp_send_json_error( __( 'Missing object ID.', 'formidable' ), 400 ); } $postarr = array( 'post_type' => 'page' );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
classes/controllers/FrmFormsController.php(1 hunks)classes/views/frm-forms/settings-buttons.php(2 hunks)
🔇 Additional comments (5)
classes/views/frm-forms/settings-buttons.php (2)
13-17: LGTM! Proper hook deprecation handling.
The conditional logic correctly implements WordPress hook deprecation practices, maintaining backward compatibility while warning about future removal.
32-36: Verify usage of deprecated hooks.
Let's verify if these hooks are still being used in the codebase to ensure we're not breaking any existing functionality.
✅ Verification successful
Hooks are still in active use - deprecation approach is correct
The verification shows that both hooks are actively used in the core functionality:
-
frm_add_form_style_tab_optionsis used in:- Added via
FrmHooksController - Removed via
FrmAppController - Used for conditional section display in
FrmFormsController
- Added via
-
frm_add_form_button_optionsis used in:- Checked for conditional section display in
FrmFormsController - Both hooks control visibility of the 'buttons' section
- Checked for conditional section display in
The current implementation using do_action_deprecated() is the correct approach as it maintains backward compatibility while signaling deprecation to developers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of these hooks in the codebase
echo "Checking usage of frm_add_form_style_tab_options hook:"
rg "add_action.*frm_add_form_style_tab_options" -B 2 -A 2
echo -e "\nChecking usage of frm_add_form_button_options hook:"
rg "add_action.*frm_add_form_button_options" -B 2 -A 2
Length of output: 929
Script:
#!/bin/bash
# Let's search for any usage of these hooks, including apply_filters and do_action calls
echo "Checking all usage of frm_add_form_style_tab_options hook:"
rg "frm_add_form_style_tab_options" -B 2 -A 2
echo -e "\nChecking all usage of frm_add_form_button_options hook:"
rg "frm_add_form_button_options" -B 2 -A 2
# Let's also check for any potential usage in plugin integrations or add-ons
echo -e "\nChecking for potential plugin integration files:"
fd -e php | grep -i "integration\|addon\|add-on"
Length of output: 3872
classes/controllers/FrmFormsController.php (3)
Line range hint 1050-1053: Evaluate the implications of changing delete_all method visibility to public
The delete_all() method has been changed from private to public. Exposing this method publicly allows it to be called from outside the class, which might introduce security risks or unintended usage. Verify that appropriate permission checks are in place to prevent unauthorized access to this method.
Run the following script to verify if delete_all() is being called externally and ensure permission checks are in place:
#!/bin/bash
# Description: Check for external calls to `delete_all()` method.
# Search for calls to `delete_all()` outside the `FrmFormsController` class.
rg 'FrmFormsController::delete_all\('Line range hint 1041-1050: Assess the impact of updating form_key in rename_form method
In the rename_form() method, the form_key is updated alongside the form name when a form is renamed. Changing the form_key may have unintended side effects if the form is referenced elsewhere using the old form_key. Ensure that updating the form_key will not break any existing references, shortcodes, or integrations that rely on the original form_key.
Run the following script to find hard-coded references to the old form_key in the codebase:
#!/bin/bash
# Description: Search for hard-coded references to the old form_key.
# Replace 'old_form_key' with the actual old form_key value.
rg 'old_form_key'1412-1414: Ensure consistent handling of the 'buttons' settings tab
The conditional removal of the 'buttons' section from the settings tabs is appropriate if the associated hooks are not in use. However, verify that there are no unconditional references to $sections['buttons'] elsewhere in the codebase, which could lead to undefined index errors.
Run the following script to check for unconditional usages of $sections['buttons']:
✅ Verification successful
The previous search didn't yield the expected results as it used the wrong pattern. Let me try a more comprehensive search:
Let me correct the syntax and try again:
Let me try a simpler pattern without backreferences:
The 'buttons' section removal is safely implemented
The search results show that $sections['buttons'] is only referenced once in the codebase, and it's properly guarded by the conditional check that verifies if the related hooks are present. There are no other unconditional references to this array key that could cause undefined index errors.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of $sections['buttons'] without prior checks.
# Find all occurrences of $sections['buttons'] in the codebase.
rg --php '$sections\["buttons"\]'
Length of output: 117
Script:
#!/bin/bash
# Search for any references to $sections['buttons'] with different quote styles
rg --php -A 3 -B 3 '\$sections\[(["'\''])buttons\1\]'
Length of output: 463
Script:
#!/bin/bash
# Search for any references to $sections['buttons'] with different quote styles
rg -A 3 -B 3 '\$sections\[(["'\''])buttons\1\]'
# Also search for array access with curly braces syntax
rg -A 3 -B 3 '\$sections\{["'\'']buttons["\'']\}'
Length of output: 435
Script:
#!/bin/bash
# Search for any references to $sections['buttons'] or $sections["buttons"]
rg -A 3 -B 3 '\$sections\[(["'\''])buttons["'\''\]]'
# Also search for the string 'buttons' within the context of $sections
rg -A 3 -B 3 '\$sections.*buttons'
Length of output: 1141
|
It seems something about this update causes the e2e tests to fail. |
|
@lauramekaj1 Maybe you can help with this failing unit test issue we're having here. This update really just removes the "Buttons" section in form settings. Is there a test that is checking for this somewhere that needs to be updated? Thank you! |
@Crabcyborg yes sure I will look into this! |
|
@Crabcyborg @truongwp When I switch to the Note: If you have Formidable Forms Pro activated, then deactivate and you should be able to reproduce this issue. Let me know if you need further information! |
|
Thank you @lauramekaj1! So there's a real bug here. @truongwp I'm seeing this fatal error in my logs |
|
Thanks @lauramekaj1 and @Crabcyborg. I fixed the fatal error. I'm not sure why it didn't trigger that fatal error when I tested before the commit 43aee7a |
|
@truongwp I don't believe that passing a |
|
@Crabcyborg Oh sorry, I fixed it. |
3 participants
Related PR: https://github.com/Strategy11/formidable-pro/pull/5479
In the related PR, we won't use
frm_add_form_style_tab_optionsandfrm_add_form_button_optionshooks to print hidden inputs anymore. If these hooks are still used by custom code, we will keep the Buttons settings tab, and show the deprecation notices. If these hooks aren't used, we remove the Buttons settings tab completely.