You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.
Use the following commands to manage reviews:
@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.
Use the checkboxes below for quick actions:
▶️ Resume reviews
🔍 Trigger review
📝 Walkthrough
Walkthrough
Adds 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 .htaccess file.
Changes
Cohort / File(s)
Summary
New Field Types classes/models/fields/FrmFieldProduct.php, classes/models/fields/FrmFieldQuantity.php, classes/models/fields/FrmFieldTotal.php
Implements product/quantity/total field classes with builder views, front-end rendering, price extraction, quantity integration, and post-submit total validation.
Factory & Field Metadata classes/factories/FrmFieldFactory.php, classes/models/FrmField.php
Registers new types in factory; adds pricing section to field registry and helpers field_section_labels() and remove_moved_field_types_from_pro().
Field Controllers / HTML attrs classes/controllers/FrmFieldsController.php, classes/helpers/FrmFieldsHelper.php
Adds currency/price data attributes to field HTML and a builder helper show_add_field_link() for sectioned field rendering.
Groups add-field links by section, defers rendering for sectioned fields, and uses new helpers for pro-field removal.
Frontend Logic (JS) js/formidable.js
Large addition: quantity min/max clamping, product total aggregation, currency normalization/formatting, rounding helpers, event bindings for repeaters and field changes (~+430 lines).
Selector refinements for direct-child spans, new .frmright utility, responsive modal image rule, and an added duplicate tooltip block in testing CSS.
DB Version & Migration classes/helpers/FrmAppHelper.php, classes/models/FrmMigrate.php
Bumps DB version to 105, adds migration step to set frm_show_pricing_fields_modal. Note: migrate_to_105() appears duplicated in FrmMigrate. Localizes new admin script strings and clears the option after showing modal.
Stripe Lite helpers stripe/controllers/FrmStrpLiteAppController.php
Adds is_payment_gateway_configured() and get_payments_settings_url() and uses them in redirect logic.
Repository config removal .htaccess
Removes previously committed .htaccess that denied global access then allowed specific assets (file content deleted).
Sequence Diagram
sequenceDiagram
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
Loading
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
🐰 I hopped through fields of product and price,
Quantities counted, totals precise.
JS tallies sums, PHP checks the score,
Currencies sing, options galore—
A rabbit applauds this pricing device.
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name
Status
Explanation
Resolution
Docstring Coverage
⚠️ Warning
Docstring coverage is 78.64% which is insufficient. The required threshold is 80.00%.
Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name
Status
Explanation
Description Check
✅ Passed
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
✅ Passed
The title accurately describes the main feature being implemented: enabling pricing fields (product, quantity, total) in the Lite version of Formidable Forms.
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches🧪 Generate unit tests (beta)
Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch rock-enable-pricing-fields
📝 Coding Plan
Generate coding plan for human review comments
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.
We reviewed changes in 745b145...948977c on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.
We reviewed changes in f434ee9...631c20e on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.
Some issues found as part of this review are outside of the diff in this pull request and aren't shown in the inline review comments due to GitHub's API limitations. You can see those issues on the DeepSource dashboard.
PR Report Card
Overall Grade
Focus Area: Hygiene
Security
Reliability
Complexity
Hygiene
Feedback
Mutable bindings used where immutability suffices
Multiple variables are declared with mutable bindings despite never being reassigned, which obscures intent and enables accidental mutation; prefer const-first and enable ESLint's prefer-const rule to stop the pattern.
Implicit type coercion through concatenation
String concatenation and ad-hoc conversions hide type intent and invite subtle bugs in formatting/price logic; use template literals and explicit String/Number conversions and enforce no-useless-concat and consistent-type linting.
Old JS idioms scattered across the file
The mix of concatenation, manual coercion, and let/var patterns shows no single modern style is enforced; adopt a contemporary style guide, run codemods and ESLint autofixes to align idioms and prevent recurrence.
The reason will be displayed to describe this comment to others. Learn more.
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 | 🟡 Minor
Use the dialogclose event 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-click remains registered and can incorrectly affect subsequent unrelated info modals.
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.
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.
Use the Lite text domain for the new placeholders.
Product Name and Price are added in a Lite template, but both are registered under formidable-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.
"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.
Race 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.
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.
Text 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.
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.
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
getCurrency returns undefined when currency is not found.
If __FRMCURR is defined but doesn't contain the formId, this function returns undefined. Callers like calcProductsTotal then access properties on undefined (e.g., currency.decimal_separator), which will throw a TypeError.
Consider adding a fallback or early return in calcProductsTotal:
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.
Potential type error with trim() on non-string value.
$price_quantity['parent_field_id'] might be null, an integer, or another non-string type. Calling trim() on a non-string in strict environments can cause issues. While PHP will coerce the value, it's safer to cast explicitly.
The empty() check already handles null, empty string, and falsy values, making trim() 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.
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".
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_fields parameter lacks a type specification. Since this is a pass-by-reference parameter modifying an array, add the array type 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.
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 foreach loop with an immediate break is inefficient for retrieving just the first option. Consider using array functions instead:
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.
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: false is 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 closeOnEscape defaults to true in jQuery UI Dialog, you can remove this custom handler or explicitly disable jQuery UI's handler if you need custom behavior:
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 @since tag to new method.
The new add_currency_field_attributes() method should include a @since x.x tag for documentation consistency.
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_id is assigned but never used. Additionally, line 180 contains commented-out code that should be removed.
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_create hook ensures product/quantity data is gathered first. However, the hook is added but never removed. If validate() 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 with self::.
get_data_type_settings() is a private instance method (not static), but it's being called with self::get_data_type_settings(). While this works in PHP, it's misleading. Use $this-> for instance methods.
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() and get_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(). If product_field is already an empty array [], this will evaluate to false and unnecessarily recreate an empty array. While functionally correct, the intent might be clearer with an explicit check.
Casting to (array) handles both null and 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
📥 Commits
Reviewing files that changed from the base of the PR and between 745b145 and 948977c.
⛔ Files ignored due to path filters (2)
images/upsell/pricing-fields.png is excluded by !**/*.png
package-lock.json is excluded by !**/package-lock.json
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
wc -l classes/helpers/FrmCurrencyHelper.php
Repository: Strategy11/formidable-forms
Length of output: 112
🏁 Script executed:
cat -n classes/helpers/FrmCurrencyHelper.php
Repository: Strategy11/formidable-forms
Length of output: 16154
Add type information for the new helper API.
These new methods are missing PHP type declarations:
format_price() has no docstring or type hints
prepare_price(), maybe_use_decimal(), and add_currency_to_global() have docstrings but lack parameter and return type declarations in their signatures
Add type hints to all four methods before merging so CI passes PHPStan checks.
🧰 Tools🪛 GitHub Check: PHPStan
[failure] 129-129:
Method FrmCurrencyHelper::add_currency_to_global() has no return type specified.
[failure] 106-106:
Method FrmCurrencyHelper::maybe_use_decimal() has no return type specified.
[failure] 82-82:
Method FrmCurrencyHelper::prepare_price() has no return type specified.
[failure] 58-58:
Method FrmCurrencyHelper::format_price() has parameter $currency with no type specified.
[failure] 58-58:
Method FrmCurrencyHelper::format_price() has parameter $amount with no type specified.
[failure] 58-58:
Method FrmCurrencyHelper::format_price() has no return type specified.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@classes/helpers/FrmCurrencyHelper.php` around lines 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.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
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
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.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
Normalize composite field keys before building the .frm_t… class.
Line 2374 uses the raw key, but show_add_field_buttons() normalizes | to - for the same class family. Any composite key rendered through this helper will miss the existing styling/JS hooks.
Verify each finding against the current code and only fix it if needed.
In `@classes/helpers/FrmFieldsHelper.php` around lines 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.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cat -n classes/helpers/FrmFormsHelper.php | sed -n '2140,2170p'
Repository: Strategy11/formidable-forms
Length of output: 929
🏁 Script executed:
# Also check the entire method and any surrounding docblocks
cat -n classes/helpers/FrmFormsHelper.php | sed -n '2140,2175p'
Repository: Strategy11/formidable-forms
Length of output: 1203
🏁 Script executed:
# Search for where $frm_vars['currency'] is set to understand data flow
rg -n "frm_vars\[.currency" --type php -B 2 -A 2
Repository: Strategy11/formidable-forms
Length of output: 1481
🏁 Script executed:
# Check if json_encode is used elsewhere in similar contexts
rg -n "json_encode" classes/helpers/FrmFormsHelper.php -B 2 -A 2
Repository: Strategy11/formidable-forms
Length of output: 331
🏁 Script executed:
# Search for wordpress json encoding patterns
rg -n "wp_json_encode" --type php | head -20
Repository: Strategy11/formidable-forms
Length of output: 1662
🏁 Script executed:
# Check what get_currency() returns and how currency data is built
rg -n "public static function get_currency" classes/helpers/FrmCurrencyHelper.php -A 30
Repository: Strategy11/formidable-forms
Length of output: 903
🏁 Script executed:
# Check if currency values go through any sanitization/escaping
rg -n "get_currency|currency\[" classes/helpers/FrmCurrencyHelper.php -B 1 -A 5 | head -60
Repository: Strategy11/formidable-forms
Length of output: 1644
🏁 Script executed:
# Look at the structure of currency array - what keys/values does it contain?
rg -n "currency_symbol|currency_format|currency" classes/helpers/FrmCurrencyHelper.php | head -30
Repository: Strategy11/formidable-forms
Length of output: 1852
🏁 Script executed:
# Look at the get_currencies() function to see the actual currency data structure
rg -n "public static function get_currencies" classes/helpers/FrmCurrencyHelper.php -A 50 | head -70
Repository: Strategy11/formidable-forms
Length of output: 1945
🏁 Script executed:
# Search for where currency data might be filtered or modified
rg -n "apply_filters.*currency|do_action.*currency" --type php
Repository: Strategy11/formidable-forms
Length of output: 53
🏁 Script executed:
# Check if there are any tests or documentation about what goes into currency array
rg -n "symbol_left|symbol_right|decimal_separator" --type php -B 2 -A 2 | head -40
Repository: Strategy11/formidable-forms
Length of output: 3064
🏁 Script executed:
# Check if currency data is ever user-supplied or modified through filters/actions
rg -n "wp_localize_script|wp_add_inline_script" classes/helpers/FrmFormsHelper.php -B 5 -A 5
Repository: Strategy11/formidable-forms
Length of output: 53
🏁 Script executed:
# Check WordPress documentation comment about json_encode vs wp_json_encode
rg -n "wp_localize\|wp_add_inline" classes/ -l | head -10
Repository: Strategy11/formidable-forms
Length of output: 53
🏁 Script executed:
# Verify if the currency symbol values could ever contain script-breaking characters
python3 << 'EOF'import re# These are the hardcoded symbols from the get_currencies functionsymbols = [ '$', '৳', 'R$', 'CAD', '¥', 'Kč', '€', 'C$', 'kr', 'Kč', 'kr', 'dk', 'kr', '€', '£']# Check if any contain characters that could break JSON in script tagbreaking_patterns = [ r'</script>', r'<\/script', r'<!--', r'-->']for symbol in symbols: for pattern in breaking_patterns: if re.search(pattern, symbol, re.IGNORECASE): print(f"Found breaking pattern in: {symbol}")# All symbols are safe - but json_encode doesn't escape </script> anywayprint("All hardcoded symbols are safe")print("\nBut json_encode() does NOT escape </ sequences by default:")import jsontest = {'value': 'test</script>'}print(f"json_encode test: {json.dumps(test)}")EOF
Repository: Strategy11/formidable-forms
Length of output: 206
🏁 Script executed:
# Check WordPress code to understand wp_json_encode behavior
rg -n "wp_json_encode" --type php -A 3 | grep -A 3 "function wp_json_encode"| head -20
Repository: Strategy11/formidable-forms
Length of output: 53
🏁 Script executed:
# Look at how wp_json_encode is used with inline scripts in the codebase
rg -n "wp_json_encode.*script\|script.*wp_json_encode" --type php -B 2 -A 2
Repository: Strategy11/formidable-forms
Length of output: 53
🏁 Script executed:
# Verify the actual behavior difference between json_encode and wp_json_encode for script safety
python3 << 'EOF'# In PHP, wp_json_encode uses JSON_HEX_TAG flag which escapes <, >, and & characters# This makes it safe for use in script tags# json_encode() does not have this flag by defaultprint("Key difference:")print("json_encode() output: {'test': '</script>'} -> {\"test\": \"</script>\"}")print("wp_json_encode() with JSON_HEX_TAG escapes: </ becomes \\u003c/")print("This prevents the </script> from closing the script tag prematurely.")print("\nThe original review comment is CORRECT about the vulnerability.")EOF
Repository: Strategy11/formidable-forms
Length of output: 352
🏁 Script executed:
# Check the exact implementation in FrmListHelper to see the recommended pattern
cat -n classes/helpers/FrmListHelper.php | sed -n '1330,1345p'
Repository: Strategy11/formidable-forms
Length of output: 502
Use wp_json_encode() instead of json_encode() for inline scripts.
Line 2171 embeds raw JSON directly in a <script> tag. Since the currency array contains translatable strings (e.g., the 'name' field), a translation containing </script> will terminate the tag early and break the script. Replace json_encode() with wp_json_encode(), which safely escapes these sequences. This pattern is already used throughout the codebase (e.g., FrmListHelper.php:1337).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@classes/helpers/FrmFormsHelper.php` around lines 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.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cat -n js/frm-pricing-fields-modal.js
Repository: Strategy11/formidable-forms
Length of output: 3261
Scope the dialog mutations to this modal instance.
Lines 33 and 37-44 target every .ui-dialog and .ui-widget-overlay on the page. Opening this modal will move and restyle unrelated jQuery UI dialogs too, which is risky on the builder screen.
Suggested fix
position: { my: 'center', at: 'center', of: window },
create: function() {
- // Move dialog to end of body to ensure proper stacking- jQuery( '.ui-dialog' ).appendTo( 'body' );+ // Move only this dialog widget to the end of body.+ jQuery( this ).dialog( 'widget' ).appendTo( 'body' );
},
open: function() {
- // Add overlay with gray background- jQuery( '.ui-widget-overlay' ).css( {+ const $widget = jQuery( this ).dialog( 'widget' );+ const $overlay = jQuery( '.ui-widget-overlay' ).last();++ $overlay.css( {
'z-index': 100001,
'opacity': 0.7
} );
- jQuery( '.ui-dialog' ).css( {+ $widget.css( {
'z-index': 100002,
'background-color': 'white'
} );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/frm-pricing-fields-modal.js` around lines 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.
rg "frm-pricing-fields-modal|frmPricingModal" -B 3 -A 3 --type php
Repository: Strategy11/formidable-forms
Length of output: 1870
Check if modal is open and improve click handler to work with jQuery UI dialog structure.
The document-level click handler (lines 96-101) doesn't properly account for jQuery UI dialog's widget structure. When the dialog opens, it wraps the original element, so the current checks against $modal may not correctly identify clicks within the dialog or on its overlay. Additionally, if showPricingFieldsModal() is called from within a click handler, that event will bubble to this listener and immediately close the modal.
Use dialog('isOpen') to verify the dialog is open before processing clicks, reference dialog('widget') to target the actual interactive element, and specifically detect overlay clicks:
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Verify each finding against the current code and only fix it if needed.
In `@js/frm-pricing-fields-modal.js` around lines 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.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
cat -n js/src/admin/admin.js | sed -n '385,405p'
Repository: Strategy11/formidable-forms
Length of output: 689
🏁 Script executed:
rg -n "infoModal\(" js/src/admin/admin.js -A 1 | head -60
Repository: Strategy11/formidable-forms
Length of output: 1124
Fix the string-type check before normalizing modal input.
Line 387 compares msg to the literal 'string' instead of checking typeof msg. Every existing infoModal( '...' ) call will fail to convert the string to an object, and Object.assign() will ignore the string argument, leaving the modal with the default message instead of the passed text.
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
if('string'===msg){
msg={ msg };
}
msg=Object.assign({
title: '',
msg: __('Are you sure?','formidable'),
closeText: __('Got it!','formidable'),
actionUrl: '',
actionText: '',
},msg);
if('string'===typeofmsg){
msg={ msg };
}
msg=Object.assign({
title: '',
msg: __('Are you sure?','formidable'),
closeText: __('Got it!','formidable'),
actionUrl: '',
actionText: '',
},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 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).
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
♻️ Duplicate comments (4)
js/src/admin/admin.js (3)
2246-2255: ⚠️ Potential issue | 🟠 Major
Gate pricing fields behind a shared “payments configured?” check.
This branch is unconditional, so click-add is blocked for product, quantity, and total on 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 | 🟠 Major
Fix 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 like checkCalculationCreatedByUser() will flow straight into .html( msg.msg ), which currently accepts raw user-entered markup.
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 | 🔴 Critical
Pass the input element into validateProductPriceValue().
Every other frmDom.util.documentOn() usage in this file treats the callback argument as an event object, but validateProductPriceValue() immediately reads .value. As wired, the first .frm_product_price change will throw before any validation runs.
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 | 🟠 Major
Guard 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 defines get_posted_price(). If a non-product object reaches this path, it will fatal. Narrow the object first (instanceof FrmFieldProduct or method_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 for formTotals.
formTotals is 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
📥 Commits
Reviewing files that changed from the base of the PR and between 948977c and ae5e9af.
⛔ Files ignored due to path filters (1)
package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (14)
classes/controllers/FrmAppController.php
classes/controllers/FrmFieldsController.php
classes/controllers/FrmFormsController.php
classes/helpers/FrmAppHelper.php
classes/helpers/FrmFieldsHelper.php
classes/helpers/FrmFormsHelper.php
classes/models/FrmField.php
classes/models/FrmMigrate.php
classes/views/frm-forms/add_field_links.php
css/frm_admin.css
js/formidable-web-components.js
js/formidable.js
js/formidable_admin.js
js/src/admin/admin.js
🚧 Files skipped from review as they are similar to previous changes (3)
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant section
head -n 420 classes/models/FrmField.php | tail -n 30
Repository: Strategy11/formidable-forms
Length of output: 757
🏁 Script executed:
# Also check around line 410 more precisely to see the method signature
sed -n '403,420p' classes/models/FrmField.php
Repository: Strategy11/formidable-forms
Length of output: 441
🏁 Script executed:
# Check the full method to understand its structure
sed -n '403,430p' classes/models/FrmField.php
Repository: Strategy11/formidable-forms
Length of output: 677
Type $pro_fields as array.
The method signature lacks a type hint for the &$pro_fields parameter, while the body unsets only array keys. Adding the array type makes the contract explicit and resolves static analysis failures.
Suggested fix
- public static function remove_moved_field_types_from_pro( &$pro_fields ) {+ public static function remove_moved_field_types_from_pro( array &$pro_fields ) {
🧰 Tools🪛 GitHub Check: PHPStan
[failure] 410-410:
Method FrmField::remove_moved_field_types_from_pro() has parameter $pro_fields with no type specified.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@classes/models/FrmField.php` at 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.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟡 Minor
Don’t show the section pill unconditionally.
FrmAppHelper::show_pill_text() runs for every section header here, so the Pricing section stays badged indefinitely and any future section added through section / frm_available_field_sections inherits the same badge. Gate it behind a section-specific flag instead of rendering it for all sections.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@classes/views/frm-forms/add_field_links.php` around lines 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.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
Wrap JSON.parse in try-catch to prevent runtime errors.
If data-frmproduct contains invalid JSON, JSON.parse() will throw an exception, causing the entire calculation to fail. The empty string check on Line 1986 won't be reached if parsing fails.
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Verify each finding against the current code and only fix it if needed.
In `@js/formidable.js` around lines 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.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟡 Minor
Use the existing formidable text domain for this message.
This bundle's surrounding admin strings use formidable; keeping formidable-pro here will leave the validation message untranslated in the core admin UI.
Suggested fix
- const validationFailMessage = __( 'Please enter a valid number.', 'formidable-pro' );+ const validationFailMessage = __( 'Please enter a valid number.', 'formidable' );
📝 Committable suggestion
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
constvalidationFailMessage=__('Please enter a valid number.','formidable-pro');
frmAdminBuild.infoModal(validationFailMessage);
constvalidationFailMessage=__('Please enter a valid number.','formidable');
frmAdminBuild.infoModal(validationFailMessage);
🤖 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 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.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
♻️ Duplicate comments (4)
css/frm_testing_mode.css (1)
3-3: ⚠️ Potential issue | 🟠 Major
Scope 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_tooltip to keep this isolated to Formidable’s tooltip variant.
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 | 🟡 Minor
Use the formidable text domain here.
Keeping formidable-pro on 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 | 🔴 Critical
Use typeof when normalizing string modal input.
Line 387 compares msg to 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 | 🔴 Critical
Gate pricing fields on drag/drop too.
This modal only blocks click-to-add. insertNewFieldByDragging() still inserts product, quantity, and total directly, 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.
is_payment_gateway_configured() currently just forwards FrmTransLiteAppHelper::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
📥 Commits
Reviewing files that changed from the base of the PR and between ae5e9af and eb6ec49.
⛔ Files ignored due to path filters (2)
images/upsell/pricing-fields.png is excluded by !**/*.png
package-lock.json is excluded by !**/package-lock.json
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
Sanitize modal HTML before injecting it.
This now renders msg.msg with .html(...). Several callers pass builder input straight into infoModal, so this turns validation/warning paths into an admin XSS sink unless the content is purified first.
Verify each finding against the current code and only fix it if needed.
In `@js/src/admin/admin.js` around lines 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.
# Check if it's referenced as a module
rg "upgrade-popup" js/src/admin/admin.js
Repository: Strategy11/formidable-forms
Length of output: 229
🏁 Script executed:
# Check the upgrade-popup module to understand how modals are initialized
cat js/src/admin/upgrade-popup.js
Repository: Strategy11/formidable-forms
Length of output: 8738
🏁 Script executed:
# Also check what $ variables are available - look for frm_confirm_modal usage
rg "frm_confirm_modal|frm_info_modal" js/src/admin/admin.js -B 1 -A 1 | head -40
Repository: Strategy11/formidable-forms
Length of output: 897
Use dialogclose event to clean up handlers on all dialog close methods.
The current implementation only removes event listeners when the specific dismiss elements are clicked. However, the jQuery UI dialog can also be closed via Escape key (enabled with closeOnEscape: true in initModal) or programmatic calls to .dialog('close'), which bypass these click handlers and leave the listeners attached. This causes the handlers to persist for subsequent modals, potentially marking older fields invalid. Use the dialogclose event instead, following the pattern already established for #frm_confirm_modal elsewhere in the codebase.
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Verify each finding against the current code and only fix it if needed.
In `@js/src/admin/admin.js` around lines 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.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"Check whether FrmStrpLiteAppController is subclassed or these methods are overridden in-repo:"
rg -n -C2 --type=php 'class\s+\w+\s+extends\s+FrmStrpLiteAppController\b|function\s+is_payment_gateway_configured\s*\(|function\s+get_payments_settings_url\s*\('echoecho"Inspect the redirect method call sites:"
rg -n -C3 --type=php 'maybe_redirect_to_stripe_settings\s*\(|self::is_payment_gateway_configured\s*\(|self::get_payments_settings_url\s*\('
Repository: Strategy11/formidable-forms
Length of output: 2181
🏁 Script executed:
#!/bin/bash# Get the full context of FrmStrpLiteAppController.php to understand design intent
cat -n stripe/controllers/FrmStrpLiteAppController.php | head -120
Repository: Strategy11/formidable-forms
Length of output: 3733
🏁 Script executed:
#!/bin/bash# Check if there are any references in PRs, comments, or docs about these methods
rg -n --type=php "is_payment_gateway_configured|get_payments_settings_url" -B2 -A2
Repository: Strategy11/formidable-forms
Length of output: 2161
🏁 Script executed:
#!/bin/bash# Check for class hierarchy or any stripe-related controllers
fd -e php -path "*stripe*controller*"| head -20
Repository: Strategy11/formidable-forms
Length of output: 239
🏁 Script executed:
#!/bin/bash# Check for Pro/Premium/Extended versions or alternate implementations
rg -i "class\s+Frm.*Stripe|class\s+.*Payment.*Pro|class\s+.*Payment.*Extended" --type=php
Repository: Strategy11/formidable-forms
Length of output: 53
🏁 Script executed:
#!/bin/bash# Check git history or comments about why these wrappers were introduced
rg -n "wrapper" --type=php stripe/
Repository: Strategy11/formidable-forms
Length of output: 837
🏁 Script executed:
#!/bin/bash# Check for any recent commits or git blame on the wrapper methods to understand intent
git log --oneline -n 20 -- stripe/controllers/FrmStrpLiteAppController.php
Repository: Strategy11/formidable-forms
Length of output: 124
Use late static binding here if these wrappers are meant to be overridden.
The wrapper methods is_payment_gateway_configured() and get_payments_settings_url() are designed as public static extension points (evident from the "Lite" naming pattern and their extraction as separate methods). Calling them via self:: hard-binds the method resolution to FrmStrpLiteAppController at compile time. If a subclass overrides these methods, those overrides will be ignored. Use static:: to enable late static binding for proper polymorphic behavior.
Suggested fix
- if ( self::is_payment_gateway_configured() ) {+ if ( static::is_payment_gateway_configured() ) {
return;
}
- wp_safe_redirect( self::get_payments_settings_url() );+ wp_safe_redirect( static::get_payments_settings_url() );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@stripe/controllers/FrmStrpLiteAppController.php` around lines 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.
The reason will be displayed to describe this comment to others. Learn more.
'formId' is never reassigned. Use 'const' instead
Variables that are never re-assigned a new value after their initial declaration should be declared with the const keyword. This prevents the programmer from erroneously re-assigning to a read-only variable, and informs those reading the code that a variable is a constant value.
The reason will be displayed to describe this comment to others. Learn more.
'currency' is never reassigned. Use 'const' instead
Variables that are never re-assigned a new value after their initial declaration should be declared with the const keyword. This prevents the programmer from erroneously re-assigning to a read-only variable, and informs those reading the code that a variable is a constant value.
60-69: is_payment_gateway_configured() reads broader than what it actually checks.
FrmTransLiteAppHelper::payments_table_exists() in stripe/helpers/FrmTransLiteAppHelper.php:29-56 only 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 returning true.
🤖 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
📥 Commits
Reviewing files that changed from the base of the PR and between eb6ec49 and c48d9fd.
📒 Files selected for processing (7)
classes/controllers/FrmFieldsController.php
classes/helpers/FrmCurrencyHelper.php
classes/models/FrmField.php
classes/models/fields/FrmFieldProduct.php
classes/models/fields/FrmFieldTotal.php
js/formidable.js
stripe/controllers/FrmStrpLiteAppController.php
🚧 Files skipped from review as they are similar to previous changes (1)
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
Don't collapse the selected value '0' to an empty product.
empty( $field['value'] ) is true for '0', so a valid product option with separate value 0 gets a zero data-frmprice here on later pages. That makes multipage totals drop that selection entirely.
Verify each finding against the current code and only fix it if needed.
In `@classes/controllers/FrmFieldsController.php` around lines 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.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
Handle the no-match path explicitly.
preg_match_all() still leaves $matches populated when nothing matches, so the current \$matches ? end( $matches[0] ) : 0 branch can return false here. That leaks a non-numeric value out of prepare_price() instead of the expected zero fallback.
[failure] 105-105:
Method FrmCurrencyHelper::prepare_price() should return float but returns string|false.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@classes/helpers/FrmCurrencyHelper.php` around lines 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.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
Keep zero-like product selections in $frm_products.
empty( $args['value'] ) also treats '0' as not selected. With separate values, that means a legitimate product choice is skipped during validation, so the expected total can undercount or reject a valid submission.
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
$parent_errors = parent::validate( $args );
if ( $parent_errors || empty( $args['value'] ) ) {
Verify each finding against the current code and only fix it if needed.
In `@classes/models/fields/FrmFieldProduct.php` around lines 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.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
Keep the JS decimal heuristic aligned with the PHP helper.
The PHP helper accepts a 1- or 2-digit suffix after . when . is the thousand separator, but this mirror only accepts 2 digits. For EUR/DKK/NOK-style locales, an input like 12.5 is parsed as 125 in JS while the server turns it into 12.5, so frontend totals can disagree with backend validation.
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Verify each finding against the current code and only fix it if needed.
In `@js/formidable.js` around lines 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.
The reason will be displayed to describe this comment to others. Learn more.
'$form' is never reassigned. Use 'const' instead
Variables that are never re-assigned a new value after their initial declaration should be declared with the const keyword. This prevents the programmer from erroneously re-assigning to a read-only variable, and informs those reading the code that a variable is a constant value.
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 | 🟠 Major
Accept 1-digit decimals when . may be the decimal mark.
With the current check, inputs like 12.5 in .-thousands/,-decimal locales are parsed as 125, so totals are wrong for user-defined prices.
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
📥 Commits
Reviewing files that changed from the base of the PR and between c48d9fd and a508db4.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
Use nearest rounding for zero-decimal currencies.
Line 1898 always rounds up with Math.ceil, so a whole-unit currency total like 100.1 becomes 101 instead of rounding normally. That skews totals, especially once discounts introduce fractions.
🐛 Minimal fix
if ( ! isLargeTotal ) {
const { decimals } = currency;
- total = decimals > 0 ? Math.round10( total, decimals ) : Math.ceil( total );+ total = decimals > 0 ? Math.round10( total, decimals ) : Math.round( total );
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/formidable.js` around lines 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).
The reason will be displayed to describe this comment to others. Learn more.
@truongwp This file gets deleted automatically if it detects an issue on your server.
But we want to keep this file in the repo. We don't want to commit it when it gets deleted.
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
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