Fix Conditional Logic not copied over when duplicating a field#2945
Fix Conditional Logic not copied over when duplicating a field#2945AbdiTolesa wants to merge 6 commits intomasterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces inline parent form id logic in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FrmFieldsController as FrmFieldsController
participant FrmFormsDB as FrmFormsDB
participant FrmFieldsHelper as FrmFieldsHelper
Client->>FrmFieldsController: request load_single_field(field_object, values)
FrmFieldsController->>FrmFieldsController: call get_form_id_from_field_or_form(field_object, values)
alt values contains form_key
FrmFieldsController-->>FrmFieldsController: return values['id'] if set else field_object->form_id
else
FrmFieldsController->>FrmFormsDB: query parent_form_id by field_object->form_id
FrmFormsDB-->>FrmFieldsController: parent_form_id or null
FrmFieldsController-->>FrmFieldsController: return parent_form_id if set else field_object->form_id
end
FrmFieldsController->>FrmFieldsHelper: FrmFieldsHelper::setup_edit_vars(field_object)
FrmFieldsHelper-->>Client: return setup field data
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| PHP |  |
Feb 24, 2026 9:11a.m. | Review ↗ |
| JavaScript | |
Feb 24, 2026 9:11a.m. | Review ↗ |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@classes/controllers/FrmFieldsController.php`:
- Around line 219-226: The helper get_parent_form_id may return the string '0'
from FrmDb::get_var(...) which makes ?? ineffective for treating top-level
forms; update the method to use the Elvis operator so falsy '0' falls back to
$form_id and ensure an int is returned: in get_parent_form_id( $field_object,
$values ) replace the FrmDb::get_var(... ) ?? $form_id expression with (int)(
FrmDb::get_var( 'frm_forms', array( 'id' => $form_id ), 'parent_form_id' ) ?:
$form_id ), and also cast the early return (when isset($values['form_key'])) to
(int) so the function consistently returns an int.
| private static function get_parent_form_id( $field_object, $values ) { | ||
| $form_id = $field_object->form_id; | ||
| if ( isset( $values['form_key'] ) ) { | ||
| return $values['id'] ?? $form_id; | ||
| } | ||
|
|
||
| return FrmDb::get_var( 'frm_forms', array( 'id' => $form_id ), 'parent_form_id' ) ?? $form_id; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the FrmDb class and its get_var method
fd -t f -e php 'FrmDb' | head -20Repository: Strategy11/formidable-forms
Length of output: 133
🏁 Script executed:
# Check the structure around the method in question
cat -n classes/controllers/FrmFieldsController.php | sed -n '209,230p'Repository: Strategy11/formidable-forms
Length of output: 746
🏁 Script executed:
# Find the include_new_field method mentioned in the comment
rg -n 'include_new_field' --type=php -A 30 | head -50Repository: Strategy11/formidable-forms
Length of output: 4252
🏁 Script executed:
# Check how parent_form_id is actually used downstream
rg -n "parent_form_id" --type=php -B 2 -A 2 | head -100Repository: Strategy11/formidable-forms
Length of output: 6175
🏁 Script executed:
# Find the include_new_field method
rg -n 'public static function include_new_field' --type=php -A 40Repository: Strategy11/formidable-forms
Length of output: 3288
🏁 Script executed:
# Check FrmDb::get_var implementation
cat -n classes/models/FrmDb.php | head -100Repository: Strategy11/formidable-forms
Length of output: 2944
🏁 Script executed:
# Search for where get_parent_form_id is called
rg -n 'get_parent_form_id' --type=php -B 2 -A 2Repository: Strategy11/formidable-forms
Length of output: 934
🏁 Script executed:
# Let's specifically look at the lines around 116-120 mentioned in the review
cat -n classes/controllers/FrmFieldsController.php | sed -n '100,130p'Repository: Strategy11/formidable-forms
Length of output: 1103
🏁 Script executed:
# Let's find the get_var method implementation in FrmDb
rg -n 'public static function get_var' --type=php -A 20Repository: Strategy11/formidable-forms
Length of output: 1386
🏁 Script executed:
# Check how parent_form_id is stored in the database
rg -n 'parent_form_id' classes/models/FrmForm.php | grep -E '(absint|intval|=>)'Repository: Strategy11/formidable-forms
Length of output: 726
🏁 Script executed:
# Let's understand the data type being returned - check wpdb usage
rg -n '\$wpdb->get_var' --type=php -B 2 -A 2 | head -40Repository: Strategy11/formidable-forms
Length of output: 2015
🏁 Script executed:
# Let's check the type casting or conversion in FrmDb methods
rg -n 'check_cache' classes/models/FrmDb.php -A 30 | head -60Repository: Strategy11/formidable-forms
Length of output: 1735
🏁 Script executed:
# Check if there's any type casting in get_var or check_cache
rg -n 'function check_cache' classes/models/FrmDb.php -A 40Repository: Strategy11/formidable-forms
Length of output: 1454
🏁 Script executed:
# Let's also verify how parent_form_id is used in load_single_field and downstream
cat -n classes/controllers/FrmFieldsController.php | sed -n '160,210p'Repository: Strategy11/formidable-forms
Length of output: 2272
🏁 Script executed:
# Check if the field object parent_form_id is actually used after being set
rg -n 'parent_form_id' classes/controllers/FrmFieldsController.php -B 2 -A 2Repository: Strategy11/formidable-forms
Length of output: 1190
🏁 Script executed:
# Check if there's any type casting happening on parent_form_id in setup_edit_vars
rg -n 'setup_edit_vars' classes/helpers/FrmFieldsHelper.php -A 50 | head -80Repository: Strategy11/formidable-forms
Length of output: 1654
🏁 Script executed:
# Let's also check the actual data types involved
rg -n 'parent_form_id' classes/helpers/FrmFieldsHelper.php -B 2 -A 2Repository: Strategy11/formidable-forms
Length of output: 397
🏁 Script executed:
# Let's verify the exact behavior difference by checking WordPress wpdb->get_var documentation behavior
# and also look at whether parent_form_id is ever checked with strict type comparison
rg -n 'parent_form_id.*===' --type=phpRepository: Strategy11/formidable-forms
Length of output: 192
🏁 Script executed:
# Check if parent_form_id is ever cast to int anywhere
rg -n 'intval.*parent_form_id|absint.*parent_form_id|(int).*parent_form_id' --type=phpRepository: Strategy11/formidable-forms
Length of output: 1509
Consider using the Elvis operator ?: instead of null coalescing ?? to handle top-level forms consistently.
FrmDb::get_var() returns '0' (string) when parent_form_id is 0 in the database. The null coalescing operator ?? only triggers on null, so '0' is returned as-is rather than falling back to $form_id.
This differs from include_new_field (line 118), which uses if ( $parent_form_id ) to skip setting the field's parent_form_id for top-level forms. The new helper always assigns a value, potentially causing inconsistent behavior between the two code paths.
Additionally, the return type is documented as @return int, but the method can return a string from the database query. Using the Elvis operator would ensure top-level forms (where parent_form_id = 0) fall back to the form's own ID and maintain type consistency:
Suggested change
- return FrmDb::get_var( 'frm_forms', array( 'id' => $form_id ), 'parent_form_id' ) ?? $form_id;
+ return FrmDb::get_var( 'frm_forms', array( 'id' => $form_id ), 'parent_form_id' ) ?: $form_id;🤖 Prompt for AI Agents
In `@classes/controllers/FrmFieldsController.php` around lines 219 - 226, The
helper get_parent_form_id may return the string '0' from FrmDb::get_var(...)
which makes ?? ineffective for treating top-level forms; update the method to
use the Elvis operator so falsy '0' falls back to $form_id and ensure an int is
returned: in get_parent_form_id( $field_object, $values ) replace the
FrmDb::get_var(... ) ?? $form_id expression with (int)( FrmDb::get_var(
'frm_forms', array( 'id' => $form_id ), 'parent_form_id' ) ?: $form_id ), and
also cast the early return (when isset($values['form_key'])) to (int) so the
function consistently returns an int.
lauramekaj1
left a comment
There was a problem hiding this comment.
@AbdiTolesa I tested it and it's working as expected. Thank you!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 221-223: In FrmFieldsController replace the null-coalescing return
that uses $values['id'] ?? $form_id with a falsy-checking fallback so a value of
'0' or 0 falls back to $form_id; specifically change the return in the block
that checks isset($values['form_key']) (the line returning $values['id'] ??
$form_id) to use a ternary or equivalent falsy check (e.g. return $values['id']
? $values['id'] : $form_id) so it matches the DB-query path behavior.
| if ( isset( $values['form_key'] ) ) { | ||
| return $values['id'] ?? $form_id; | ||
| } |
There was a problem hiding this comment.
?? doesn't guard against $values['id'] === 0.
The DB-query path on line 227 deliberately uses a ternary to catch falsy results (including '0' from the DB). Line 222 uses ??, which only falls back on null / unset — so a $values['id'] of 0 (e.g. when the AJAX request carries no valid form ID) is returned as-is instead of falling back to $form_id.
🛡️ Proposed fix
- return $values['id'] ?? $form_id;
+ return ( $values['id'] ?? 0 ) ?: $form_id;📝 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.
| if ( isset( $values['form_key'] ) ) { | |
| return $values['id'] ?? $form_id; | |
| } | |
| if ( isset( $values['form_key'] ) ) { | |
| return ( $values['id'] ?? 0 ) ?: $form_id; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@classes/controllers/FrmFieldsController.php` around lines 221 - 223, In
FrmFieldsController replace the null-coalescing return that uses $values['id']
?? $form_id with a falsy-checking fallback so a value of '0' or 0 falls back to
$form_id; specifically change the return in the block that checks
isset($values['form_key']) (the line returning $values['id'] ?? $form_id) to use
a ternary or equivalent falsy check (e.g. return $values['id'] ? $values['id'] :
$form_id) so it matches the DB-query path behavior.
truongwp
left a comment
There was a problem hiding this comment.
This looks good to me. Thanks @AbdiTolesa!
Fix https://github.com/Strategy11/formidable-pro/issues/6308
Steps to reproduce
Summary by CodeRabbit