Project: Add option to limit field choices selection #2513
Conversation
AbdiTolesa
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
![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
🧹 Nitpick comments (1)
classes/views/frm-fields/front-end/checkbox-field.php (1)
76-78: A11y nit: avoidaria-requiredon a disabled checkboxIf the first option is disabled due to limit reached, adding
aria-required="true"on that input is misleading. Consider applyingaria-requiredon the group (fieldset/legend) or only when the targeted input isn’t disabled. For example:if ( 0 === $option_index && FrmField::is_required( $field ) && ! $disabled ) { echo ' aria-required="true" '; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
classes/controllers/FrmFieldsController.php(1 hunks)classes/views/frm-fields/front-end/checkbox-field.php(1 hunks)classes/views/frm-fields/front-end/dropdown-field.php(1 hunks)classes/views/frm-fields/front-end/radio-field.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
classes/controllers/FrmFieldsController.php (1)
classes/models/FrmField.php (2)
FrmField(6-1501)is_field_type(1468-1486)
classes/views/frm-fields/front-end/checkbox-field.php (1)
classes/controllers/FrmFieldsController.php (2)
FrmFieldsController(6-1037)maybe_disable_option(388-397)
classes/views/frm-fields/front-end/dropdown-field.php (1)
classes/controllers/FrmFieldsController.php (2)
FrmFieldsController(6-1037)maybe_disable_option(388-397)
classes/views/frm-fields/front-end/radio-field.php (1)
classes/controllers/FrmFieldsController.php (2)
FrmFieldsController(6-1037)maybe_disable_option(388-397)
🔇 Additional comments (2)
classes/views/frm-fields/front-end/radio-field.php (1)
58-61: LGTM: checked gating aligns with disabled stateConditionally emitting
checkedonly when not disabled is correct and prevents selecting disabled radios. Will continue to work after the controller’s$echodefault remains true.classes/views/frm-fields/front-end/checkbox-field.php (1)
71-74: LGTM: prevents checked state on disabled optionsOnly emitting
checkedwhen not disabled is correct and matches the intended quota behavior.
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
@AbdiTolesa How possible is it to do this using frm_field_input_html and not requiring any other changes in Lite?
I tried it with those lines added in Pro: https://github.com/Strategy11/formidable-pro/pull/6005/commits/574a8460860f28cccbbabc403401f521ccb59ee2#diff-a96a88bcd18b41a7802482b18e4a1b6f8780d53899653edd4033c1dc61e2d9a0R367-R376 @Crabcyborg Could you respond to this one too? |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/views/frm-fields/front-end/radio-field.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/views/frm-fields/front-end/radio-field.php (1)
classes/controllers/FrmFieldsController.php (1)
input_html(532-557)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
classes/views/frm-fields/front-end/radio-field.php (1)
24-32: Replace string-based signaling with direct method call.This implementation uses a fragile 'limit_reached' string marker captured from action output buffering. As noted in the previous review, this approach is brittle and inconsistent with the checkbox and dropdown field implementations, which call
FrmFieldsController::maybe_disable_option($field, $opt_key)directly.Refactor to match the established pattern:
- ob_start(); - do_action( 'frm_field_input_html', $field, $opt_key ); - $input_html = ob_get_clean(); - $choice_limit_reached = strpos( $input_html, 'limit_reached' ) !== false; - if ( ! empty( $form_options['disable_on_choice_limit'] ) ) { - if ( $choice_limit_reached ) { - continue; - } - } + $is_disabled = method_exists( 'FrmFieldsController', 'maybe_disable_option' ) + ? FrmFieldsController::maybe_disable_option( $field, $opt_key ) + : false; + + if ( ! empty( $form_options['disable_on_choice_limit'] ) && $is_disabled ) { + continue; + }This eliminates the string marker dependency and aligns with the approach used in
checkbox-field.phpanddropdown-field.php.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/views/frm-fields/front-end/radio-field.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/views/frm-fields/front-end/radio-field.php (3)
classes/models/FrmDb.php (2)
FrmDb(6-759)get_var(209-223)classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4622)unserialize_or_decode(3289-3299)classes/controllers/FrmFieldsController.php (1)
input_html(532-557)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
css/admin/frm-settings-components.css(1 hunks)css/font_icons.css(1 hunks)js/formidable_dashboard.js(1 hunks)js/formidable_overlay.js(1 hunks)resources/scss/admin/components/option/_single-option.scss(0 hunks)
💤 Files with no reviewable changes (1)
- resources/scss/admin/components/option/_single-option.scss
✅ Files skipped from review due to trivial changes (1)
- css/font_icons.css
🧰 Additional context used
🧬 Code graph analysis (1)
js/formidable_dashboard.js (1)
js/src/admin/admin.js (3)
e(8322-8322)inputId(8681-8681)
🪛 Biome (2.1.2)
js/formidable_dashboard.js
[error] 15-15: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 83-83: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 234-234: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 491-491: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 741-741: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
js/formidable_overlay.js
[error] 16-16: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 224-224: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 481-481: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
🔇 Additional comments (2)
js/formidable_overlay.js (1)
1-733: Verify this file's inclusion in the PR.This webpack-compiled bundle contains overlay and utility components that don't appear related to the PR objective of "limiting field choices selection." Additionally, reviewing compiled/bundled JavaScript files is not effective—source files should be reviewed instead.
Verify whether this file was included intentionally or if only source files (js/src/**) should be reviewed.
js/formidable_dashboard.js (1)
1-854: Verify this file's inclusion in the PR.This webpack-compiled bundle contains dashboard components (counters, tabs navigator, inbox) that don't appear related to the PR objective of "limiting field choices selection." As with the overlay file, reviewing compiled/bundled JavaScript is not effective—source files should be reviewed instead.
Verify whether this file was included intentionally or if only source files (js/src/**) should be reviewed.
|
Thanks @AbdiTolesa. It doesn't really look like anything better is coming up trying that way. Let's just try to make this work in whatever the most clean way we can is. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
classes/views/frm-fields/front-end/radio-field.php (3)
18-19: Consider caching form options to avoid repeated DB queries.Fetching form options on every render could impact performance, especially for forms with multiple choice fields. Consider caching the form options at the form level or passing them as a parameter to avoid repeated database queries.
21-21: Fix parameter count mismatch in maybe_disable_option call.The method signature of
FrmFieldsController::maybe_disable_option(lines 387-400 in classes/controllers/FrmFieldsController.php) accepts only 2 parameters ($field,$opt_key), but this call passes 3 parameters including$form_options.Apply this diff to fix the parameter count:
- $disabled = FrmFieldsController::maybe_disable_option( $field, $opt_key, $form_options ); + $disabled = FrmFieldsController::maybe_disable_option( $field, $opt_key );If
$form_optionsis needed by the filter hook insidemaybe_disable_option, consider adding it to the$fieldarray before calling the method, or update the method signature to accept a third parameter.
22-22: Verify FrmProFieldsController class availability in Lite version.Static analysis reports that
FrmProFieldsController::should_hide_field_choice()is called on an unknown class. This suggests the class may only exist in the Pro version. Since this PR targets the Lite version (branchlimit_choices_selection_lite_part), calling Pro-only methods will cause fatal errors.Run the following script to verify the class definition:
#!/bin/bash # Description: Check for FrmProFieldsController class definition and its availability # Search for the class definition rg -n "class\s+FrmProFieldsController" --type php # If not found in Lite, search for any conditional checks around Pro functionality rg -n "FrmProFieldsController" --type php -C 3If the class is Pro-only, wrap the call with a conditional check:
- if ( FrmProFieldsController::should_hide_field_choice( $disabled, $shortcode_atts, $opt_key, $form_options ) ) { + if ( class_exists( 'FrmProFieldsController' ) && FrmProFieldsController::should_hide_field_choice( $disabled, $shortcode_atts, $opt_key, $form_options ) ) { continue; }Or implement a Lite-compatible version of the method in
FrmFieldsController.
🧹 Nitpick comments (3)
classes/views/frm-fields/front-end/radio-field.php (2)
59-59: Consider adding data-max-reached for consistency.The disabled logic works correctly, but unlike checkbox-field.php (line 73), radio buttons don't emit
data-max-reached="1". While radios typically allow only one selection, adding this attribute would maintain consistency and could be useful for client-side logic.If desired, apply this diff:
- echo $disabled ? ' disabled="disabled" ' : $checked . ' '; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped + echo $disabled ? ' disabled="disabled" data-max-reached="1" ' : $checked . ' '; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
59-59: Add data-max-reached attribute for consistency with checkbox field.The checkbox field implementation (lines 72-76 in checkbox-field.php) includes
data-max-reached="1"when an option is disabled, which can be useful for client-side behavior or styling. The radio field should follow the same pattern for consistency.Apply this diff to align with the checkbox implementation:
- echo $disabled ? ' disabled="disabled" ' : $checked . ' '; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped + if ( $disabled ) { + echo ' disabled="disabled" data-max-reached="1" '; + } else { + echo $checked . ' '; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped + }Alternatively, if the
data-max-reachedattribute is not needed for radio buttons, document why the implementations differ.classes/views/frm-fields/front-end/checkbox-field.php (1)
19-20: Consider caching form options to avoid repeated DB queries.Fetching form options on every render could impact performance, especially for forms with multiple choice fields. Consider caching the form options at the form level or passing them as a parameter to avoid repeated database queries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
js/addons-page.js.mapis excluded by!**/*.map,!**/*.mapjs/form-templates.js.mapis excluded by!**/*.map,!**/*.mapjs/formidable-settings-components.js.mapis excluded by!**/*.map,!**/*.mapjs/onboarding-wizard.js.mapis excluded by!**/*.map,!**/*.map
📒 Files selected for processing (3)
classes/controllers/FrmFieldsController.php(1 hunks)classes/views/frm-fields/front-end/checkbox-field.php(2 hunks)classes/views/frm-fields/front-end/radio-field.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/controllers/FrmFieldsController.php
🧰 Additional context used
🧬 Code graph analysis (2)
classes/views/frm-fields/front-end/radio-field.php (3)
classes/models/FrmDb.php (2)
FrmDb(6-759)get_var(209-223)classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4622)unserialize_or_decode(3289-3299)classes/controllers/FrmFieldsController.php (2)
FrmFieldsController(6-1041)maybe_disable_option(388-401)
classes/views/frm-fields/front-end/checkbox-field.php (3)
classes/models/FrmDb.php (2)
FrmDb(6-759)get_var(209-223)classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4622)unserialize_or_decode(3289-3299)classes/controllers/FrmFieldsController.php (2)
FrmFieldsController(6-1041)maybe_disable_option(388-401)
🪛 GitHub Check: PHPStan
classes/views/frm-fields/front-end/radio-field.php
[failure] 22-22:
Call to static method should_hide_field_choice() on an unknown class FrmProFieldsController.
[failure] 21-21:
Static method FrmFieldsController::maybe_disable_option() invoked with 3 parameters, 2 required.
classes/views/frm-fields/front-end/checkbox-field.php
[failure] 23-23:
Call to static method should_hide_field_choice() on an unknown class FrmProFieldsController.
[failure] 22-22:
Static method FrmFieldsController::maybe_disable_option() invoked with 3 parameters, 2 required.
🔇 Additional comments (4)
classes/views/frm-fields/front-end/checkbox-field.php (3)
19-20: LGTM! Form options retrieved before loop.Good placement—fetching and decoding form options once before the loop prevents redundant database queries.
72-76: LGTM! Disabled state handled correctly.The conditional logic properly emits
disabled="disabled" data-max-reached="1"when disabled, and only outputs the checked attribute when enabled. Thedata-max-reachedattribute provides useful client-side context.
72-76: LGTM! Clear disabled state handling with data attribute.The conditional rendering properly handles the disabled state by outputting
disabled="disabled" data-max-reached="1"when the option is disabled, and preserving the existing checked behavior otherwise. Thedata-max-reachedattribute can be useful for client-side behavior.classes/views/frm-fields/front-end/radio-field.php (1)
18-19: LGTM! Addresses previous review feedback.Form options are now retrieved once before the loop, eliminating redundant database queries.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/views/frm-fields/front-end/dropdown-field.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/views/frm-fields/front-end/dropdown-field.php (4)
classes/models/FrmDb.php (2)
FrmDb(6-759)get_var(209-223)classes/helpers/FrmAppHelper.php (3)
FrmAppHelper(6-4622)unserialize_or_decode(3289-3299)check_selected(2234-2241)classes/helpers/FrmFieldsHelper.php (3)
FrmFieldsHelper(6-2508)get_value_from_array(668-672)get_label_from_array(674-678)classes/controllers/FrmFieldsController.php (2)
FrmFieldsController(6-1041)maybe_disable_option(388-401)
🪛 GitHub Check: PHPStan
classes/views/frm-fields/front-end/dropdown-field.php
[failure] 57-57:
Call to static method should_hide_field_choice() on an unknown class FrmProFieldsController.
🔇 Additional comments (1)
classes/views/frm-fields/front-end/dropdown-field.php (1)
79-82: Selection logic correct; echo suppression verified. maybe_disable_option returns only a boolean and contains no echo/print statements.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
classes/views/frm-fields/front-end/checkbox-field.php (1)
36-45: Update filter docblock to includefield_val.
The filter now receivescompact( 'field', 'field_val' ); docs should match.🛠️ Proposed fix
- * `@param` array $args The arguments. Contains `field`. + * `@param` array $args The arguments. Contains `field`, `field_val`.classes/views/frm-fields/front-end/radio-field.php (1)
36-45: Update filter docblock to includefield_val.
Docs still say args contain onlyfield, butfield_valis now passed.🛠️ Proposed fix
- * `@param` array $args The arguments. Contains `field`. + * `@param` array $args The arguments. Contains `field`, `field_val`.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@classes/views/frm-fields/front-end/checkbox-field.php`:
- Around line 41-44: Update the docblock for the 'frm_choice_field_option_label'
filter to reflect that the passed args include both $field and $field_val (not
just $field): update the `@param/`@args description above the apply_filters call
(where $label is set via apply_filters( 'frm_choice_field_option_label', $opt,
compact( 'field', 'field_val' ) )) so integrators know $field_val is provided.
🧹 Nitpick comments (1)
classes/views/frm-fields/front-end/radio-field.php (1)
47-49: Avoid doublecheck_selectedand reuse a boolean selection.
check_selectedis called again at Line 63; compute once and reuse for both$checkedandprepare_other_input.♻️ Refactor suggestion
- $checked = FrmAppHelper::check_selected( $field['value'], $field_val ) ? 'checked="checked" ' : ' '; - $should_echo_disabled_att = FrmFieldsHelper::should_echo_disabled_attribute( $opt_key, trim( $checked ) !== '', $field ); + $is_selected = FrmAppHelper::check_selected( $field['value'], $field_val ); + $checked = $is_selected ? 'checked="checked" ' : ' '; + $should_echo_disabled_att = FrmFieldsHelper::should_echo_disabled_attribute( $opt_key, $is_selected, $field ); @@ - $checked = FrmAppHelper::check_selected( $field['value'], $field_val ) ? 'checked="checked" ' : ' '; $other_opt = false; - $other_args = FrmFieldsHelper::prepare_other_input( compact( 'field_name', 'opt_key', 'field' ), $other_opt, $checked ); + $other_args = FrmFieldsHelper::prepare_other_input( compact( 'field_name', 'opt_key', 'field' ), $other_opt, $checked );
…m/Strategy11/formidable-forms into limit_choices_selection_lite_part
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@classes/views/frm-fields/front-end/dropdown-field.php`:
- Around line 87-94: The hook FrmFieldsHelper::after_choice_input is being
invoked inside the <option> element (between the option text and '</option>'),
which can inject invalid HTML; move the FrmFieldsHelper::after_choice_input(
$field, $opt_key ) call so it executes immediately after echo '</option>' (or
remove it entirely for dropdowns) and keep the existing calls to
FrmAppHelper::array_to_html_params( $option_params, true ), selected( $selected
), and esc_html( $opt === '' ? ' ' : $opt ) unchanged so option attributes and
text remain intact.
🧹 Nitpick comments (2)
classes/views/frm-fields/front-end/radio-field.php (1)
63-63: Redundant$checkedcomputation.
$checkedis computed at line 47 with identical logic. This second assignment at line 63 is unnecessary.Suggested fix
- $checked = FrmAppHelper::check_selected( $field['value'], $field_val ) ? 'checked="checked" ' : ' '; $other_opt = false;classes/views/frm-fields/front-end/checkbox-field.php (1)
60-60: Pass a boolean toshould_disable_optionfor consistency and type safety.
$checkedis a string (''or' checked="checked"') at this point, butshould_disable_optiondocuments$is_selected_choiceas a boolean. While this works because PHP casts the string to boolean, it's inconsistent with:
dropdown-field.phpwhich passes$selected(a proper boolean)radio-field.phpwhich passestrim( $checked ) !== ''(explicit boolean conversion)Suggested fix
- $should_echo_disabled_att = FrmFieldsHelper::should_disable_option( $opt_key, $checked, $field ); + $should_echo_disabled_att = FrmFieldsHelper::should_disable_option( $opt_key, '' !== $checked, $field );
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@classes/helpers/FrmFieldsHelper.php`:
- Around line 2879-2889: The docblock inside the after_choice_input method has a
parameter name mismatch: it uses $opt_key instead of $choice_key. To fix this,
update the docblock parameter name to $choice_key to align with the actual
method parameter and the argument passed to the frm_after_option_input action,
ensuring consistency and clarity for developers using the hook.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
classes/views/frm-fields/front-end/radio-field.php (1)
47-65: Compute disabled state afterprepare_other_inputto preserve selected “Other” options.
Line 48 derives disabled state beforeprepare_other_inputcan mark an “Other” choice as selected, so filters may disable a selected “Other” option and its value won’t submit.🛠️ Proposed fix
- $checked = FrmAppHelper::check_selected( $field['value'], $field_val ) ? 'checked="checked" ' : ' '; - $should_echo_disabled_att = FrmFieldsHelper::should_disable_option( $opt_key, trim( $checked ) !== '', $field ); - - if ( $include_label ) { + $checked = FrmAppHelper::check_selected( $field['value'], $field_val ) ? 'checked="checked" ' : ' '; + $other_opt = false; + $other_args = FrmFieldsHelper::prepare_other_input( compact( 'field_name', 'opt_key', 'field' ), $other_opt, $checked ); + $is_selected = trim( $checked ) !== ''; + $should_echo_disabled_att = FrmFieldsHelper::should_disable_option( $opt_key, $is_selected, $field ); + + if ( $include_label ) { $label_attributes = array( 'for' => $html_id . '-' . $opt_key, ); @@ - $checked = FrmAppHelper::check_selected( $field['value'], $field_val ) ? 'checked="checked" ' : ' '; - $other_opt = false; - $other_args = FrmFieldsHelper::prepare_other_input( compact( 'field_name', 'opt_key', 'field' ), $other_opt, $checked );
🤖 Fix all issues with AI agents
In `@classes/views/frm-fields/front-end/checkbox-field.php`:
- Line 60: The call to FrmFieldsHelper::should_disable_option currently passes
the HTML snippet $checked (a string) which breaks the hook's boolean contract;
update the invocation in the checkbox rendering so the second argument is a
boolean (e.g., cast the selection state or use a boolean expression such as !
empty( $checked ) or $checked === 'checked' ) when calling
FrmFieldsHelper::should_disable_option( $opt_key, ..., $field ) so filters
receive true/false rather than a string.
| * | ||
| * @return bool | ||
| */ | ||
| public static function should_hide_field_choice( $choice_key, $field ) { |
There was a problem hiding this comment.
@AbdiTolesa I'm thinking option might make sense here as well for consistency?
There was a problem hiding this comment.
@Crabcyborg Actually, I was in favor of field_choice instead of field_option as we use field_option to refer to any variable in $field->field_options across our plugins and choice makes it less vague, I used choice in all of the functions related to the this project in Pro too. If we should change this here, I think we need to make so many similar changes in Pro but I'll leave the decision to you after sharing my thoughts on this.
There was a problem hiding this comment.
Thanks @AbdiTolesa.
I mostly want consistency.
I think choice is fine, as long as we aren't using option too in the new code.
This PR requires:
https://github.com/Strategy11/formidable-pro/pull/6005
https://github.com/Strategy11/formidable-surveys/pull/339
Summary by CodeRabbit
New Features
Refactor
Style
✏️ Tip: You can customize this high-level summary in your review settings.