Rock: Enable pricing fields in Lite#3006
Merged
Crabcyborg merged 90 commits intomasterfrom Mar 30, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds product pricing: new field types (product, quantity, total), currency helpers and front-end JS for price/quantity/total calculations, admin UI/modal updates, DB migration to version 105, templates/styles, and removes the repository Changes
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant JS
participant Backend
participant DB
User->>Frontend: interact (select product / change qty)
Frontend->>JS: change event (data-frmprice, quantity)
JS->>JS: calcProductsTotal(formId) → format currency
JS->>Frontend: update total readout + hidden total field
User->>Frontend: submit form
Frontend->>Backend: POST form data (includes product details + total)
Backend->>DB: read form / currency settings (if needed)
Backend->>Backend: validate_total() recomputes expected sum from posted product data
alt totals match
Backend->>User: accept entry
else mismatch
Backend->>User: return validation error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
DeepSource Code ReviewWe reviewed changes in See full review on DeepSource ↗ Code Review Summary
|
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| PHP |  |
Mar 27, 2026 6:25p.m. | Review ↗ |
| JavaScript | |
Mar 27, 2026 6:25p.m. | Review ↗ |
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
js/src/admin/admin.js-9741-9754 (1)
9741-9754:⚠️ Potential issue | 🟡 MinorUse the
dialogcloseevent instead of individual click listeners.The current implementation only handles click dismissals, missing Escape key and dialog titlebar close button. When those non-click dismissals occur, the handler on
#frm-info-clickremains registered and can incorrectly affect subsequent unrelated info modals.Suggested fix
function handleModalDismiss( input ) { - const modalDismissers = document.querySelectorAll( '#frm_info_modal .dismiss, `#frm_info_modal` `#frm-info-click`, .ui-widget-overlay.ui-front' ); - function onModalClose() { + jQuery( '#frm_info_modal' ).one( 'dialogclose', function() { input.classList.add( 'frm_invalid_field' ); setTimeout( () => input.focus(), 0 ); - modalDismissers.forEach( el => { - el.removeEventListener( 'click', onModalClose ); - }); - } - - modalDismissers.forEach( el => { - el.addEventListener( 'click', onModalClose ); - }); + } ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/src/admin/admin.js` around lines 9741 - 9754, The handler in handleModalDismiss currently adds click listeners to multiple elements and misses non-click dismissals (Escape/titlebar), so replace that approach by attaching a single 'dialogclose' event listener to the modal element (`#frm_info_modal`) instead; in handleModalDismiss (and/or the onModalClose function) add the 'frm_invalid_field' class to the input, focus it, and remove any previously added click listeners and the 'dialogclose' listener itself to fully clean up; keep references to the modal element and any click-listener cleanup logic (the modalDismissers NodeList and their removeEventListener calls) so they are invoked from the dialogclose handler rather than relying on individual click events.classes/views/frm-fields/back-end/quantity-options.php-8-10 (1)
8-10:⚠️ Potential issue | 🟡 MinorUse the Lite text domain for this new label.
This template lives in Lite, but the string is registered under
formidable-pro, so it won't be translated here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/views/frm-fields/back-end/quantity-options.php` around lines 8 - 10, The label uses the pro text domain; update the esc_html_e call for the "Product Field" label to use the Lite text domain (replace 'formidable-pro' with the Lite domain used by the plugin) so the string is translated in the Lite template—look for the esc_html_e invocation for "Product Field" in this file and change its text domain accordingly.classes/views/frm-fields/back-end/product-single-option.php-11-12 (1)
11-12:⚠️ Potential issue | 🟡 MinorUse the Lite text domain for the new placeholders.
Product NameandPriceare added in a Lite template, but both are registered underformidable-pro, so they won't be translated in this package.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/views/frm-fields/back-end/product-single-option.php` around lines 11 - 12, The placeholders for the two input fields use esc_attr_e with the pro text domain ('formidable-pro') but these are in the Lite template; change the text domain to the Lite package text domain (use the Lite domain, e.g. 'formidable') for both placeholder calls so "Product Name" and "Price" are translatable in this package—update the esc_attr_e invocation on the input with id built from $html_id . '-' . $opt_key and on the input with class frm_product_price to use the Lite text domain.classes/views/frm-forms/modals/pricing-fields-modal.php-24-31 (1)
24-31:⚠️ Potential issue | 🟡 MinorButton labels and actions appear to be swapped.
The UX seems counterintuitive:
- "I'll do it later" is styled as the primary button but links to the upgrade page
- "Setup Payments Now" just closes the modal (
.frm-cancel-modal)Users clicking "Setup Payments Now" would expect to be directed to payment gateway settings, not have the modal close. Consider either:
- Swapping the button labels, or
- Making "Setup Payments Now" navigate to the settings page where payment gateways can be configured
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/views/frm-forms/modals/pricing-fields-modal.php` around lines 24 - 31, The primary and secondary actions are reversed: the anchor using FrmAppHelper::admin_upgrade_link currently shows "I'll do it later" and the cancel button (.frm-cancel-modal) shows "Setup Payments Now"; change the buttons so the primary action (class frm-button-primary) is "Setup Payments Now" and links to the payment/setup URL (use FrmAppHelper::admin_upgrade_link or the correct payment settings URL), and make the secondary/cancel button read "I'll do it later" and keep the .frm-cancel-modal behavior to close the modal; update the text in esc_html_e calls to match the swapped roles.classes/controllers/FrmAppController.php-867-881 (1)
867-881:⚠️ Potential issue | 🟡 MinorRace condition: Option deletion happens before modal is confirmed to display.
The
delete_option('frm_show_pricing_fields_modal')is called during asset enqueueing (line 880), but the modal isn't actually shown until later when the page renders and JavaScript executes. If the page load fails or the user navigates away before seeing the modal, the option is already deleted and the user will never see the modal.Consider moving the option deletion to JavaScript after the modal has been successfully displayed, or use an AJAX callback when the modal is closed/interacted with.
💡 Alternative approach using JavaScript
// Add modal to admin footer add_action( 'admin_footer', array( __CLASS__, 'include_pricing_fields_modal' ) ); - - // Delete the option so modal only shows once - delete_option( 'frm_show_pricing_fields_modal' ); }Then in JavaScript, make an AJAX call to delete the option when the modal is shown or dismissed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/controllers/FrmAppController.php` around lines 867 - 881, The option frm_show_pricing_fields_modal is being deleted during enqueueing (delete_option in FrmAppController block) which can race with the modal display; instead, keep the option intact during PHP enqueue and remove the delete_option call, then perform the deletion from client-side JS (in frm-pricing-fields-modal.js) after the modal is successfully shown or dismissed by calling an AJAX endpoint (create a small admin-ajax handler/method that calls delete_option('frm_show_pricing_fields_modal') and reference it from the modal script), or trigger deletion from the server via a dedicated include_pricing_fields_modal flow only after confirmation.classes/models/fields/FrmFieldProduct.php-155-157 (1)
155-157:⚠️ Potential issue | 🟡 MinorText domain inconsistency.
Same issue - using
'formidable-pro'text domain in Lite code.🐛 Proposed fix
protected function get_bulk_edit_string() { - return __( 'Bulk Edit Products', 'formidable-pro' ); + return __( 'Bulk Edit Products', 'formidable' ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/models/fields/FrmFieldProduct.php` around lines 155 - 157, The get_bulk_edit_string() method is using the pro text domain 'formidable-pro' in Lite code; update the translation call in get_bulk_edit_string to use the correct Lite text domain (replace 'formidable-pro' with the Lite domain, e.g., 'formidable') so the string __("Bulk Edit Products", ...) uses the proper text domain for this code path.classes/models/fields/FrmFieldProduct.php-28-37 (1)
28-37:⚠️ Potential issue | 🟡 MinorText domain inconsistency: Using 'formidable-pro' in Lite code.
This code is part of Formidable Forms Lite, but uses the
'formidable-pro'text domain for the "Product 1" string. This will cause translation issues as the string won't be found in the Lite translation files.🐛 Proposed fix
protected function new_field_settings() { return array( 'options' => serialize( array( '', - __( 'Product 1', 'formidable-pro' ), + __( 'Product 1', 'formidable' ), ) ), ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/models/fields/FrmFieldProduct.php` around lines 28 - 37, The string in new_field_settings (FrmFieldProduct::new_field_settings) uses the 'formidable-pro' text domain in the serialized 'options' array; update the text domain to the Lite domain (e.g., 'formidable') so translations are loaded correctly, preserving the surrounding serialize(...) structure and array keys exactly as-is.classes/models/fields/FrmFieldProduct.php-276-289 (1)
276-289:⚠️ Potential issue | 🟡 MinorText domain inconsistency.
Same issue - using
'formidable-pro'text domain for "New Product" string.🐛 Proposed fix
private static function hidden_field_option( $field ) { $opt_key = '000'; - $field_val = __( 'New Product', 'formidable-pro' ); + $field_val = __( 'New Product', 'formidable' );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/models/fields/FrmFieldProduct.php` around lines 276 - 289, The string __('New Product', 'formidable-pro') in hidden_field_option is using the wrong text domain; update the text domain to the project standard (replace 'formidable-pro' with the correct domain used elsewhere, e.g., 'formidable') so $field_val uses the consistent translation domain; modify the __('New Product', ...) call inside the hidden_field_option method accordingly (refer to $field_val and hidden_field_option to locate the change).js/formidable.js-2003-2007 (1)
2003-2007:⚠️ Potential issue | 🟡 Minor
getCurrencyreturnsundefinedwhen currency is not found.If
__FRMCURRis defined but doesn't contain theformId, this function returnsundefined. Callers likecalcProductsTotalthen access properties onundefined(e.g.,currency.decimal_separator), which will throw a TypeError.Consider adding a fallback or early return in
calcProductsTotal:🛡️ Proposed defensive fix in getCurrency
function getCurrency( formId ) { if ( typeof __FRMCURR !== 'undefined' && typeof __FRMCURR[ formId ] !== 'undefined' ) { return __FRMCURR[ formId ]; } + return null; }Then in
calcProductsTotal, add a check:currency = getCurrency( formId ); +if ( ! currency ) { + return; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/formidable.js` around lines 2003 - 2007, getCurrency can return undefined when __FRMCURR[formId] is missing which causes callers like calcProductsTotal to access properties on undefined; update getCurrency to return a safe default currency object when __FRMCURR or __FRMCURR[formId] is missing (e.g. include keys decimal_separator, thousands_separator, decimal_digit, symbol with sensible defaults) and also add a defensive check at the start of calcProductsTotal to bail out or use the default when getCurrency(formId) returns falsy so you never access properties on undefined; refer to the getCurrency and calcProductsTotal functions and the __FRMCURR lookup when making these changes.classes/models/fields/FrmFieldTotal.php-158-158 (1)
158-158:⚠️ Potential issue | 🟡 MinorPotential type error with
trim()on non-string value.
$price_quantity['parent_field_id']might benull, an integer, or another non-string type. Callingtrim()on a non-string in strict environments can cause issues. While PHP will coerce the value, it's safer to cast explicitly.🛡️ Proposed fix
-} elseif ( ! empty( trim( $price_quantity['parent_field_id'] ) ) ) { +} elseif ( ! empty( $price_quantity['parent_field_id'] ) ) {The
empty()check already handles null, empty string, and falsy values, makingtrim()unnecessary here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/models/fields/FrmFieldTotal.php` at line 158, The condition in FrmFieldTotal.php uses trim() on $price_quantity['parent_field_id'], which can be non-string and cause type issues; remove the trim() and change the check to simply use ! empty( $price_quantity['parent_field_id'] ) (or explicitly cast to string only if whitespace trimming is required) so the code uses the existing empty() behavior safely when evaluating $price_quantity['parent_field_id'].
🧹 Nitpick comments (14)
classes/views/shared/info-overlay.php (1)
19-24: Good restructuring of action buttons.The button container provides clean semantic grouping. One minor consideration: the empty anchor on line 23 could benefit from
aria-hidden="true"until JavaScript populates it with content, to ensure screen readers don't encounter an empty interactive element.🔧 Optional accessibility improvement
- <a href="#" class="info-modal-action-link button button-primary frm-button-primary frm_hidden"></a> + <a href="#" class="info-modal-action-link button button-primary frm-button-primary frm_hidden" aria-hidden="true"></a>Note: The JavaScript that shows this element should also set
aria-hidden="false"when making it visible and adding content.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/views/shared/info-overlay.php` around lines 19 - 24, The empty interactive anchor with class "info-modal-action-link" should be hidden from assistive tech until populated: add aria-hidden="true" to the <a class="info-modal-action-link"> element in info-overlay.php and update the JS that reveals/populates this element to set aria-hidden="false" (and remove the frm_hidden class) when content is inserted and it becomes visible; also ensure any script that clears or hides it again resets aria-hidden back to "true".stripe/controllers/FrmStrpLiteAppController.php (1)
60-69: Rename this helper or make it check real gateway configuration.
FrmTransLiteAppHelper::payments_table_exists()only tells us the payments schema is installed. It does not prove Stripe or any other gateway is configured, so this new public API/docblock can produce false positives if it gets reused for pricing/payment feature gating.♻️ If the intent is only “payments tables installed”, rename it to match
- * Checks if any payment gateway is configured. + * Checks if the payments tables are installed. @@ - public static function is_payment_gateway_configured() { + public static function payments_table_exists() { return FrmTransLiteAppHelper::payments_table_exists(); } @@ - if ( self::is_payment_gateway_configured() ) { + if ( self::payments_table_exists() ) { return; }Also applies to: 87-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stripe/controllers/FrmStrpLiteAppController.php` around lines 60 - 69, The method is_payment_gateway_configured currently returns FrmTransLiteAppHelper::payments_table_exists(), which only checks schema presence and can yield false positives; either rename the helper to reflect "payments_table_exists" and update the docblock/public API accordingly, or change is_payment_gateway_configured to perform a real gateway check (e.g., verify Stripe/other gateway settings, API keys, or enabled gateway flags) before returning true; update any other uses (including the similar call around the other occurrence) and adjust the docblock for FrmStrpLiteAppController::is_payment_gateway_configured to accurately describe the new semantics.classes/models/FrmField.php (1)
403-415: Add type hint to resolve PHPStan error.The static analysis tool flagged that the
$pro_fieldsparameter lacks a type specification. Since this is a pass-by-reference parameter modifying an array, add thearraytype hint.🔧 Proposed fix
/** * Remove field types that are moved from Pro to Lite. * * `@since` x.x * + * `@param` array $pro_fields The Pro fields array to modify. * `@return` void */ - public static function remove_moved_field_types_from_pro( &$pro_fields ) { + public static function remove_moved_field_types_from_pro( array &$pro_fields ) { unset( $pro_fields['credit_card'] ); unset( $pro_fields['product'] ); unset( $pro_fields['quantity'] ); unset( $pro_fields['total'] ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/models/FrmField.php` around lines 403 - 415, The static analyzer flags that the pass-by-reference parameter in FrmField::remove_moved_field_types_from_pro lacks a type, so update the method signature to declare the parameter as an array by adding the array type hint to &$pro_fields (keeping the reference) so PHPStan recognizes it as an array being modified.classes/views/frm-fields/front-end/product-single.php (2)
12-15: TODO: Currency formatting is missing for price display.The comment on line 13 indicates this is incomplete. The price is displayed as a raw number without currency symbol/formatting, which may confuse users expecting to see "$10.00" instead of "10".
Would you like me to help implement the currency formatting using
FrmCurrencyHelper::format_price()?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/views/frm-fields/front-end/product-single.php` around lines 12 - 15, The price is rendered as a raw number; update the product label rendering to format the price using the currency helper so users see a proper currency string. Replace the direct echo of $price in the frm_single_product_label block with a call to FrmCurrencyHelper::format_price($price) (fall back to esc_html on failure), and ensure the formatted value is escaped before output; keep the label text ($opt) escaped as-is.
7-20: Consider simplifying the single-option retrieval.The
foreachloop with an immediatebreakis inefficient for retrieving just the first option. Consider using array functions instead:♻️ Suggested simplification
-foreach ( $field['options'] as $opt_key => $opt ) { - $field_val = FrmFieldsHelper::get_value_from_array( $opt, $opt_key, $field ); - $price = FrmFieldProduct::get_price_from_array( $opt, $opt_key, $field ); - $opt = FrmFieldsHelper::get_label_from_array( $opt, $opt_key, $field ); +$opt_key = array_key_first( $field['options'] ); +$opt = $field['options'][ $opt_key ]; +$field_val = FrmFieldsHelper::get_value_from_array( $opt, $opt_key, $field ); +$price = FrmFieldProduct::get_price_from_array( $opt, $opt_key, $field ); +$opt = FrmFieldsHelper::get_label_from_array( $opt, $opt_key, $field ); +if ( $opt_key !== null ) : ?> <p class="frm_single_product_label"> - <?php // TODO: should show currency - echo esc_html( $opt ); ?>: <?php echo esc_html( $price ); ?> + <?php echo esc_html( $opt ); ?>: <?php echo esc_html( $price ); ?> </p> <input type="hidden" name="<?php echo esc_attr( $field_name ); ?>" id="<?php echo esc_attr( $html_id ); ?>" value="<?php echo esc_attr( $field_val ); ?>" data-frmprice="<?php echo esc_attr( $price ); ?>" <?php do_action( 'frm_field_input_html', $field ); ?> /> <?php - break; // We want just the first -} +endif;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/views/frm-fields/front-end/product-single.php` around lines 7 - 20, The foreach loop is only used to grab the first option then break; replace it by directly retrieving the first option (e.g., via reset/current) from $field['options'] and then call FrmFieldsHelper::get_value_from_array, FrmFieldProduct::get_price_from_array, and FrmFieldsHelper::get_label_from_array on that single option; keep the same output/escaping for the <p> and hidden input using $field_name and $html_id and preserve the data-frmprice attribute and the do_action call.classes/views/frm-fields/front-end/product-select.php (1)
17-20: Use strict comparison for empty string check.The loose comparison
$opt == ''on line 17 could incorrectly match other falsy values like0orfalse. Use strict comparison for reliability.🔧 Proposed fix
- if ( ! empty( $placeholder ) && $opt == '' && ! $skipped ) { + if ( ! empty( $placeholder ) && $opt === '' && ! $skipped ) { $skipped = true; continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/views/frm-fields/front-end/product-select.php` around lines 17 - 20, Change the loose comparison $opt == '' to a strict comparison $opt === '' so falsy values like 0 or false won't be treated as an empty string; update the conditional in the block that checks $placeholder, $opt and $skipped (the if using $placeholder, $opt and $skipped) to use $opt === '' and keep the rest of the logic ($skipped = true; continue;) unchanged.js/pricing-fields-modal.js (2)
70-77: Escape key handling may conflict with jQuery UI's built-in behavior.jQuery UI Dialog already handles the Escape key by default (unless
closeOnEscape: falseis set). This custom handler may cause the close logic to execute twice or conflict with the dialog's internal state.💡 Consider removing duplicate escape handler
Since
closeOnEscapedefaults totruein jQuery UI Dialog, you can remove this custom handler or explicitly disable jQuery UI's handler if you need custom behavior:$modal.dialog({ autoOpen: false, modal: true, width: 500, height: 'auto', resizable: false, draggable: false, + closeOnEscape: true, // This is already the default position: { my: 'center', at: 'center', of: window },Then remove lines 70-77 as they're redundant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/pricing-fields-modal.js` around lines 70 - 77, The global keydown handler that listens for Escape and manually calls $('#frm-pricing-fields-modal').dialog('close') is redundant with jQuery UI Dialog's built-in closeOnEscape behavior and may double-close or conflict; remove the entire keydown handler block that checks e.keyCode === 27 and calls dialog('isOpen')/dialog('close'), or alternatively disable the dialog's built-in handler by setting closeOnEscape: false on the $('#frm-pricing-fields-modal').dialog(...) initialization and then keep a single delegated handler — ensure you modify the dialog initialization (closeOnEscape) or remove the keydown block to avoid duplicate escape handling.
27-30: Selector may move multiple dialogs unintentionally.The selector
$('.ui-dialog')will match all jQuery UI dialogs on the page, not just the pricing modal's dialog. This could cause unintended side effects if other dialogs exist.🔧 Use specific dialog reference
create: function() { // Move dialog to the end of body to ensure proper stacking - $('.ui-dialog').appendTo('body'); + $(this).closest('.ui-dialog').appendTo('body'); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/pricing-fields-modal.js` around lines 27 - 30, The create callback currently moves all dialogs by using the global selector $('.ui-dialog'); narrow this to the pricing modal's dialog by selecting the specific dialog widget for the current instance (use the create callback's this context and obtain the dialog widget or the element's closest .ui-dialog) and append only that element to body instead of the global selector; update the create function in pricing-fields-modal.js to use the instance-specific dialog reference rather than $('.ui-dialog').classes/controllers/FrmFieldsController.php (1)
1054-1061: Add@sincetag to new method.The new
add_currency_field_attributes()method should include a@since x.xtag for documentation consistency.📝 Add documentation
/** * Add product attributes to fields in multi-paged forms. * + * `@since` x.x + * * `@param` array $field * `@param` array $add_html * * `@return` void */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/controllers/FrmFieldsController.php` around lines 1054 - 1061, The docblock for the new method add_currency_field_attributes() is missing a `@since` tag; update the method's PHPDoc block (the comment immediately above the add_currency_field_attributes function) to include a `@since` x.x entry matching your release version to keep documentation consistent, e.g., add a line like "@since x.x" within the existing docblock alongside the `@param/`@return tags.classes/models/fields/FrmFieldTotal.php (2)
178-180: Unused variable and commented-out code.Static analysis correctly identifies
$form_idis assigned but never used. Additionally, line 180 contains commented-out code that should be removed.♻️ Proposed fix
protected function prepare_display_value( $value, $atts ) { if ( ! empty( $atts['format'] ) && 'number' === $atts['format'] ) { return $value; } - $form_id = $this->get_field_column( 'form_id' ); $currency = FrmCurrencyHelper::get_currency(); - // $currency = FrmProCurrencyHelper::apply_shortcode_atts( $currency, $atts ); return FrmCurrencyHelper::format_price( $value, $currency ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/models/fields/FrmFieldTotal.php` around lines 178 - 180, Remove the dead assignment and the commented-out line: delete the unused $form_id = $this->get_field_column( 'form_id' ); and remove the commented-out $currency = FrmProCurrencyHelper::apply_shortcode_atts( $currency, $atts ); line in FrmFieldTotal (the code around get_field_column and FrmCurrencyHelper::get_currency), leaving only the active $currency assignment to avoid unused variable warnings and stale commented code.
56-69: Validation hook pattern is sound but consider cleanup.The pattern of deferring validation via
frm_entries_before_createhook ensures product/quantity data is gathered first. However, the hook is added but never removed. Ifvalidate()is called multiple times for different fields, multiple callbacks could accumulate.Consider whether the hook should be removed after execution in
validate_total():♻️ Possible cleanup
public function validate_total( $errors, $form ) { + // Remove this callback to prevent multiple executions + remove_filter( 'frm_entries_before_create', array( $this, 'validate_total' ), 10 ); + $value = $this->posted_value_args['value'];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/models/fields/FrmFieldTotal.php` around lines 56 - 69, The validate() method adds a frm_entries_before_create hook (via add_filter) to defer validation but never removes it; update the validate_total() method to call remove_filter('frm_entries_before_create', array($this, 'validate_total'), 10, 2) immediately after running so the callback is cleaned up and posted_value_args is cleared (or reset) to prevent accumulation when validate() is invoked multiple times for different fields; ensure the remove_filter uses the same priority and accepted args as the original add_filter.classes/models/fields/FrmFieldProduct.php (2)
137-137: Non-static method called withself::.
get_data_type_settings()is a private instance method (not static), but it's being called withself::get_data_type_settings(). While this works in PHP, it's misleading. Use$this->for instance methods.♻️ Proposed fix
-} elseif ( array_key_exists( $product_type, self::get_data_type_settings() ) ) { +} elseif ( array_key_exists( $product_type, $this->get_data_type_settings() ) ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/models/fields/FrmFieldProduct.php` at line 137, The conditional uses self::get_data_type_settings() but get_data_type_settings() is a private instance method of FrmFieldProduct; change the call to $this->get_data_type_settings() (and any other occurrences in the class) so instance method invocation is clear and correct, keeping the method non-static and ensuring the surrounding conditional (the elseif in the method where this appears) calls $this->get_data_type_settings().
72-80: Missing return type declarations (PHPStan).PHPStan correctly identifies that
get_data_type_settings()andget_posted_price()lack return type declarations. Adding these improves code clarity and enables better static analysis.♻️ Proposed fix
-private function get_data_type_settings() { +private function get_data_type_settings(): array { return array(-public function get_posted_price( $posted_value ) { +public function get_posted_price( $posted_value ): array|int { $price = 0;Also applies to: 203-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/models/fields/FrmFieldProduct.php` around lines 72 - 80, Add explicit return type declarations to the methods flagged by PHPStan: update get_data_type_settings() to declare it returns array (e.g., get_data_type_settings(): array) and update get_posted_price() to declare a precise numeric return type that matches its implementation (e.g., float or ?float if it may return null); ensure the declared types reflect the actual values returned by get_posted_price() and adjust any callers or internal conversions if needed.classes/models/fields/FrmFieldQuantity.php (1)
41-48: Potential issue with empty array check.Line 44 uses a truthiness check
$field['product_field'] ? (array) $field['product_field'] : array(). Ifproduct_fieldis already an empty array[], this will evaluate tofalseand unnecessarily recreate an empty array. While functionally correct, the intent might be clearer with an explicit check.Consider simplifying:
-$field['product_field'] = $field['product_field'] ? (array) $field['product_field'] : array(); +$field['product_field'] = (array) $field['product_field'];Casting to
(array)handles bothnulland scalar values correctly, making the ternary unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/models/fields/FrmFieldQuantity.php` around lines 41 - 48, In show_primary_options, simplify the product_field handling: instead of the ternary `$field['product_field'] ? (array) $field['product_field'] : array()`, directly cast with `(array) $field['product_field']` so null/scalar/array are handled consistently; update the assignment to `$field['product_field'] = (array) $field['product_field'];` and leave the include and parent::show_primary_options($args) call unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@classes/controllers/FrmFieldsController.php`:
- Around line 1084-1098: The get_product_price function currently assumes
FrmFieldFactory::get_field_object($field['id']) returns an object with
get_posted_price; update it to guard against that by checking the object's type
or method first (e.g., use instanceof FrmFieldProduct or
method_exists($field_obj, 'get_posted_price')) and only call get_posted_price
when available; if the check fails, return a safe default (e.g., 0 or null) or
handle gracefully so no fatal error occurs. Ensure you update get_product_price
to reference FrmFieldFactory::get_field_object, FrmFieldProduct, and
get_posted_price in the fix.
In `@classes/helpers/FrmCurrencyHelper.php`:
- Around line 89-95: The regex in FrmCurrencyHelper that extracts numeric chunks
using preg_match_all incorrectly excludes space and apostrophe thousand
separators, so update the pattern used where preg_match_all is called (the block
that sets $price and then calls self::maybe_use_decimal and str_replace) to
allow the configured thousand separator(s) (including space and apostrophe) and
the currency decimal separator; specifically include \s and ' (and also
dynamically include $currency['thousand_separator'] and
$currency['decimal_separator'] when building the pattern), ensure proper
escaping of those separators in the regex, and keep the existing fallback logic
($matches ? end($matches[0]) : 0) and subsequent maybe_use_decimal/str_replace
flow.
- Around line 58-129: Update the method signatures to include PHP type hints and
return types to satisfy PHPStan: change format_price to accept float|string
$amount and array $currency and return string; change prepare_price to accept
string $price and array $currency and return string; change maybe_use_decimal to
accept string $amount and array $currency and return string; change
add_currency_to_global to accept int|string $form_id and return void. Also
update the corresponding docblocks to match these types (or remove redundant
`@param/`@return types) for functions format_price, prepare_price,
maybe_use_decimal, and add_currency_to_global.
In `@classes/helpers/FrmFieldsHelper.php`:
- Around line 2373-2381: Normalize composite field keys by replacing pipe
characters before building the class/id strings: create a normalized key (e.g.,
$norm_key = str_replace('|', '-', $field_type['key'])) and use $norm_key when
composing $classes ('frmbutton frm6 frm_t' . $norm_key) and the li id attribute;
this matches the normalization used in show_add_field_buttons() so composite
fields get the correct styling/JS hooks.
In `@classes/helpers/FrmFormsHelper.php`:
- Around line 2153-2163: In load_currency_js() replace the use of PHP's
json_encode when serializing $frm_vars['currency'] for inline output with
WordPress's wp_json_encode to safely escape sequences like "</script>" and other
problematic data; locate the json_encode call that builds 'var frmcurrency=' and
swap it to wp_json_encode($frm_vars['currency']) so the generated frmcurrency,
__FRMCURR assignment/merge code, and existing output logic remain the same but
safe for translatable strings.
In `@classes/models/fields/FrmFieldTotal.php`:
- Around line 83-87: The code calls FrmProFieldsHelper::is_field_hidden()
directly which will fatally fail when the Pro class is absent; update the
conditional in FrmFieldTotal (where $this->field and $errors and
get_field_column('id') are used) to first verify the Pro helper exists and the
method is callable (e.g., class_exists('FrmProFieldsHelper') &&
is_callable(['FrmProFieldsHelper','is_field_hidden'])) before invoking
is_field_hidden, and if the check fails treat the field as not hidden (skip the
hidden-logic branch) so Lite installations won't error.
In `@classes/views/frm-fields/back-end/product-single-option.php`:
- Around line 15-20: The add-option trigger ID is not unique because
id="frm_add_opt_<?php echo esc_attr( $field['id'] ); ?>" is rendered for every
option row; change the id to include the option key (use $opt_key or similar) so
each row gets a unique id (e.g. frm_add_opt_{field_id}-{opt_key}) and update the
related data-showlast selector to reference that unique id
(data-showlast="#frm_add_opt_{field_id}-{opt_key}"), ensuring the elements
referenced by frm_add_opt, frm_remove_tag, and data-showlast remain consistent
with the new id naming.
In `@classes/views/frm-fields/back-end/separate-values.php`:
- Around line 14-18: The current upsell block uses a non-interactive <p> with a
disabled checkbox (class names like frm6 frm_form_field frm6_followed and
data-content="separate-values") which is not focusable; replace the <p> with a
real interactive element (e.g., a <button> or <a> styled as the same block) that
preserves the existing data- attributes (data-upgrade, data-message,
data-medium, data-content) and visual markup around the label and span, make the
element keyboard-accessible (native button or add role="button" and
tabindex="0"), wire its click and keypress handlers to open the upgrade/upsell
flow, and keep the presentational checkbox input but mark it aria-hidden="true"
or remove its focusability so screen readers and keyboard users interact with
the actionable control instead; ensure aria-label/aria-describedby uses the
existing esc_html_e text for accessibility.
In `@css/frm_testing_mode.css`:
- Line 3: The global Bootstrap tooltip rules starting at the selector
".tooltip{...}" are currently unscoped and affect all tooltips; scope them to
Formidable by prefixing those selectors with the Formidable tooltip class (e.g.,
change ".tooltip", ".tooltip.show", ".tooltip .arrow", ".tooltip
.arrow::before", ".bs-tooltip-top", ".bs-tooltip-auto[x-placement^=top]",
".bs-tooltip-right", ".bs-tooltip-auto[x-placement^=right]",
".bs-tooltip-bottom", ".bs-tooltip-auto[x-placement^=bottom]",
".bs-tooltip-left", ".bs-tooltip-auto[x-placement^=left]", and ".tooltip-inner"
to target only the Formidable tooltip variant such as ".tooltip.frm_tooltip",
".tooltip.frm_tooltip.show", ".tooltip.frm_tooltip .arrow",
".tooltip.frm_tooltip .arrow::before", ".bs-tooltip-top.frm_tooltip",
".bs-tooltip-auto.frm_tooltip[x-placement^=top]", etc.), ensuring all placement
selectors get the same .frm_tooltip prefix so unrelated admin tooltips are
unaffected.
In `@js/formidable.js`:
- Line 1872: Remove the stray debug console.log call that prints formTotals;
locate the console.log(formTotals) statement (referenced in the diff) and delete
it so production code no longer outputs form totals to the browser console. If
you need to preserve the value for future debugging, replace it with a
debug-only logger guarded by a feature flag or development check (e.g., only log
when NODE_ENV !== 'production') referencing the same formTotals variable.
- Line 1910: Remove the leftover debug statement console.log( total ) from
js/formidable.js; locate the occurrence of the console.log( total ) call (the
debug print referencing the variable total) and delete it so the production
browser console is not polluted, ensuring no other logic or side-effects are
affected by its removal.
In `@js/frm-pricing-fields-modal.js`:
- Around line 31-44: The callbacks are mutating every dialog and overlay via
selectors '.ui-dialog' and '.ui-widget-overlay'; instead scope changes to this
modal by using the dialog widget and the overlay instance for this open call: in
create/open use jQuery(this).dialog('widget') (or store it in a variable like
$widget) to move/style only that dialog, and target only the overlay for this
modal with jQuery('.ui-widget-overlay').last() (or the overlay element
associated with this dialog) when changing z-index/opacity so unrelated dialogs
are not affected.
- Around line 95-101: The document click handler should first check that the
dialog is actually open via $modal.dialog('isOpen') and then use
$modal.dialog('widget') to test whether the click was inside the dialog widget;
also explicitly detect clicks on the jQuery UI overlay ('.ui-widget-overlay') so
overlay clicks close the modal, and ignore the original triggering click that
opened the dialog by ensuring the event target is not the element that opened it
(or by checking that the event occurred after the dialog was opened). Update the
listener around the '#frm-pricing-fields-modal' reference and use
dialog('isOpen'), dialog('widget'), and '.ui-widget-overlay' checks before
calling hidePricingFieldsModal(), so clicks inside the widget do nothing, clicks
on the overlay close, and the opening click does not immediately close the
modal.
In `@js/src/admin/admin.js`:
- Around line 2245-2255: The pricing-field gate that shows infoModal for
['product','quantity','total'] is only in addFieldClick(); replicate the same
guard and modal logic inside insertNewFieldByDragging() (or call a shared helper
like the existing field-guard function) so drag/drop cannot add those fields
without a gateway, and replace the hard-coded actionUrl 'http://truongwp.com'
with the correct dynamic admin/payment-settings URL used elsewhere in the app
(e.g., the existing payment settings route or function that builds admin URLs)
so the modal's action links to the current site's payment settings.
- Around line 410-435: The modal leaves CTA-specific state set after a previous
infoModal call; before applying msg.actionUrl you must reset the modal to its
default non-CTA state: ensure the close button (element id 'frm-info-click') has
primary classes ('button-primary' and 'frm-button-primary') and does not have
secondary classes ('button-secondary' or 'frm-button-secondary'), remove the
'frmright' class from the buttons wrapper (querySelector '.info-modal-buttons'),
hide the action button (querySelector '.info-modal-action-link') by adding
'frm_hidden', clear or reset its href/textContent, and only then apply the CTA
changes when msg.actionUrl is present so subsequent plain infoModal(...) calls
don’t show stale CTA UI.
- Around line 390-400: The code is incorrectly checking the type of msg using
the literal 'string' === msg so string arguments never normalize; update the
type check to use typeof msg === 'string' (or equivalent) before converting msg
to an object in the modal normalization block (the branch that currently does
"if ( 'string' === msg ) { msg = { msg }; }") so that calls like
infoModal('...') are converted to { msg: '...' } and then merged with
Object.assign defaults (title, msg, closeText, actionUrl, actionText).
---
Minor comments:
In `@classes/controllers/FrmAppController.php`:
- Around line 867-881: The option frm_show_pricing_fields_modal is being deleted
during enqueueing (delete_option in FrmAppController block) which can race with
the modal display; instead, keep the option intact during PHP enqueue and remove
the delete_option call, then perform the deletion from client-side JS (in
frm-pricing-fields-modal.js) after the modal is successfully shown or dismissed
by calling an AJAX endpoint (create a small admin-ajax handler/method that calls
delete_option('frm_show_pricing_fields_modal') and reference it from the modal
script), or trigger deletion from the server via a dedicated
include_pricing_fields_modal flow only after confirmation.
In `@classes/models/fields/FrmFieldProduct.php`:
- Around line 155-157: The get_bulk_edit_string() method is using the pro text
domain 'formidable-pro' in Lite code; update the translation call in
get_bulk_edit_string to use the correct Lite text domain (replace
'formidable-pro' with the Lite domain, e.g., 'formidable') so the string
__("Bulk Edit Products", ...) uses the proper text domain for this code path.
- Around line 28-37: The string in new_field_settings
(FrmFieldProduct::new_field_settings) uses the 'formidable-pro' text domain in
the serialized 'options' array; update the text domain to the Lite domain (e.g.,
'formidable') so translations are loaded correctly, preserving the surrounding
serialize(...) structure and array keys exactly as-is.
- Around line 276-289: The string __('New Product', 'formidable-pro') in
hidden_field_option is using the wrong text domain; update the text domain to
the project standard (replace 'formidable-pro' with the correct domain used
elsewhere, e.g., 'formidable') so $field_val uses the consistent translation
domain; modify the __('New Product', ...) call inside the hidden_field_option
method accordingly (refer to $field_val and hidden_field_option to locate the
change).
In `@classes/models/fields/FrmFieldTotal.php`:
- Line 158: The condition in FrmFieldTotal.php uses trim() on
$price_quantity['parent_field_id'], which can be non-string and cause type
issues; remove the trim() and change the check to simply use ! empty(
$price_quantity['parent_field_id'] ) (or explicitly cast to string only if
whitespace trimming is required) so the code uses the existing empty() behavior
safely when evaluating $price_quantity['parent_field_id'].
In `@classes/views/frm-fields/back-end/product-single-option.php`:
- Around line 11-12: The placeholders for the two input fields use esc_attr_e
with the pro text domain ('formidable-pro') but these are in the Lite template;
change the text domain to the Lite package text domain (use the Lite domain,
e.g. 'formidable') for both placeholder calls so "Product Name" and "Price" are
translatable in this package—update the esc_attr_e invocation on the input with
id built from $html_id . '-' . $opt_key and on the input with class
frm_product_price to use the Lite text domain.
In `@classes/views/frm-fields/back-end/quantity-options.php`:
- Around line 8-10: The label uses the pro text domain; update the esc_html_e
call for the "Product Field" label to use the Lite text domain (replace
'formidable-pro' with the Lite domain used by the plugin) so the string is
translated in the Lite template—look for the esc_html_e invocation for "Product
Field" in this file and change its text domain accordingly.
In `@classes/views/frm-forms/modals/pricing-fields-modal.php`:
- Around line 24-31: The primary and secondary actions are reversed: the anchor
using FrmAppHelper::admin_upgrade_link currently shows "I'll do it later" and
the cancel button (.frm-cancel-modal) shows "Setup Payments Now"; change the
buttons so the primary action (class frm-button-primary) is "Setup Payments Now"
and links to the payment/setup URL (use FrmAppHelper::admin_upgrade_link or the
correct payment settings URL), and make the secondary/cancel button read "I'll
do it later" and keep the .frm-cancel-modal behavior to close the modal; update
the text in esc_html_e calls to match the swapped roles.
In `@js/formidable.js`:
- Around line 2003-2007: getCurrency can return undefined when __FRMCURR[formId]
is missing which causes callers like calcProductsTotal to access properties on
undefined; update getCurrency to return a safe default currency object when
__FRMCURR or __FRMCURR[formId] is missing (e.g. include keys decimal_separator,
thousands_separator, decimal_digit, symbol with sensible defaults) and also add
a defensive check at the start of calcProductsTotal to bail out or use the
default when getCurrency(formId) returns falsy so you never access properties on
undefined; refer to the getCurrency and calcProductsTotal functions and the
__FRMCURR lookup when making these changes.
In `@js/src/admin/admin.js`:
- Around line 9741-9754: The handler in handleModalDismiss currently adds click
listeners to multiple elements and misses non-click dismissals
(Escape/titlebar), so replace that approach by attaching a single 'dialogclose'
event listener to the modal element (`#frm_info_modal`) instead; in
handleModalDismiss (and/or the onModalClose function) add the
'frm_invalid_field' class to the input, focus it, and remove any previously
added click listeners and the 'dialogclose' listener itself to fully clean up;
keep references to the modal element and any click-listener cleanup logic (the
modalDismissers NodeList and their removeEventListener calls) so they are
invoked from the dialogclose handler rather than relying on individual click
events.
---
Nitpick comments:
In `@classes/controllers/FrmFieldsController.php`:
- Around line 1054-1061: The docblock for the new method
add_currency_field_attributes() is missing a `@since` tag; update the method's
PHPDoc block (the comment immediately above the add_currency_field_attributes
function) to include a `@since` x.x entry matching your release version to keep
documentation consistent, e.g., add a line like "@since x.x" within the existing
docblock alongside the `@param/`@return tags.
In `@classes/models/fields/FrmFieldProduct.php`:
- Line 137: The conditional uses self::get_data_type_settings() but
get_data_type_settings() is a private instance method of FrmFieldProduct; change
the call to $this->get_data_type_settings() (and any other occurrences in the
class) so instance method invocation is clear and correct, keeping the method
non-static and ensuring the surrounding conditional (the elseif in the method
where this appears) calls $this->get_data_type_settings().
- Around line 72-80: Add explicit return type declarations to the methods
flagged by PHPStan: update get_data_type_settings() to declare it returns array
(e.g., get_data_type_settings(): array) and update get_posted_price() to declare
a precise numeric return type that matches its implementation (e.g., float or
?float if it may return null); ensure the declared types reflect the actual
values returned by get_posted_price() and adjust any callers or internal
conversions if needed.
In `@classes/models/fields/FrmFieldQuantity.php`:
- Around line 41-48: In show_primary_options, simplify the product_field
handling: instead of the ternary `$field['product_field'] ? (array)
$field['product_field'] : array()`, directly cast with `(array)
$field['product_field']` so null/scalar/array are handled consistently; update
the assignment to `$field['product_field'] = (array) $field['product_field'];`
and leave the include and parent::show_primary_options($args) call unchanged.
In `@classes/models/fields/FrmFieldTotal.php`:
- Around line 178-180: Remove the dead assignment and the commented-out line:
delete the unused $form_id = $this->get_field_column( 'form_id' ); and remove
the commented-out $currency = FrmProCurrencyHelper::apply_shortcode_atts(
$currency, $atts ); line in FrmFieldTotal (the code around get_field_column and
FrmCurrencyHelper::get_currency), leaving only the active $currency assignment
to avoid unused variable warnings and stale commented code.
- Around line 56-69: The validate() method adds a frm_entries_before_create hook
(via add_filter) to defer validation but never removes it; update the
validate_total() method to call remove_filter('frm_entries_before_create',
array($this, 'validate_total'), 10, 2) immediately after running so the callback
is cleaned up and posted_value_args is cleared (or reset) to prevent
accumulation when validate() is invoked multiple times for different fields;
ensure the remove_filter uses the same priority and accepted args as the
original add_filter.
In `@classes/models/FrmField.php`:
- Around line 403-415: The static analyzer flags that the pass-by-reference
parameter in FrmField::remove_moved_field_types_from_pro lacks a type, so update
the method signature to declare the parameter as an array by adding the array
type hint to &$pro_fields (keeping the reference) so PHPStan recognizes it as an
array being modified.
In `@classes/views/frm-fields/front-end/product-select.php`:
- Around line 17-20: Change the loose comparison $opt == '' to a strict
comparison $opt === '' so falsy values like 0 or false won't be treated as an
empty string; update the conditional in the block that checks $placeholder, $opt
and $skipped (the if using $placeholder, $opt and $skipped) to use $opt === ''
and keep the rest of the logic ($skipped = true; continue;) unchanged.
In `@classes/views/frm-fields/front-end/product-single.php`:
- Around line 12-15: The price is rendered as a raw number; update the product
label rendering to format the price using the currency helper so users see a
proper currency string. Replace the direct echo of $price in the
frm_single_product_label block with a call to
FrmCurrencyHelper::format_price($price) (fall back to esc_html on failure), and
ensure the formatted value is escaped before output; keep the label text ($opt)
escaped as-is.
- Around line 7-20: The foreach loop is only used to grab the first option then
break; replace it by directly retrieving the first option (e.g., via
reset/current) from $field['options'] and then call
FrmFieldsHelper::get_value_from_array, FrmFieldProduct::get_price_from_array,
and FrmFieldsHelper::get_label_from_array on that single option; keep the same
output/escaping for the <p> and hidden input using $field_name and $html_id and
preserve the data-frmprice attribute and the do_action call.
In `@classes/views/shared/info-overlay.php`:
- Around line 19-24: The empty interactive anchor with class
"info-modal-action-link" should be hidden from assistive tech until populated:
add aria-hidden="true" to the <a class="info-modal-action-link"> element in
info-overlay.php and update the JS that reveals/populates this element to set
aria-hidden="false" (and remove the frm_hidden class) when content is inserted
and it becomes visible; also ensure any script that clears or hides it again
resets aria-hidden back to "true".
In `@js/pricing-fields-modal.js`:
- Around line 70-77: The global keydown handler that listens for Escape and
manually calls $('#frm-pricing-fields-modal').dialog('close') is redundant with
jQuery UI Dialog's built-in closeOnEscape behavior and may double-close or
conflict; remove the entire keydown handler block that checks e.keyCode === 27
and calls dialog('isOpen')/dialog('close'), or alternatively disable the
dialog's built-in handler by setting closeOnEscape: false on the
$('#frm-pricing-fields-modal').dialog(...) initialization and then keep a single
delegated handler — ensure you modify the dialog initialization (closeOnEscape)
or remove the keydown block to avoid duplicate escape handling.
- Around line 27-30: The create callback currently moves all dialogs by using
the global selector $('.ui-dialog'); narrow this to the pricing modal's dialog
by selecting the specific dialog widget for the current instance (use the create
callback's this context and obtain the dialog widget or the element's closest
.ui-dialog) and append only that element to body instead of the global selector;
update the create function in pricing-fields-modal.js to use the
instance-specific dialog reference rather than $('.ui-dialog').
In `@stripe/controllers/FrmStrpLiteAppController.php`:
- Around line 60-69: The method is_payment_gateway_configured currently returns
FrmTransLiteAppHelper::payments_table_exists(), which only checks schema
presence and can yield false positives; either rename the helper to reflect
"payments_table_exists" and update the docblock/public API accordingly, or
change is_payment_gateway_configured to perform a real gateway check (e.g.,
verify Stripe/other gateway settings, API keys, or enabled gateway flags) before
returning true; update any other uses (including the similar call around the
other occurrence) and adjust the docblock for
FrmStrpLiteAppController::is_payment_gateway_configured to accurately describe
the new semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 115cb8a5-637b-4e7f-9970-650a1157a418
⛔ Files ignored due to path filters (2)
images/upsell/pricing-fields.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
.htaccessclasses/controllers/FrmAppController.phpclasses/controllers/FrmFieldsController.phpclasses/controllers/FrmFormsController.phpclasses/factories/FrmFieldFactory.phpclasses/helpers/FrmAppHelper.phpclasses/helpers/FrmCurrencyHelper.phpclasses/helpers/FrmFieldsHelper.phpclasses/helpers/FrmFormsHelper.phpclasses/models/FrmField.phpclasses/models/FrmMigrate.phpclasses/models/fields/FrmFieldProduct.phpclasses/models/fields/FrmFieldQuantity.phpclasses/models/fields/FrmFieldTotal.phpclasses/views/frm-fields/back-end/field-total.phpclasses/views/frm-fields/back-end/product-options.phpclasses/views/frm-fields/back-end/product-single-option.phpclasses/views/frm-fields/back-end/quantity-options.phpclasses/views/frm-fields/back-end/separate-values.phpclasses/views/frm-fields/front-end/product-radio.phpclasses/views/frm-fields/front-end/product-select.phpclasses/views/frm-fields/front-end/product-single.phpclasses/views/frm-forms/add_field_links.phpclasses/views/frm-forms/modals/pricing-fields-modal.phpclasses/views/shared/info-overlay.phpcss/frm-pricing-fields-modal.csscss/frm_admin.csscss/frm_testing_mode.cssjs/formidable.jsjs/formidable_admin.jsjs/frm-pricing-fields-modal.jsjs/pricing-fields-modal.jsjs/src/admin/admin.jsresources/scss/admin/components/accordion/_accordion.scssresources/scss/admin/utilities/typography/_text-align-part2.scssstripe/controllers/FrmStrpLiteAppController.php
💤 Files with no reviewable changes (1)
- .htaccess
Comment on lines
+89
to
+95
| preg_match_all( '/[\-]*[0-9,.]*\.?\,?[0-9]+/', $price, $matches ); | ||
| $price = $matches ? end( $matches[0] ) : 0; | ||
|
|
||
| if ( $price ) { | ||
| $price = self::maybe_use_decimal( $price, $currency ); | ||
| $price = str_replace( $currency['decimal_separator'], '.', str_replace( $currency['thousand_separator'], '', $price ) ); | ||
| } |
Contributor
There was a problem hiding this comment.
Fix price parsing for currencies that use spaces or apostrophes as thousand separators.
Line 89 only matches digits, commas, and dots, so inputs like 1 234,56 or 1'234.56 get truncated to the last chunk before Line 94 ever strips the configured separator. That breaks several currencies this helper explicitly supports.
💡 One way to keep the configured separators intact
- preg_match_all( '/[\-]*[0-9,.]*\.?\,?[0-9]+/', $price, $matches );
+ $allowed_separators = preg_quote( $currency['thousand_separator'] . $currency['decimal_separator'], '/' );
+ preg_match_all( '/-?[0-9' . $allowed_separators . ']+/u', $price, $matches );
$price = $matches ? end( $matches[0] ) : 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@classes/helpers/FrmCurrencyHelper.php` around lines 89 - 95, The regex in
FrmCurrencyHelper that extracts numeric chunks using preg_match_all incorrectly
excludes space and apostrophe thousand separators, so update the pattern used
where preg_match_all is called (the block that sets $price and then calls
self::maybe_use_decimal and str_replace) to allow the configured thousand
separator(s) (including space and apostrophe) and the currency decimal
separator; specifically include \s and ' (and also dynamically include
$currency['thousand_separator'] and $currency['decimal_separator'] when building
the pattern), ensure proper escaping of those separators in the regex, and keep
the existing fallback logic ($matches ? end($matches[0]) : 0) and subsequent
maybe_use_decimal/str_replace flow.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
js/src/admin/admin.js (3)
2246-2255:⚠️ Potential issue | 🟠 MajorGate pricing fields behind a shared “payments configured?” check.
This branch is unconditional, so click-add is blocked for
product,quantity, andtotalon every site.insertNewFieldByDragging()can still bypass it, and the CTA still uses a hard-coded off-site URL. This should be a shared guard/helper so both add paths behave the same and the action points to the local payment settings screen.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/src/admin/admin.js` around lines 2246 - 2255, The click-add branch unconditionally blocks adding product/quantity/total and uses a hard-coded off-site URL; extract the payment-check into a shared helper (e.g., isPaymentGatewayConfigured) and use it wherever fields are gated (the current click-add handler and insertNewFieldByDragging), so both add paths behave identically; replace the inline infoModal actionUrl with the local payment settings route (use your app’s payment settings URL constant or route) and update the infoModal call to run only when isPaymentGatewayConfigured() is false, returning false to block and otherwise allowing the add operation to proceed.
387-405:⚠️ Potential issue | 🟠 MajorFix string normalization here, and don't pipe raw modal strings into
.html().This is still unresolved from the earlier review: plain
infoModal( '...' )callers never normalize, so they fall back to the default text. Once that check is corrected, callers likecheckCalculationCreatedByUser()will flow straight into.html( msg.msg ), which currently accepts raw user-entered markup.Suggested fix
- if ( 'string' === msg ) { + if ( 'string' === typeof msg ) { msg = { msg }; } @@ if ( msg.msg ) { - $info.find( '.frm-info-msg' ).html( msg.msg ); + $info.find( '.frm-info-msg' ).html( purifyHtml( msg.msg ) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/src/admin/admin.js` around lines 387 - 405, Fix the string normalization and avoid injecting raw HTML: change the faulty type check to use typeof (i.e. if (typeof msg === 'string') msg = { msg };) so string callers are properly converted, and replace the unsafe HTML insertion ($info.find('.frm-info-msg').html( msg.msg )) with a safe text insertion (use .text(...) or set textContent) so user-entered markup is not rendered.
10673-10673:⚠️ Potential issue | 🔴 CriticalPass the input element into
validateProductPriceValue().Every other
frmDom.util.documentOn()usage in this file treats the callback argument as an event object, butvalidateProductPriceValue()immediately reads.value. As wired, the first.frm_product_pricechange will throw before any validation runs.Suggested fix
- frmDom.util.documentOn( 'change', '.frm_product_price', validateProductPriceValue ); + frmDom.util.documentOn( 'change', '.frm_product_price', event => validateProductPriceValue( event.target ) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/src/admin/admin.js` at line 10673, The current event hookup calls validateProductPriceValue directly, but that function expects the input element (reads .value) not the event object; update the frmDom.util.documentOn call for '.frm_product_price' so the handler passes the element (e.g., event.target or event.currentTarget / this) into validateProductPriceValue instead of the event object, ensuring the function receives the input element that has the .value property.classes/controllers/FrmFieldsController.php (1)
1095-1096:⚠️ Potential issue | 🟠 MajorGuard
get_posted_price()on the factory result.This is still the reason Psalm/PHPStan are failing:
FrmFieldFactory::get_field_object()is not guaranteed to return a type that definesget_posted_price(). If a non-product object reaches this path, it will fatal. Narrow the object first (instanceof FrmFieldProductormethod_exists) and return a safe fallback when it is not a product field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/controllers/FrmFieldsController.php` around lines 1095 - 1096, Guard the call to get_posted_price by ensuring the factory result is a product field: retrieve the object via FrmFieldFactory::get_field_object( $field['id'] ), check that it is an instance of FrmFieldProduct (or at minimum that method_exists($field_obj, 'get_posted_price')), and only then call $field_obj->get_posted_price( $value ); otherwise return a safe fallback (for example the unmodified $value or null) to avoid a fatal when a non-product field is encountered in FrmFieldsController.
🧹 Nitpick comments (1)
js/formidable.js (1)
1787-1788: Consider using an object instead of an array forformTotals.
formTotalsis declared as an array but accessed with string keys (formTotals[formId]). While this works in JavaScript, using an object ({}) is semantically clearer and avoids confusion.♻️ Suggested change
- var formTotals = [], + var formTotals = {}, totalFields;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/formidable.js` around lines 1787 - 1788, formTotals is declared as an array but used with string keys (e.g., formTotals[formId]); change its declaration to an object to reflect intended use and avoid confusion: replace the array declaration of formTotals with an object literal and ensure any later references (places using formTotals and the formId key) continue to work with the object-based map (look for the symbol formTotals and usages where formId is used as the key).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@classes/models/FrmField.php`:
- Line 410: The method remove_moved_field_types_from_pro currently accepts an
untyped reference parameter &$pro_fields but only operates on array keys; add an
explicit array type hint to the signature (change &$pro_fields to array
&$pro_fields) so the contract is clear and static analysis passes, keeping the
parameter by reference and leaving the function body unchanged.
In `@classes/views/frm-forms/add_field_links.php`:
- Around line 133-135: The code calls FrmAppHelper::show_pill_text()
unconditionally in the section header, causing the Pricing section (and any
section added via frm_available_field_sections) to always display the pill;
update the rendering so the pill is shown only when the current section has a
specific flag (e.g., a property on the section array like 'show_pill' or a slug
check for 'pricing') by checking that flag before calling
FrmAppHelper::show_pill_text(); locate the section rendering block that outputs
the span and wrap the call to FrmAppHelper::show_pill_text() in a conditional
that reads the section's flag (or verifies the section key/slug) so only flagged
sections render the pill.
In `@js/formidable.js`:
- Around line 1982-1996: The loop over quantityFields calls JSON.parse on
this.getAttribute('data-frmproduct') which can throw on invalid JSON; wrap the
JSON.parse call in a try-catch inside the quantityFields.each callback (around
the parsing of data-frmproduct), and on catch treat it as empty/invalid (skip
this element, e.g., continue by returning true) so that parsing errors don’t
break the calculation; keep the rest of the logic using ids, fieldID and
assigning quantity as before.
In `@js/src/admin/admin.js`:
- Around line 9718-9719: Change the i18n text domain used for the validation
message so it matches the rest of the admin bundle: update the call that sets
validationFailMessage (the __('Please enter a valid number.', 'formidable-pro')
expression) to use 'formidable' instead of 'formidable-pro' so the message will
be translated in the core admin UI; keep the same variable name
(validationFailMessage) and invocation of frmAdminBuild.infoModal.
---
Duplicate comments:
In `@classes/controllers/FrmFieldsController.php`:
- Around line 1095-1096: Guard the call to get_posted_price by ensuring the
factory result is a product field: retrieve the object via
FrmFieldFactory::get_field_object( $field['id'] ), check that it is an instance
of FrmFieldProduct (or at minimum that method_exists($field_obj,
'get_posted_price')), and only then call $field_obj->get_posted_price( $value );
otherwise return a safe fallback (for example the unmodified $value or null) to
avoid a fatal when a non-product field is encountered in FrmFieldsController.
In `@js/src/admin/admin.js`:
- Around line 2246-2255: The click-add branch unconditionally blocks adding
product/quantity/total and uses a hard-coded off-site URL; extract the
payment-check into a shared helper (e.g., isPaymentGatewayConfigured) and use it
wherever fields are gated (the current click-add handler and
insertNewFieldByDragging), so both add paths behave identically; replace the
inline infoModal actionUrl with the local payment settings route (use your app’s
payment settings URL constant or route) and update the infoModal call to run
only when isPaymentGatewayConfigured() is false, returning false to block and
otherwise allowing the add operation to proceed.
- Around line 387-405: Fix the string normalization and avoid injecting raw
HTML: change the faulty type check to use typeof (i.e. if (typeof msg ===
'string') msg = { msg };) so string callers are properly converted, and replace
the unsafe HTML insertion ($info.find('.frm-info-msg').html( msg.msg )) with a
safe text insertion (use .text(...) or set textContent) so user-entered markup
is not rendered.
- Line 10673: The current event hookup calls validateProductPriceValue directly,
but that function expects the input element (reads .value) not the event object;
update the frmDom.util.documentOn call for '.frm_product_price' so the handler
passes the element (e.g., event.target or event.currentTarget / this) into
validateProductPriceValue instead of the event object, ensuring the function
receives the input element that has the .value property.
---
Nitpick comments:
In `@js/formidable.js`:
- Around line 1787-1788: formTotals is declared as an array but used with string
keys (e.g., formTotals[formId]); change its declaration to an object to reflect
intended use and avoid confusion: replace the array declaration of formTotals
with an object literal and ensure any later references (places using formTotals
and the formId key) continue to work with the object-based map (look for the
symbol formTotals and usages where formId is used as the key).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 39340069-1b5b-4cf1-a5cb-630b6ee70749
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
classes/controllers/FrmAppController.phpclasses/controllers/FrmFieldsController.phpclasses/controllers/FrmFormsController.phpclasses/helpers/FrmAppHelper.phpclasses/helpers/FrmFieldsHelper.phpclasses/helpers/FrmFormsHelper.phpclasses/models/FrmField.phpclasses/models/FrmMigrate.phpclasses/views/frm-forms/add_field_links.phpcss/frm_admin.cssjs/formidable-web-components.jsjs/formidable.jsjs/formidable_admin.jsjs/src/admin/admin.js
🚧 Files skipped from review as they are similar to previous changes (3)
- classes/helpers/FrmFormsHelper.php
- classes/helpers/FrmAppHelper.php
- classes/controllers/FrmFormsController.php
truongwp
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
css/frm_testing_mode.css (1)
3-3:⚠️ Potential issue | 🟠 MajorScope the Bootstrap tooltip reset to
.frm_tooltip.Line 3 still applies the base tooltip rules globally, so any tooltip on screens loading this stylesheet can pick up Formidable’s
z-index, padding, and arrow overrides. Prefix the.tooltip,.tooltip-inner, and.bs-tooltip-*selectors with.frm_tooltipto keep this isolated to Formidable’s tooltip variant.Suggested direction
-.tooltip{...}.tooltip.show{...}.tooltip .arrow{...}.tooltip-inner{...} -.bs-tooltip-top,...,.bs-tooltip-left,... +.tooltip.frm_tooltip{...}.tooltip.frm_tooltip.show{...}.tooltip.frm_tooltip .arrow{...}.tooltip.frm_tooltip .tooltip-inner{...} +.bs-tooltip-top.frm_tooltip,...,.bs-tooltip-left.frm_tooltip,...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@css/frm_testing_mode.css` at line 3, The global Bootstrap tooltip rules (.tooltip, .tooltip-inner, .bs-tooltip-top/.bs-tooltip-right/.bs-tooltip-bottom/.bs-tooltip-left and related .arrow rules) are applied site-wide; scope them to Formidable by prefixing those selectors with the Formidable container (use .frm_tooltip or .frm_tooltip .tooltip as appropriate) so only Formidable’s tooltip variant receives the z-index, padding, arrow and opacity overrides; update all occurrences of .tooltip, .tooltip-inner, .bs-tooltip-* and .arrow selectors in this diff to the prefixed versions to isolate the styles.js/src/admin/admin.js (3)
9726-9727:⚠️ Potential issue | 🟡 MinorUse the
formidabletext domain here.Keeping
formidable-proon Line 9726 will leave this validation message untranslated in the core/Lite admin UI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/src/admin/admin.js` around lines 9726 - 9727, The translation domain used for the validation message is incorrect; update the call that defines validationFailMessage (the __(...) invocation that currently uses 'formidable-pro') to use the core 'formidable' text domain instead so the message is translatable in the Lite admin UI—this change occurs where validationFailMessage is created and passed to frmAdminBuild.infoModal.
387-389:⚠️ Potential issue | 🔴 CriticalUse
typeofwhen normalizing string modal input.Line 387 compares
msgto the literal'string', so plain-string callers never become{ msg }and the modal falls back to the default message instead of the passed text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/src/admin/admin.js` around lines 387 - 389, The conditional that attempts to normalize a string modal input is comparing the variable to the literal 'string' instead of using typeof; change the check in the modal normalization (around the block that currently reads "if ( 'string' === msg ) { msg = { msg }; }") to use typeof msg === 'string' so plain-string callers are wrapped into the object form (msg = { msg }) and the modal receives the passed text correctly.
2253-2264:⚠️ Potential issue | 🔴 CriticalGate pricing fields on drag/drop too.
This modal only blocks click-to-add.
insertNewFieldByDragging()still insertsproduct,quantity, andtotaldirectly, so the payment-setup requirement is still bypassable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/src/admin/admin.js` around lines 2253 - 2264, The payment-setup check currently shown when clicking to add fields is missing from the drag handler, so update insertNewFieldByDragging to enforce the same restriction: when frm_admin_js.shouldShowPaymentsSettingsModal is true and the fieldType is one of 'product','quantity','total', abort the insertion and call the same infoModal configuration (using frm_admin_js.paymentsSettingsUrl and the same title/msg/actionText/closeText/noCenter) and return false; ensure you reference insertNewFieldByDragging and the existing modal logic to keep behavior identical to the click-to-add path.
🧹 Nitpick comments (1)
stripe/controllers/FrmStrpLiteAppController.php (1)
61-68: Rename this helper or narrow the docblock.
is_payment_gateway_configured()currently just forwardsFrmTransLiteAppHelper::payments_table_exists(), which reads more like an installation-state check than a gateway-configuration check. The current name/docblock overstate what this method guarantees and make future call sites easy to misuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stripe/controllers/FrmStrpLiteAppController.php` around lines 61 - 68, The method is_payment_gateway_configured() only proxies FrmTransLiteAppHelper::payments_table_exists(), which checks for a payments table (installation state) not actual gateway config; either rename this method to reflect that (e.g., payments_table_exists or has_payments_table) or change its docblock/behavior to accurately reflect what it checks: update the docblock to say "Checks whether the payments table exists (installation/state check)" and/or implement a true gateway configuration check inside is_payment_gateway_configured() that queries gateway settings (e.g., examine stored API keys/options) instead of calling FrmTransLiteAppHelper::payments_table_exists(); update all call sites to use the new name or rely on the new behavior accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/src/admin/admin.js`:
- Around line 405-406: The code injects untrusted HTML via
$info.find('.frm-info-msg').html(msg.msg) which creates an XSS sink; replace
this with a sanitized insertion by either using text() for plain text
($info.find('.frm-info-msg').text(msg.msg)) or, if HTML is required, run msg.msg
through a sanitizer (e.g. DOMPurify.sanitize(msg.msg) before passing to
.html()). Update the infoModal/handler that sets msg.msg to import/require
DOMPurify (or add a safe fallback) and use DOMPurify.sanitize(msg.msg) where
.html(...) is called so only allowed markup is rendered.
- Around line 9704-9716: In handleModalDismiss, the current cleanup only removes
click handlers from modalDismissers and misses non-click closes (Escape or
.dialog('close')); update it to attach a single dialogclose handler on
'#frm_info_modal' that calls the existing onModalClose logic (adds
'frm_invalid_field', sets focus) and removes all added listeners (both the
per-element click listeners and the dialogclose handler itself) so cleanup runs
for all close methods; reference the modalDismissers NodeList and the
onModalClose function and ensure you detach via the dialog's 'dialogclose' event
(and/or jQuery .off('dialogclose')) to prevent handlers persisting for
subsequent modals.
In `@stripe/controllers/FrmStrpLiteAppController.php`:
- Around line 91-95: The call site uses self:: which prevents overridden methods
from being invoked; update the call in FrmStrpLiteAppController (the block that
currently calls self::is_payment_gateway_configured() and
self::get_payments_settings_url()) to use late static binding
(static::is_payment_gateway_configured() and
static::get_payments_settings_url()) so subclasses overriding those public
static wrapper methods receive proper polymorphic behavior.
---
Duplicate comments:
In `@css/frm_testing_mode.css`:
- Line 3: The global Bootstrap tooltip rules (.tooltip, .tooltip-inner,
.bs-tooltip-top/.bs-tooltip-right/.bs-tooltip-bottom/.bs-tooltip-left and
related .arrow rules) are applied site-wide; scope them to Formidable by
prefixing those selectors with the Formidable container (use .frm_tooltip or
.frm_tooltip .tooltip as appropriate) so only Formidable’s tooltip variant
receives the z-index, padding, arrow and opacity overrides; update all
occurrences of .tooltip, .tooltip-inner, .bs-tooltip-* and .arrow selectors in
this diff to the prefixed versions to isolate the styles.
In `@js/src/admin/admin.js`:
- Around line 9726-9727: The translation domain used for the validation message
is incorrect; update the call that defines validationFailMessage (the __(...)
invocation that currently uses 'formidable-pro') to use the core 'formidable'
text domain instead so the message is translatable in the Lite admin UI—this
change occurs where validationFailMessage is created and passed to
frmAdminBuild.infoModal.
- Around line 387-389: The conditional that attempts to normalize a string modal
input is comparing the variable to the literal 'string' instead of using typeof;
change the check in the modal normalization (around the block that currently
reads "if ( 'string' === msg ) { msg = { msg }; }") to use typeof msg ===
'string' so plain-string callers are wrapped into the object form (msg = { msg
}) and the modal receives the passed text correctly.
- Around line 2253-2264: The payment-setup check currently shown when clicking
to add fields is missing from the drag handler, so update
insertNewFieldByDragging to enforce the same restriction: when
frm_admin_js.shouldShowPaymentsSettingsModal is true and the fieldType is one of
'product','quantity','total', abort the insertion and call the same infoModal
configuration (using frm_admin_js.paymentsSettingsUrl and the same
title/msg/actionText/closeText/noCenter) and return false; ensure you reference
insertNewFieldByDragging and the existing modal logic to keep behavior identical
to the click-to-add path.
---
Nitpick comments:
In `@stripe/controllers/FrmStrpLiteAppController.php`:
- Around line 61-68: The method is_payment_gateway_configured() only proxies
FrmTransLiteAppHelper::payments_table_exists(), which checks for a payments
table (installation state) not actual gateway config; either rename this method
to reflect that (e.g., payments_table_exists or has_payments_table) or change
its docblock/behavior to accurately reflect what it checks: update the docblock
to say "Checks whether the payments table exists (installation/state check)"
and/or implement a true gateway configuration check inside
is_payment_gateway_configured() that queries gateway settings (e.g., examine
stored API keys/options) instead of calling
FrmTransLiteAppHelper::payments_table_exists(); update all call sites to use the
new name or rely on the new behavior accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aa77ccd3-0071-4014-9cf9-4fc25c24870e
⛔ Files ignored due to path filters (2)
images/upsell/pricing-fields.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
classes/helpers/FrmAppHelper.phpclasses/views/shared/info-overlay.phpcss/frm_admin.csscss/frm_testing_mode.cssjs/formidable_admin.jsjs/src/admin/admin.jsresources/scss/admin/components/modal/_info-modals.scssstripe/controllers/FrmStrpLiteAppController.php
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/views/shared/info-overlay.php
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
stripe/controllers/FrmStrpLiteAppController.php (1)
60-69:is_payment_gateway_configured()reads broader than what it actually checks.
FrmTransLiteAppHelper::payments_table_exists()instripe/helpers/FrmTransLiteAppHelper.php:29-56only checks installation state, not whether Stripe is actually configured with credentials. That makes this new public wrapper easy to misread and reuse incorrectly. I’d either rename it to reflect “payments installed” semantics, or make it validate real gateway configuration before returningtrue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stripe/controllers/FrmStrpLiteAppController.php` around lines 60 - 69, The public method is_payment_gateway_configured() currently just proxies FrmTransLiteAppHelper::payments_table_exists(), which only checks installation state and is misleading; either rename is_payment_gateway_configured() to something like is_payments_table_installed() (and update all callers) OR extend is_payment_gateway_configured() to validate actual Stripe credentials/configuration before returning true (e.g., call the helper that reads stored Stripe keys or use get_option('frm_stripe_*') and verify they are non-empty/valid), keeping the original payments_table_exists() call as part of the new check; update method docblock and all usages accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@classes/controllers/FrmFieldsController.php`:
- Around line 1074-1079: The condition treating the selected product value as
empty incorrectly collapses valid string "0"; in the block handling $type ===
'product' and 'hidden' === $field['type'] (inside FrmFieldsController where
get_product_price is used), replace the empty($field['value']) check with a
strict-empty-string test (e.g., check that $field['value'] === '' or use
strlen($field['value']) === 0) or use isset($field['value']) and $field['value']
!== '' before calling self::get_product_price($field), so that a value of "0" is
preserved and correctly passed to get_product_price and used for data-frmprice.
In `@classes/helpers/FrmCurrencyHelper.php`:
- Around line 97-105: The preg_match_all branch can still let end($matches[0])
return false when there are no matches; update the extraction to explicitly
check that $matches[0] is non-empty before calling end and otherwise set $price
to 0 (e.g. replace the current `$matches ? end($matches[0]) : 0` logic with an
explicit `!empty($matches[0]) ? end($matches[0]) : 0`), then continue to call
self::maybe_use_decimal($price, $currency) and the subsequent str_replace only
when $price is truthy so the function prepare_price (and uses of preg_match_all,
$matches, maybe_use_decimal) never returns a false non-numeric value.
In `@classes/models/fields/FrmFieldProduct.php`:
- Around line 223-226: The current validation uses empty($args['value']) which
treats '0' as empty and will skip legitimate zero-like product selections; in
FrmFieldProduct::validate (and surrounding logic that builds $frm_products)
replace the empty() check with a strict existence/empty-string check such as (!
isset($args['value']) || $args['value'] === '') so that '0' (and other non-empty
zero-like values) are preserved during validation and not erroneously skipped
while still handling missing or truly empty values.
In `@js/formidable.js`:
- Around line 2061-2068: The decimal heuristic in maybeUseDecimal currently
treats a dot followed by exactly two digits as a decimal; update it to accept a
one- or two-digit suffix after '.' to match the PHP helper: in maybeUseDecimal,
change the check on amountParts[1].length so usedForDecimal becomes true when
the fractional part length is 1 or 2 (e.g., length === 1 || length === 2, or
length >=1 && length <=2), then proceed to replace the '.' with
currency.decimal_separator as before so inputs like "12.5" are parsed as "12.5"
for EUR/DKK/NOK-style locales.
- Around line 1824-1837: Remove the illegal reassignment of the const $this in
the non-SELECT branch: do not reassign $this = jQuery(this) inside the else
branch; instead rely on the already-declared const $this (and existing
isUserDef/isSingle variables) initialized before the if (this.tagName ===
'SELECT') check so radio/checkbox/hidden product fields can be processed without
throwing a TypeError in the quantity/price calculation logic.
---
Nitpick comments:
In `@stripe/controllers/FrmStrpLiteAppController.php`:
- Around line 60-69: The public method is_payment_gateway_configured() currently
just proxies FrmTransLiteAppHelper::payments_table_exists(), which only checks
installation state and is misleading; either rename
is_payment_gateway_configured() to something like is_payments_table_installed()
(and update all callers) OR extend is_payment_gateway_configured() to validate
actual Stripe credentials/configuration before returning true (e.g., call the
helper that reads stored Stripe keys or use get_option('frm_stripe_*') and
verify they are non-empty/valid), keeping the original payments_table_exists()
call as part of the new check; update method docblock and all usages
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 188c3ee6-9919-4f6f-b4d1-88fb79b148c7
📒 Files selected for processing (7)
classes/controllers/FrmFieldsController.phpclasses/helpers/FrmCurrencyHelper.phpclasses/models/FrmField.phpclasses/models/fields/FrmFieldProduct.phpclasses/models/fields/FrmFieldTotal.phpjs/formidable.jsstripe/controllers/FrmStrpLiteAppController.php
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/models/FrmField.php
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
js/formidable.js (2)
1977-1983:⚠️ Potential issue | 🟠 MajorGuard
data-frmproductbefore parsing.Line 1980 can throw for empty or invalid
data-frmproduct, which breaks total calculation before the generic-quantity fallback runs.🐛 Minimal fix
quantityFields.each( function() { let ids; - - ids = JSON.parse( this.getAttribute( 'data-frmproduct' ).trim() ); - if ( '' === ids ) { + const rawIds = this.getAttribute( 'data-frmproduct' ).trim(); + if ( '' === rawIds ) { + return true; + } + try { + ids = JSON.parse( rawIds ); + } catch ( e ) { return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/formidable.js` around lines 1977 - 1983, In quantityFields.each (the loop that reads the data-frmproduct attribute) guard the call to JSON.parse by first reading the attribute into a variable, checking it's non-null/non-empty and well-formed before parsing; if the attribute is missing, empty, or invalid, skip parsing or fall back to the generic-quantity behavior. Use a try-catch around JSON.parse of this.getAttribute('data-frmproduct') to catch malformed JSON and treat it as an empty/absent value so the function (quantityFields.each / the total calculation) continues without throwing.
2060-2067:⚠️ Potential issue | 🟠 MajorAccept 1-digit decimals when
.may be the decimal mark.With the current check, inputs like
12.5in.-thousands/,-decimal locales are parsed as125, so totals are wrong for user-defined prices.🐛 Minimal fix
if ( currency.thousand_separator == '.' ) { amountParts = amount.split( '.' ); - usedForDecimal = ( 2 == amountParts.length && 2 == amountParts[1].length ); + usedForDecimal = amountParts.length === 2 && [ 1, 2 ].includes( amountParts[1].length ); if ( usedForDecimal ) { amount = amount.replace( '.', currency.decimal_separator ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/formidable.js` around lines 2060 - 2067, The function maybeUseDecimal currently treats the dot as a thousands separator unless the fractional part has exactly 2 digits, causing inputs like "12.5" to be mis-parsed; update the usedForDecimal check in maybeUseDecimal to accept fractional parts of length 1 or 2 (e.g., change the condition from amountParts[1].length == 2 to amountParts[1].length >= 1 && amountParts[1].length <= 2) so "12.5" will be treated as a decimal and the replace( '.', currency.decimal_separator ) will run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/formidable.js`:
- Around line 1896-1899: The current branch in the isLargeTotal check always
uses Math.ceil for zero-decimal currencies which biases totals up; change the
zero-decimal branch to use nearest rounding (e.g., Math.round) instead of
Math.ceil so total is rounded to the nearest whole unit (keep using Math.round10
for decimals > 0 and apply Math.round to the total when currency.decimals === 0,
referencing the existing symbols total, currency.decimals, isLargeTotal,
Math.round10).
---
Duplicate comments:
In `@js/formidable.js`:
- Around line 1977-1983: In quantityFields.each (the loop that reads the
data-frmproduct attribute) guard the call to JSON.parse by first reading the
attribute into a variable, checking it's non-null/non-empty and well-formed
before parsing; if the attribute is missing, empty, or invalid, skip parsing or
fall back to the generic-quantity behavior. Use a try-catch around JSON.parse of
this.getAttribute('data-frmproduct') to catch malformed JSON and treat it as an
empty/absent value so the function (quantityFields.each / the total calculation)
continues without throwing.
- Around line 2060-2067: The function maybeUseDecimal currently treats the dot
as a thousands separator unless the fractional part has exactly 2 digits,
causing inputs like "12.5" to be mis-parsed; update the usedForDecimal check in
maybeUseDecimal to accept fractional parts of length 1 or 2 (e.g., change the
condition from amountParts[1].length == 2 to amountParts[1].length >= 1 &&
amountParts[1].length <= 2) so "12.5" will be treated as a decimal and the
replace( '.', currency.decimal_separator ) will run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1382eb89-b727-4f1d-8df2-39afeda4cd05
📒 Files selected for processing (1)
js/formidable.js
Contributor
Author
|
@Crabcyborg I fixed all of them. There is a Psalm error, can you take a look at it? In the code, the |
Contributor
@truongwp I believe it's because it checks for something like We can use I made this commit to get Psalm passing 2d98651 |
Crabcyborg
reviewed
Mar 26, 2026
Crabcyborg
reviewed
Mar 26, 2026
Crabcyborg
reviewed
Mar 26, 2026
Crabcyborg
reviewed
Mar 26, 2026
Crabcyborg
reviewed
Mar 26, 2026
…rmidable-forms into rock-enable-pricing-fields
Crabcyborg
reviewed
Mar 27, 2026
Crabcyborg
reviewed
Mar 27, 2026
Crabcyborg
reviewed
Mar 27, 2026
Crabcyborg
approved these changes
Mar 30, 2026
Contributor
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
Thanks @truongwp!
I think we can merge this now.
I'll continue to try testing it out as I work on PayPal. Having this merged means I can try both updates together more easily.
🚀
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please check the Handoff for the details: https://linear.app/strategy11/document/feature-handoff-a33762f44d84
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores