Defer when field settings HTML is added to page - #3532
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 2 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: Strategy11/formidable-forms/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
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 |
franky-the-going-merry
Bot
added
the
franky-working
 |
|
Overall Grade |
Security  Reliability  Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| PHP |  |
Oct 2, 2026 9:35p.m. | Review ↗ |
| JavaScript |  |
Oct 2, 2026 9:35p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
| FrmFieldsController::load_single_field( $field, array( 'doing_ajax' => true ) ); | ||
| $html = ob_get_clean(); | ||
|
|
||
| $this->assertStringContainsString( 'id="frm-single-settings-' . $field->id . '"', $html ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $html = ob_get_clean(); | ||
|
|
||
| $this->assertStringContainsString( 'id="frm-single-settings-' . $field->id . '"', $html ); | ||
| $this->assertStringNotContainsString( 'frm-deferred-settings-meta', $html ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| ) | ||
| ); | ||
|
|
||
| $this->assertSame( 'Changed field', FrmField::getOne( $edited->id )->name ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
|
|
||
| $this->assertSame( 'Changed field', FrmField::getOne( $edited->id )->name ); | ||
| $after = FrmField::getOne( $untouched->id ); | ||
| $this->assertSame( $before->name, $after->name ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( 'Changed field', FrmField::getOne( $edited->id )->name ); | ||
| $after = FrmField::getOne( $untouched->id ); | ||
| $this->assertSame( $before->name, $after->name ); | ||
| $this->assertSame( $before->default_value, $after->default_value ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( 12, (int) $settings->meta['order'] ); | ||
| $this->assertSame( 'frm_half custom_class', $settings->meta['classes'] ); | ||
| $this->assertSame( $field->name, $settings->meta['name'] ); | ||
| $this->assertSame( $type, $settings->meta['type'] ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( 'frm_half custom_class', $settings->meta['classes'] ); | ||
| $this->assertSame( $field->name, $settings->meta['name'] ); | ||
| $this->assertSame( $type, $settings->meta['type'] ); | ||
| $this->assertSame( $field->field_key, $settings->meta['key'] ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( $field->name, $settings->meta['name'] ); | ||
| $this->assertSame( $type, $settings->meta['type'] ); | ||
| $this->assertSame( $field->field_key, $settings->meta['key'] ); | ||
| $this->assertStringContainsString( 'id="frm-single-settings-' . $field->id . '"', $settings->html ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( $type, $settings->meta['type'] ); | ||
| $this->assertSame( $field->field_key, $settings->meta['key'] ); | ||
| $this->assertStringContainsString( 'id="frm-single-settings-' . $field->id . '"', $settings->html ); | ||
| $this->assertStringContainsString( 'name="frm_fields_submitted[]"', $settings->html ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( $field->field_key, $settings->meta['key'] ); | ||
| $this->assertStringContainsString( 'id="frm-single-settings-' . $field->id . '"', $settings->html ); | ||
| $this->assertStringContainsString( 'name="frm_fields_submitted[]"', $settings->html ); | ||
| $this->assertStringContainsString( 'name="field_options[type_' . $field->id . ']"', $settings->html ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| FrmFieldsController::load_single_field( $field, array( 'doing_ajax' => true ) ); | ||
| $html = ob_get_clean(); | ||
|
|
||
| $this->assertStringContainsString( 'id="frm-single-settings-' . $field->id . '"', $html ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $html = ob_get_clean(); | ||
|
|
||
| $this->assertStringContainsString( 'id="frm-single-settings-' . $field->id . '"', $html ); | ||
| $this->assertStringNotContainsString( 'frm-deferred-settings-meta', $html ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| ) | ||
| ); | ||
|
|
||
| $this->assertSame( 'Changed field', FrmField::getOne( $edited->id )->name ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
|
|
||
| $this->assertSame( 'Changed field', FrmField::getOne( $edited->id )->name ); | ||
| $after = FrmField::getOne( $untouched->id ); | ||
| $this->assertSame( $before->name, $after->name ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( 'Changed field', FrmField::getOne( $edited->id )->name ); | ||
| $after = FrmField::getOne( $untouched->id ); | ||
| $this->assertSame( $before->name, $after->name ); | ||
| $this->assertSame( $before->default_value, $after->default_value ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( 12, (int) $settings->meta['order'] ); | ||
| $this->assertSame( 'frm_half custom_class', $settings->meta['classes'] ); | ||
| $this->assertSame( $field->name, $settings->meta['name'] ); | ||
| $this->assertSame( $type, $settings->meta['type'] ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( 'frm_half custom_class', $settings->meta['classes'] ); | ||
| $this->assertSame( $field->name, $settings->meta['name'] ); | ||
| $this->assertSame( $type, $settings->meta['type'] ); | ||
| $this->assertSame( $field->field_key, $settings->meta['key'] ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( $field->name, $settings->meta['name'] ); | ||
| $this->assertSame( $type, $settings->meta['type'] ); | ||
| $this->assertSame( $field->field_key, $settings->meta['key'] ); | ||
| $this->assertStringContainsString( 'id="frm-single-settings-' . $field->id . '"', $settings->html ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( $type, $settings->meta['type'] ); | ||
| $this->assertSame( $field->field_key, $settings->meta['key'] ); | ||
| $this->assertStringContainsString( 'id="frm-single-settings-' . $field->id . '"', $settings->html ); | ||
| $this->assertStringContainsString( 'name="frm_fields_submitted[]"', $settings->html ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( $field->field_key, $settings->meta['key'] ); | ||
| $this->assertStringContainsString( 'id="frm-single-settings-' . $field->id . '"', $settings->html ); | ||
| $this->assertStringContainsString( 'name="frm_fields_submitted[]"', $settings->html ); | ||
| $this->assertStringContainsString( 'name="field_options[type_' . $field->id . ']"', $settings->html ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| FrmFieldsController::load_single_field( $field, array( 'doing_ajax' => true ) ); | ||
| $html = ob_get_clean(); | ||
|
|
||
| $this->assertStringContainsString( 'id="frm-single-settings-' . $field->id . '"', $html ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $html = ob_get_clean(); | ||
|
|
||
| $this->assertStringContainsString( 'id="frm-single-settings-' . $field->id . '"', $html ); | ||
| $this->assertStringNotContainsString( 'frm-deferred-settings-meta', $html ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| ) | ||
| ); | ||
|
|
||
| $this->assertSame( 'Changed field', FrmField::getOne( $edited->id )->name ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
|
|
||
| $this->assertSame( 'Changed field', FrmField::getOne( $edited->id )->name ); | ||
| $after = FrmField::getOne( $untouched->id ); | ||
| $this->assertSame( $before->name, $after->name ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( 'Changed field', FrmField::getOne( $edited->id )->name ); | ||
| $after = FrmField::getOne( $untouched->id ); | ||
| $this->assertSame( $before->name, $after->name ); | ||
| $this->assertSame( $before->default_value, $after->default_value ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( 12, (int) $settings->meta['order'] ); | ||
| $this->assertSame( 'frm_half custom_class', $settings->meta['classes'] ); | ||
| $this->assertSame( $field->name, $settings->meta['name'] ); | ||
| $this->assertSame( $type, $settings->meta['type'] ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( 'frm_half custom_class', $settings->meta['classes'] ); | ||
| $this->assertSame( $field->name, $settings->meta['name'] ); | ||
| $this->assertSame( $type, $settings->meta['type'] ); | ||
| $this->assertSame( $field->field_key, $settings->meta['key'] ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( $field->name, $settings->meta['name'] ); | ||
| $this->assertSame( $type, $settings->meta['type'] ); | ||
| $this->assertSame( $field->field_key, $settings->meta['key'] ); | ||
| $this->assertStringContainsString( 'id="frm-single-settings-' . $field->id . '"', $settings->html ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( $type, $settings->meta['type'] ); | ||
| $this->assertSame( $field->field_key, $settings->meta['key'] ); | ||
| $this->assertStringContainsString( 'id="frm-single-settings-' . $field->id . '"', $settings->html ); | ||
| $this->assertStringContainsString( 'name="frm_fields_submitted[]"', $settings->html ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( $field->field_key, $settings->meta['key'] ); | ||
| $this->assertStringContainsString( 'id="frm-single-settings-' . $field->id . '"', $settings->html ); | ||
| $this->assertStringContainsString( 'name="frm_fields_submitted[]"', $settings->html ); | ||
| $this->assertStringContainsString( 'name="field_options[type_' . $field->id . ']"', $settings->html ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| FrmFieldsController::load_single_field( $field, array( 'doing_ajax' => true ) ); | ||
| $html = ob_get_clean(); | ||
|
|
||
| $this->assertStringContainsString( 'id="frm-single-settings-' . $field->id . '"', $html ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $html = ob_get_clean(); | ||
|
|
||
| $this->assertStringContainsString( 'id="frm-single-settings-' . $field->id . '"', $html ); | ||
| $this->assertStringNotContainsString( 'frm-deferred-settings-meta', $html ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| ) | ||
| ); | ||
|
|
||
| $this->assertSame( 'Changed field', FrmField::getOne( $edited->id )->name ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
|
|
||
| $this->assertSame( 'Changed field', FrmField::getOne( $edited->id )->name ); | ||
| $after = FrmField::getOne( $untouched->id ); | ||
| $this->assertSame( $before->name, $after->name ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( 'Changed field', FrmField::getOne( $edited->id )->name ); | ||
| $after = FrmField::getOne( $untouched->id ); | ||
| $this->assertSame( $before->name, $after->name ); | ||
| $this->assertSame( $before->default_value, $after->default_value ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( 12, (int) $settings->meta['order'] ); | ||
| $this->assertSame( 'frm_half custom_class', $settings->meta['classes'] ); | ||
| $this->assertSame( $field->name, $settings->meta['name'] ); | ||
| $this->assertSame( $type, $settings->meta['type'] ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( 'frm_half custom_class', $settings->meta['classes'] ); | ||
| $this->assertSame( $field->name, $settings->meta['name'] ); | ||
| $this->assertSame( $type, $settings->meta['type'] ); | ||
| $this->assertSame( $field->field_key, $settings->meta['key'] ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( $field->name, $settings->meta['name'] ); | ||
| $this->assertSame( $type, $settings->meta['type'] ); | ||
| $this->assertSame( $field->field_key, $settings->meta['key'] ); | ||
| $this->assertStringContainsString( 'id="frm-single-settings-' . $field->id . '"', $settings->html ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( $type, $settings->meta['type'] ); | ||
| $this->assertSame( $field->field_key, $settings->meta['key'] ); | ||
| $this->assertStringContainsString( 'id="frm-single-settings-' . $field->id . '"', $settings->html ); | ||
| $this->assertStringContainsString( 'name="frm_fields_submitted[]"', $settings->html ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
| $this->assertSame( $field->field_key, $settings->meta['key'] ); | ||
| $this->assertStringContainsString( 'id="frm-single-settings-' . $field->id . '"', $settings->html ); | ||
| $this->assertStringContainsString( 'name="frm_fields_submitted[]"', $settings->html ); | ||
| $this->assertStringContainsString( 'name="field_options[type_' . $field->id . ']"', $settings->html ); |
There was a problem hiding this comment.
The method you are trying to call is not defined, which can result in a fatal error.
![franky-the-going-merry[bot]](https://avatars.githubusercontent.com/in/4233782?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approve with two non-blocking performance notes. I found no correctness regression.
Live-tested on this PR's branch (preview env, "Big builder test" form, 37 fields, 26 deferred):
- Builder loads with 26
.frm-deferred-settings-metacontainers and 13 settings panels (the rest are deferred). - Click on a deferred radio field materializes its panel; inputs are correct and the preview and name stay in sync.
- Edit a label on a deferred field, Duplicate another, then Save: reload showed the edit and the copy persisted, every other field kept its name, type and order, and there were no duplicate field keys.
- After reload I opened all 37 fields: each had its settings, name matching its preview label, and options present on radio/checkbox/select. No console errors.
- Delete a field that was duplicated and materialized: preview and settings both gone, no stray
_<id>]inputs left to submit.
Not exercised, so not claimed:
- Drag/drop reorder and the layout-class (field group) path. I could not drive the sortable with synthetic mouse events here, so these are source-read only.
- The new PHPUnit tests: CI skipped PHPUnit/PHPStan/PHPCS on this head, and I did not run them locally.
- Pro and add-on JS against the deferred types. I read Pro's
builder.js: it guards on missing settings for the slider code, and itsfrm_ajax_loaded_fieldlistener only handles RTE, which is not deferred.
Two notes, inline below:
- One option delete materializes every pending panel (verified live: 13 to 39 panels).
- A reorder materializes every field whose index shifted (source-read).
| * Delete a field option. | ||
| */ | ||
| function deleteFieldOption() { | ||
| materializeAllFieldSettings( prepareLoadedFieldMarkup ); |
There was a problem hiding this comment.
Non-blocking, verified live: deleting a single option on a loaded field runs materializeAllFieldSettings() and inserts every pending panel. On the 37-field test form that took the settings count from 13 to 39 (about 160 ms). On a 150-field form most of the deferral is lost the first time anyone edits or deletes a choice. The same call is added at line 6437 and in adjustConditionalLogicOptionOrders (line 7132).
Only fields that have logic rows pointing at the changed field need their panel. A cheap guard would be to skip the call unless some pending panel could contain a .frm_logic_row, or to track which field IDs have logic rows in the preview metadata.
| if ( currentOrder != newOrder && null !== currentOrder ) { | ||
| field.value = newOrder; | ||
| singleField = fields[ i ].querySelector( `#frm-single-settings-${ fieldId }` ); | ||
| singleField = ensureFieldSettings( fieldId ); |
There was a problem hiding this comment.
Non-blocking, source-read (I could not drive a drag live): ensureFieldSettings( fieldId ) here runs for every field whose index shifted. Dropping or inserting a field near the top of a long form shifts almost every field after it, so one drag materializes all of them in one synchronous loop. Only the order input has to reach the save form, and the metadata field_order_<id> input already carries it. Moving that input into the form, and leaving the panel deferred, would keep reorder cheap.
franky-the-going-merry
Bot
removed
franky-review
franky-working
1 participant
Uh oh!
There was an error while loading. Please reload this page.