QoL Release - Lite Version#2215
Conversation
shervElmi
added
action: needs qa
run analysis
deploy
WalkthroughThis pull request introduces changes across several components of the admin interface. The updates include modifications to the way admin notices are handled and the introduction of user-specific sorting preferences. Key controller methods are renamed and refactored, new helper functions for rendering and sorting are added, and several stylistic improvements are made to the CSS. In addition, new JavaScript functions for form builder interactions are implemented. The changes also simplify certain conditional checks by leveraging helper methods. Changes
Possibly related PRs
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)
⏰ Context from checks skipped due to timeout of 90000ms (4)
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. 🪧 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
🧹 Nitpick comments (5)
classes/views/frm-fields/back-end/default-value-setting.php (1)
15-19: LGTM! The conditional display adds flexibility.The changes add the ability to control the display of the smart values icon while maintaining backward compatibility by defaulting to true.
Consider using PHP's null coalescing operator for a more concise initialization:
-$display_smart_values_icon = isset( $display_smart_values_icon ) ? $display_smart_values_icon : true; +$display_smart_values_icon = $display_smart_values_icon ?? true;classes/controllers/FrmAppController.php (1)
1356-1360: Improve method documentation.The current docblock is minimal. Consider adding more details about the method's purpose and the filters it provides.
/** - * Remembers and applies user-specific sorting preferences. + * Manages and persists user-specific sorting preferences for admin list views. * + * This method handles both saving new sort preferences when explicitly set, + * and retrieving saved preferences to maintain consistent sorting across sessions. + * + * @since x.x.x + * + * @uses-filter frm_default_list_orderby To customize the default orderby value + * @uses-filter frm_default_list_order To customize the default sort order + * @uses-filter frm_list_sort To apply the sort preferences + * * @return void */js/formidable_admin.js (1)
8428-8470: Consider adding more context to the reload messageThe reload message could be more specific about what settings might be affected by the field type change.
-message = __( 'You are changing the field type. Not all field settings will appear as expected until you reload the page. Would you like to reload the page now?', 'formidable' ); +message = __( 'You are changing the field type. Some field-specific settings and validations may not appear correctly until you reload the page. Would you like to save your changes and reload now?', 'formidable' );css/frm_admin.css (2)
784-791: Consider adding a transition for smoother border color changes.The droppable target styles could benefit from a transition on the border-color property for smoother visual feedback.
.frm-over-droppable + .frm_no_fields { border-style: solid; + transition: border-color 0.2s ease-in-out; border-color: var(--primary-500); outline-color: var(--primary-500); }
817-820: Consider using display property instead of visibility for better performance.Using
display: noneinstead ofvisibility: hiddencan improve rendering performance since hidden elements won't take up space in the layout..frm-has-fields .frm_no_fields, #frm_form_editor_container:not(.frm-has-fields) #frm_drag_placeholder { - display: none; + display: none !important; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
classes/controllers/FrmAppController.php(1 hunks)classes/controllers/FrmHooksController.php(1 hunks)classes/helpers/FrmFormsListHelper.php(2 hunks)classes/helpers/FrmListHelper.php(1 hunks)classes/views/frm-fields/back-end/default-value-setting.php(1 hunks)classes/views/frm-fields/back-end/settings.php(1 hunks)css/frm_admin.css(8 hunks)js/formidable_admin.js(3 hunks)
🔇 Additional comments (18)
classes/helpers/FrmListHelper.php (1)
394-394: LGTM! Good accessibility improvements.The addition of
tabindex='-1'andaria-hidden='true'attributes to the hidden confirmation link improves accessibility by properly removing the element from both keyboard navigation and screen reader announcements. This aligns with WCAG guidelines for managing hidden interactive elements.classes/helpers/FrmFormsListHelper.php (2)
374-387: Well-structured implementation of the view form icon.The new method is well-documented, properly scoped, and follows security best practices with proper escaping.
338-338: Clean integration of the view icon.The view icon is cleanly integrated into the existing actions column.
classes/controllers/FrmHooksController.php (1)
130-130: LGTM! Well-placed hook for custom sort functionality.The hook is appropriately placed in the admin hooks section with priority 0 to ensure early execution.
classes/views/frm-fields/back-end/settings.php (1)
205-205: LGTM! Well-placed hook for field options extensibility.The new action hook follows the established pattern and provides the necessary context data for extensions.
js/formidable_admin.js (7)
2549-2550: LGTM! Cleanup after drag and dropGood cleanup to remove the droppable highlight class after field insertion.
10384-10410: Improve UX by adding Enter key navigation for field optionsGood enhancement to allow users to press Enter to move to the next input field when editing field options. The cursor is also correctly positioned at the end of the text.
10355-10379: LGTM! Initialize select dependenciesGood implementation of dependency management for select elements. The code properly handles initial visibility and updates it on change events.
8397-8421: Good handling of field type changesThe code properly shows a "Save and Reload" modal when changing field types, which is important for maintaining field settings consistency.
6706-6724: LGTM! Phone format input adjustmentGood implementation to handle phone format input adjustments before form save.
10402-10410: Verify keyboard navigation behaviorThe Enter key navigation for field options is a good UX improvement, but we should verify it works correctly with:
- Fields that are hidden/disabled
- The last field in a group
- Different field types
6695-6705: Verify phone format handlingThe phone format input adjustment needs to be tested with:
- Different phone types
- International formats
- Custom formats
✅ Verification successful
Verification of Phone Format Handling in js/formidable_admin.js
- The code in js/formidable_admin.js adjusts the phone format input as described:
- It clears the format field when the value is set to "none" (e.g., when a custom phone type with an empty format is selected).
- It explicitly sets the input value to "international" when the international type is selected.
- Additional evidence from unit tests in tests/phpunit/fields/test_FrmFieldValidate.php confirms that both default and custom phone format configurations are validated properly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for phone format related code rg -A 5 'phone.*format'Length of output: 10098
css/frm_admin.css (6)
1499-1506: LGTM! Hover styles for preview button and search submit are well-defined.The hover styles for the preview button and search submit are properly defined with appropriate color changes and transitions.
1731-1733: LGTM! New margin utility class added.The new
.frm-mr-2xsutility class for extra small right margin is consistent with the existing margin utility classes.
1800-1802: LGTM! New padding utility class added.The new
.frm-pt-mdutility class for medium top padding follows the established pattern for padding utilities.
1815-1818: LGTM! New padding utility class added.The new
.frm-px-smutility class for small horizontal padding is consistent with existing padding utilities.
1885-1887: LGTM! New width utility class added.The new
.frm-w-halfutility class for 50% width follows the established pattern for width utilities.
2020-2022: LGTM! New alignment utility class added.The new
.frm-align-baselineutility class for vertical alignment is consistent with existing alignment utilities.
|
@shervElmi QoL
This isn't a link to the preview page. We already link to that in Quick Actions. We don't want a redundant URL here. The page we're referring to is the link to the Views tab (See https://qa.formidableforms.com/sherv11/wp-admin/admin.php?page=formidable-views&form=16 / https://qa.formidableforms.com/sherv11/wp-admin/edit.php?post_type=frm_display&show_nav=0&form=16). |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
classes/helpers/FrmAppHelper.php (1)
334-338: Update version tag in documentation block.The
@since x.xtag should be updated with the actual version number where this method was introduced.- * @since x.x + * @since 6.18
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
classes/controllers/FrmAppController.php(2 hunks)classes/controllers/FrmUsageController.php(1 hunks)classes/helpers/FrmAppHelper.php(1 hunks)classes/helpers/FrmListHelper.php(2 hunks)classes/views/frm-fields/back-end/settings.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- classes/views/frm-fields/back-end/settings.php
- classes/helpers/FrmListHelper.php
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (4)
classes/controllers/FrmUsageController.php (1)
108-110:Details
✅ Verification successful
LGTM! Good refactoring to improve code organization.
The change follows good software engineering practices by moving the implementation details to a helper class, improving code reusability and maintainability.
Please verify that
FrmAppHelper::is_admin_list_page()maintains the same behavior as the original implementation for script loading purposes. Run this script to compare the implementations:
🏁 Script executed:
#!/bin/bash # Description: Compare the original and new implementations to ensure consistent behavior. # Search for the original implementation echo "Original implementation:" git log -p -- classes/controllers/FrmUsageController.php | rg -A 10 "is_forms_list_page" # Search for the new implementation echo -e "\nNew implementation:" rg -A 10 "is_admin_list_page" classes/helpers/FrmAppHelper.phpLength of output: 454
Change Verified: Refactoring Improves Maintainability & Preserves Behavior
The updated
is_forms_list_page()method correctly delegates its logic toFrmAppHelper::is_admin_list_page(), and the helper’s implementation (checking admin page status and handling the Trash page via the form type check) aligns with the original intent for script loading. The refactoring enhances code organization and reusability without altering functionality.
- Verify that edge cases around the form type remain covered in QA.
classes/helpers/FrmAppHelper.php (1)
334-355: LGTM! Clean implementation for checking admin list pages.The method provides a clear, reusable way to check if the current page is an admin list page. The logic is sound and follows proper validation steps:
- Validates admin page status
- Checks form type is 'published'
- Verifies no action is set
classes/controllers/FrmAppController.php (2)
1348-1371: LGTM! Clean implementation of admin notice filtering.The code effectively filters out third-party notices while preserving Formidable's own notices. The helper methods
is_our_callback_stringandis_our_callback_arraymake the code more maintainable.
1334-1341:Details
❓ Verification inconclusive
Verify the impact on performance.
The
handle_current_screen()method is called on every admin page load. While it has a guard clause to checkin_our_pages(), consider caching the result of this check to avoid repeated checks in child methods.
🏁 Script executed:
#!/bin/bash # Check how often this method is called rg -A 5 "handle_current_screen" --type phpLength of output: 1021
Performance Optimization Suggestion for Admin Screen Handling
The
handle_current_screen()method is invoked on every admin page load (as set inFrmHooksController.php). While the current guard clause usingin_our_pages()appropriately limits execution to relevant pages, please evaluate whether the cost of runningin_our_pages()on each call might impact performance, especially if its logic grows more complex. Caching or memoizing its result for the duration of a request could reduce redundant checks across child methods likefilter_admin_notices()andremember_custom_sort().
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
classes/controllers/FrmAppController.php (2)
1373-1398:⚠️ Potential issue❓ Verification inconclusive
Add input validation for sorting parameters.
The current implementation of
remember_custom_sort()lacks validation for theorderbyandorderparameters, which creates potential security risks. It also directly updates user meta without checking for successful updates.Consider implementing this safer version:
private static function remember_custom_sort() { if ( ! FrmAppHelper::is_admin_list_page() && ! FrmAppHelper::is_admin_list_page( 'formidable-entries' ) ) { return; } $screen = get_current_screen(); - $orderby = FrmAppHelper::get_param( 'orderby' ); + $allowed_orderby = array( 'name', 'id', 'created_at', 'title', 'description', 'form_key' ); // Add all allowed values + $allowed_order = array( 'asc', 'desc' ); + + $orderby = in_array( FrmAppHelper::get_param( 'orderby' ), $allowed_orderby, true ) ? + FrmAppHelper::get_param( 'orderby' ) : ''; + $order = in_array( strtolower( FrmAppHelper::get_param( 'order' ) ), $allowed_order, true ) ? + FrmAppHelper::get_param( 'order' ) : ''; if ( ! $screen || ! $orderby ) { return; } + $user_id = get_current_user_id(); + $meta_key = 'frm_preferred_list_sort_' . $screen->id; + $meta_value = array( + 'orderby' => $orderby, + 'order' => $order ? $order : 'asc', + ); + + $updated = update_user_meta( + $user_id, + $meta_key, + $meta_value + ); + + if ( false === $updated ) { + // Log error or handle the failure + do_action( 'frm_sorting_preference_update_failed', array( + 'user_id' => $user_id, + 'meta_key' => $meta_key, + 'meta_value' => $meta_value, + )); + } - update_user_meta( - get_current_user_id(), - 'frm_preferred_list_sort_' . $screen->id, - array( - 'orderby' => $orderby, - 'order' => FrmAppHelper::get_param( 'order' ), - ) - ); }
🏁 Script executed:
#!/bin/bash # Search for allowed sort values in the codebase echo "Searching for allowed orderby values..." rg -p "(?:allowedOrderby|allowed_orderby|validOrderby|valid_orderby|\borderbys?=|sort\s+by)" --type phpLength of output: 194
Security: Validate Sorting Parameters & Ensure Meta Update Success
The current implementation of
remember_custom_sort()lacks necessary validation on theorderbyandorderparameters, which could expose the application to security risks. Moreover, it does not verify whether the update to the user meta data succeeded. Our search for predefined allowed sorting parameters in the codebase returned no relevant results, so explicit allowed values should be defined within this function.Recommended Actions:
Input Validation:
Define explicit arrays of allowed values fororderby(e.g.,'name','id', etc.) and fororder('asc','desc'). Validate the incoming parameters against these arrays before proceeding.Meta Update Verification:
Capture the return value ofupdate_user_meta()and handle the failure case (e.g., by triggering an error action or logging).Suggested Revision:
private static function remember_custom_sort() { if ( ! FrmAppHelper::is_admin_list_page() && ! FrmAppHelper::is_admin_list_page( 'formidable-entries' ) ) { return; } $screen = get_current_screen(); - $orderby = FrmAppHelper::get_param( 'orderby' ); + $allowed_orderby = array( 'name', 'id', 'created_at', 'title', 'description', 'form_key' ); // Define allowed columns + $allowed_order = array( 'asc', 'desc' ); + + $orderby = in_array( FrmAppHelper::get_param( 'orderby' ), $allowed_orderby, true ) + ? FrmAppHelper::get_param( 'orderby' ) : ''; + $order = in_array( strtolower( FrmAppHelper::get_param( 'order' ) ), $allowed_order, true ) + ? FrmAppHelper::get_param( 'order' ) : ''; if ( ! $screen || ! $orderby ) { return; } + $user_id = get_current_user_id(); + $meta_key = 'frm_preferred_list_sort_' . $screen->id; + $meta_value = array( + 'orderby' => $orderby, + 'order' => $order ? $order : 'asc', + ); + + $updated = update_user_meta( $user_id, $meta_key, $meta_value ); + if ( false === $updated ) { + // Log error or handle the failure appropriately + do_action( 'frm_sorting_preference_update_failed', array( + 'user_id' => $user_id, + 'meta_key' => $meta_key, + 'meta_value'=> $meta_value, + )); + } - update_user_meta( - get_current_user_id(), - 'frm_preferred_list_sort_' . $screen->id, - array( - 'orderby' => $orderby, - 'order' => FrmAppHelper::get_param( 'order' ), - ) - ); }Please verify that the allowed sort values match your application's requirements and that the error handling for the user meta update aligns with your logging and error management standards.
1399-1429: 🛠️ Refactor suggestionAdd validation and default values for the saved sort preferences.
The
apply_saved_sort_preferencemethod doesn't validate retrieved preferences or provide default values if the preferences are invalid or missing. This can lead to unpredictable behavior.Apply this improved implementation:
public static function apply_saved_sort_preference( &$orderby, &$order ) { if ( ! function_exists( 'get_current_screen' ) ) { return; } $screen = get_current_screen(); if ( ! $screen ) { return; } $user_id = get_current_user_id(); $preferred_list_sort = get_user_meta( $user_id, 'frm_preferred_list_sort_' . $screen->id, true ); + $allowed_orderby = array( 'name', 'id', 'created_at', 'title', 'description', 'form_key' ); // Add all allowed values + $allowed_order = array( 'asc', 'desc' ); - if ( is_array( $preferred_list_sort ) && ! empty( $preferred_list_sort['orderby'] ) ) { + if ( is_array( $preferred_list_sort ) && + ! empty( $preferred_list_sort['orderby'] ) && + in_array( $preferred_list_sort['orderby'], $allowed_orderby, true ) + ) { $orderby = $preferred_list_sort['orderby']; - if ( ! empty( $preferred_list_sort['order'] ) ) { + if ( ! empty( $preferred_list_sort['order'] ) && + in_array( strtolower( $preferred_list_sort['order'] ), $allowed_order, true ) + ) { $order = $preferred_list_sort['order']; + } else { + $order = 'asc'; // Default order } + } else { + $orderby = apply_filters( 'frm_default_list_orderby', 'name', $screen ); + $order = apply_filters( 'frm_default_list_order', 'asc', $screen ); } }
🧹 Nitpick comments (1)
classes/controllers/FrmAppController.php (1)
1379-1380: Avoid database updates on page load.This function gets called on every admin page load and can trigger database writes even when the user hasn't changed their sort preferences, which is inefficient.
Consider only updating when the sort order has actually changed by comparing with existing values:
private static function remember_custom_sort() { - if ( ! FrmAppHelper::is_admin_list_page() && ! FrmAppHelper::is_admin_list_page( 'formidable-entries' ) ) { + // Only save sort preferences when explicitly changing them via GET parameters + if ( ! isset( $_GET['orderby'] ) || + ( ! FrmAppHelper::is_admin_list_page() && ! FrmAppHelper::is_admin_list_page( 'formidable-entries' ) ) ) { return; } // Rest of the function... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
classes/controllers/FrmAppController.php(2 hunks)classes/helpers/FrmAppHelper.php(1 hunks)css/frm_admin.css(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (12)
classes/helpers/FrmAppHelper.php (1)
339-360: Clean and well-documented utility method for checking admin list pages.The new
is_admin_list_page()method provides a convenient way to determine if the current page is an admin list page. This addition is well-structured with proper documentation and follows the established pattern in the helper class.css/frm_admin.css (9)
1548-1556: Enhanced button hover effects for better visual feedbackThe hover states for buttons in
.frm_wrap .previewclass and the search submit button have been improved to provide clearer visual feedback to users.
1781-1783: LGTM: New utility class for extra small right marginNew utility class
.frm-mr-2xsadds a consistent extra small right margin that can be reused throughout the UI.
1855-1857: LGTM: New utility class for medium top paddingNew utility class
.frm-pt-mdadds a consistent medium-sized top padding that matches the design system variables.
1869-1873: LGTM: New utility class for small horizontal paddingNew utility class
.frm-px-smadds consistent small padding on both left and right sides simultaneously.
1940-1942: LGTM: New utility class for half-width elementsNew utility class
.frm-w-halfoffers a convenient way to create two-column layouts by setting width to 50%.
2080-2082: LGTM: New utility class for baseline vertical alignmentNew utility class
.frm-align-baselineprovides a way to align inline elements along their text baseline.
6845-6846: Improved droppable area visualization with transparent outlineAdding a transparent outline to the
.frm_no_fieldsclass creates a foundation for the hover/drag state that follows, creating a more consistent transition between states.
6849-6852: Enhanced visual feedback for drag-and-drop interactionsWhen a draggable item is over the drop area, the border and outline colors now change to the primary color, providing clearer visual feedback to users that they can drop an item here.
6878-6881: Improved conditional display logic for empty stateThis change ensures the empty state message is only shown when needed, and handles the placeholder display appropriately based on whether fields exist or not.
classes/controllers/FrmAppController.php (2)
1327-1341: Good addition of a centralized screen handler method.The new
handle_current_screen()method provides a clean entry point for performing actions specific to the Formidable admin pages. This is a good architectural improvement over directly hooking multiple functions.
1342-1371: Maintain public access as recommended in previous feedback.Per previous review comments, you've correctly kept this method public since changing it to private could break backward compatibility with code that might be calling it directly.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2215 +/- ##
============================================
- Coverage 26.95% 26.89% -0.07%
- Complexity 8328 8344 +16
============================================
Files 129 129
Lines 27508 27577 +69
============================================
+ Hits 7415 7416 +1
- Misses 20093 20161 +68 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
classes/controllers/FrmAppController.php (1)
1343-1371:⚠️ Potential issueFunction visibility changed from public to private.
The
filter_admin_noticesmethod was previously public and is now being called only internally. However, changing a public method to private can break backward compatibility if it's called elsewhere in the codebase or by third-party plugins.This change should be reverted to maintain backward compatibility:
-private static function filter_admin_notices() { +public static function filter_admin_notices() {
♻️ Duplicate comments (1)
classes/controllers/FrmAppController.php (1)
1373-1415: 🛠️ Refactor suggestionAdd input validation and optimize database operations.
The
remember_custom_sortmethod needs to validate input parameters before saving to the database and should only update when necessary (which you've implemented in lines 1405-1407). However, there are still areas for improvement:
- No validation of allowed sorting values
- Potential performance impact by updating user meta on every page load with sorting parameters
- No error handling for meta updates
Apply these improvements:
private static function remember_custom_sort() { $screen = get_current_screen(); if ( ! $screen ) { return; } if ( ! FrmAppHelper::is_admin_list_page() && ! FrmAppHelper::is_admin_list_page( 'formidable-entries' ) ) { return; } - $orderby = FrmAppHelper::get_param( 'orderby' ); + $allowed_orderby = array( 'name', 'id', 'created_at' ); // Add all allowed values + $allowed_order = array( 'asc', 'desc' ); + + $orderby = in_array( FrmAppHelper::get_param( 'orderby' ), $allowed_orderby, true ) ? + FrmAppHelper::get_param( 'orderby' ) : ''; + $order = in_array( strtolower( FrmAppHelper::get_param( 'order' ) ), $allowed_order, true ) ? + FrmAppHelper::get_param( 'order' ) : ''; if ( ! $orderby ) { return; } $user_id = get_current_user_id(); $meta_key = 'frm_preferred_list_sort_' . $screen->id; $order = FrmAppHelper::get_param( 'order' ); $new_sort = array( 'orderby' => $orderby, 'order' => $order, ); $current_sort = get_user_meta( $user_id, $meta_key, true ); if ( $new_sort !== $current_sort ) { - update_user_meta( - $user_id, - $meta_key, - array( - 'orderby' => $orderby, - 'order' => $order, - ) - ); + $updated = update_user_meta( $user_id, $meta_key, $new_sort ); + if ( false === $updated ) { + // Log error or handle the failure + do_action( 'frm_sorting_preference_update_failed', array( + 'user_id' => $user_id, + 'meta_key' => $meta_key, + 'meta_value' => $new_sort, + )); + } } }
🧹 Nitpick comments (1)
classes/controllers/FrmAppController.php (1)
1417-1446: Add validation and default values for sort preferences.The
apply_saved_sort_preferencemethod should validate the retrieved preferences and provide default values to ensure robust behavior.Apply this improvement:
public static function apply_saved_sort_preference( &$orderby, &$order ) { if ( ! function_exists( 'get_current_screen' ) ) { return; } $screen = get_current_screen(); if ( ! $screen ) { return; } $user_id = get_current_user_id(); $preferred_list_sort = get_user_meta( $user_id, 'frm_preferred_list_sort_' . $screen->id, true ); - if ( is_array( $preferred_list_sort ) && ! empty( $preferred_list_sort['orderby'] ) ) { - $orderby = $preferred_list_sort['orderby']; + $allowed_orderby = array( 'name', 'id', 'created_at' ); // Add all allowed values + $allowed_order = array( 'asc', 'desc' ); + + if ( is_array( $preferred_list_sort ) && + ! empty( $preferred_list_sort['orderby'] ) && + in_array( $preferred_list_sort['orderby'], $allowed_orderby, true ) + ) { + $orderby = $preferred_list_sort['orderby']; - if ( ! empty( $preferred_list_sort['order'] ) ) { + if ( ! empty( $preferred_list_sort['order'] ) && + in_array( strtolower( $preferred_list_sort['order'] ), $allowed_order, true ) + ) { $order = $preferred_list_sort['order']; + } else { + $order = 'asc'; // Default order } + } else { + $orderby = apply_filters( 'frm_default_list_orderby', 'name', $screen ); + $order = apply_filters( 'frm_default_list_order', 'asc', $screen ); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/controllers/FrmAppController.php(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (1)
classes/controllers/FrmAppController.php (1)
1327-1341: Good centralization of screen-related functionality.The new
handle_current_screenmethod centralizes screen-related actions, which makes the code easier to maintain and understand. It checks if we're in Formidable admin pages before executing specific actions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
classes/controllers/FrmAppController.php (3)
1430-1450: Add validation for retrieved sort preferences.The current implementation doesn't validate the retrieved preferences before applying them, which could lead to issues if the stored data is corrupted.
Consider adding validation for the retrieved preferences:
public static function apply_saved_sort_preference( &$orderby, &$order ) { if ( ! function_exists( 'get_current_screen' ) ) { return; } $screen = get_current_screen(); if ( ! $screen ) { return; } $user_id = get_current_user_id(); $preferred_list_sort = get_user_meta( $user_id, 'frm_preferred_list_sort_' . $screen->id, true ); - if ( is_array( $preferred_list_sort ) && ! empty( $preferred_list_sort['orderby'] ) ) { + // Validate allowed orderby values + $allowed_orderby = array( 'name', 'id', 'created_at', 'updated_at', 'title' ); // Add all actual allowed values + $allowed_order = array( 'asc', 'desc' ); + + if ( is_array( $preferred_list_sort ) && + ! empty( $preferred_list_sort['orderby'] ) && + in_array( $preferred_list_sort['orderby'], $allowed_orderby, true ) ) { $orderby = $preferred_list_sort['orderby']; - if ( ! empty( $preferred_list_sort['order'] ) ) { + if ( ! empty( $preferred_list_sort['order'] ) && + in_array( strtolower( $preferred_list_sort['order'] ), $allowed_order, true ) ) { $order = $preferred_list_sort['order']; + } else { + // Default to ascending if order is invalid or missing + $order = 'asc'; } + } else { + // Set defaults if preferences are invalid + $orderby = apply_filters( 'frm_default_list_orderby', 'name', $screen ); + $order = apply_filters( 'frm_default_list_order', 'asc', $screen ); } }
1408-1409: Add typecasting for better code quality.While checking the current sort with the new sort, it's a good practice to ensure the values have the same type.
- $current_sort = get_user_meta( $user_id, $meta_key, true ); + $current_sort = (array) get_user_meta( $user_id, $meta_key, true );
1399-1401: Consider using a helper function for meta key consistency.The meta key format is used in both
remember_custom_sort()andapply_saved_sort_preference(). Consider extracting it to a helper method to maintain consistency.+ /** + * Get the meta key for storing user sort preferences + * + * @param object $screen Current screen object + * @return string Meta key + */ + private static function get_sort_meta_key( $screen ) { + return 'frm_preferred_list_sort_' . $screen->id; + } private static function remember_custom_sort() { // ... $user_id = get_current_user_id(); - $meta_key = 'frm_preferred_list_sort_' . $screen->id; + $meta_key = self::get_sort_meta_key( $screen ); // ... } public static function apply_saved_sort_preference( &$orderby, &$order ) { // ... $user_id = get_current_user_id(); - $preferred_list_sort = get_user_meta( $user_id, 'frm_preferred_list_sort_' . $screen->id, true ); + $preferred_list_sort = get_user_meta( $user_id, self::get_sort_meta_key( $screen ), true ); // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/controllers/FrmAppController.php(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Cypress
- GitHub Check: Run PHP Syntax inspection (8.3)
🔇 Additional comments (3)
classes/controllers/FrmAppController.php (3)
1331-1345: New method to centralize screen-related actions.This new public method improves code organization by centralizing actions related to the current screen, making the codebase more maintainable.
1347-1375: Ensure third-party notices don't appear in Formidable admin pages.The implementation of this method correctly filters out third-party admin notices while preserving Formidable's own notices. This is a good practice that helps focus the admin UI on relevant information.
1330-1330:Details
❓ Verification inconclusive
Ensure backward compatibility for public methods.
The method signature has changed from
public static function filter_admin_notices()to a private method, which is potentially breaking for plugins that might be calling this method directly.
🏁 Script executed:
#!/bin/bash # Check if any third-party code is directly calling filter_admin_notices rg -i --no-filename "FrmAppController::filter_admin_notices" --iglob "!classes/controllers/FrmAppController.php"Length of output: 112
Ensure backward compatibility for
filter_admin_noticesremains intactOur initial repository-wide search (excluding the controller file) for references to
FrmAppController::filter_admin_noticesreturned no hits. However, since this method was previously public, external plugins might have relied on its availability—even if not detected in our internal code scan. Please manually verify external dependencies and consider providing either a public alias or a deprecated wrapper to maintain backward compatibility if any such external usage exists.
2 participants
Provides a series of small fixes to improve QoL when making forms.
Pro PR:
https://github.com/Strategy11/formidable-pro/pull/5575
QA URL:
https://qa.formidableforms.com/sherv11/wp-admin/admin.php?page=formidable&frm_action=edit&id=16
How to test
See the Feature Brief document for the task in Linear. You can find the items we improved there with full descriptions.