Apply frm_setup_new_form_vars on the modal new-form flow by vivi-the-going-merry[bot] · Pull Request #3333 · Strategy11/formidable-forms

vivi-the-going-merry · 2026-09-13T23:48:15Z

What was broken

frm_setup_new_form_vars is documented (https://formidableforms.com/knowledgebase/frm_setup_new_form_vars/) as the filter for overriding a new form's default option values (before_html, submit_html, etc). It's never actually applied on the current "Add New Form" → "Create a blank form" path — FrmFormsController::build_new_form() (the AJAX handler behind that button, wp_ajax_frm_install_form) only applies the newer frm_new_form_values filter. FrmFormsHelper::setup_new_vars(), the only place that fires the documented filter, has no other production caller left in this repo (only test factories and Pro's repeating-section embedded-form creation use it).

Confirmed live with a real WP + Formidable install (Playground, playwright-cli): a frm_setup_new_form_vars callback setting before_html fires (confirmed via a separate marker hook) but the created form's "Before Form" setting still shows the stock default — the filtered value never reaches the database.

Closes Strategy11/formidable-pro#2176

What changed

In build_new_form(), after the existing frm_new_form_values filter: apply frm_setup_new_form_vars too, then fold any of FrmFormsHelper::get_default_opts()'s keys it set back into $new_values['options'] — FrmForm::create() only reads default option values from there, not from the filter's flat $values[$key] shape.

Worth flagging explicitly for review, not something I resolved unilaterally: Formidable Pro also hooks this same filter (FrmProFormsController::setup_new_vars → FrmProFormsHelper::setup_new_vars), unconditionally overwriting its own ~25 option keys from raw $_POST (sanitize_text_field only). That hook was effectively dead on this AJAX endpoint before this change (the filter never fired here) — it now runs whenever Pro is active. In practice this endpoint stays gated on frm_edit_forms + a valid nonce (no privilege escalation — a user who could hit this could already set these same fields via the normal form-settings save right after creation), and the same Pro callback already fires today via the pre-existing repeating-section/embedded-form creation path (FrmProField::create_repeat_form()), so this isn't a new class of exposure, just a wider set of endpoints where it applies. Flagging so this trade-off gets an explicit look rather than being buried in the diff.

How verified

Red/green locally against the real WP-core PHPUnit test lib (~/Claude/test-sites/formidable/wordpress-develop, borrowed and restored per this agent's own SOP): new test fails on unpatched master (assertion mismatch, filtered value never applied) and passes with the fix. Also ran tests/phpunit/forms/test_FrmFormsControllerAjax.php, test_FrmForm.php, and test_FrmFormsController.php for regressions — all pass except a pre-existing, unrelated test_FrmForm::test_duplicate failure reproduced identically on a clean master with none of this diff applied.

No CI run yet (label added after this PR opens); this is the no-CI-fallback verification path documented for this agent.

build_new_form() (the AJAX handler behind "Create a blank form") never applied this documented filter, only the newer frm_new_form_values - so a callback overriding before_html/submit_html/etc had no effect on a form created this way, even though it fired correctly. Apply it and fold its known default-opt keys into $new_values['options'], which is what FrmForm::create() actually reads. Closes Strategy11/formidable-pro#2176

…and response-derived form id in test

coderabbitai · 2026-09-13T23:48:21Z

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: c8975c61-6b45-446b-976b-9e13f9b3de7c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

deepsource-io · 2026-09-13T23:49:13Z

DeepSource Code Review

We reviewed changes in 5d01123...6119dea on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

Important

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	Security Reliability Complexity Hygiene

Code Review Summary

Analyzer	Status	Updated (UTC)	Details
PHP		Sep 14, 2026 12:02p.m.	Review ↗
JavaScript		Sep 14, 2026 12:02p.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 · 2026-09-13T23:49:28Z

+
+		remove_filter( 'frm_setup_new_form_vars', array( $this, '_set_custom_before_html' ) );
+
+		$this->assertNotEmpty( $response['redirect'] ?? '', 'build_new_form did not return a redirect URL.' );


Call to an undefined method test_FrmFormsControllerAjax::assertNotEmpty()

The method you are trying to call is not defined, which can result in a fatal error.

deepsource-io · 2026-09-13T23:49:28Z

+		parse_str( (string) wp_parse_url( $response['redirect'], PHP_URL_QUERY ), $redirect_args );
+
+		$form = FrmForm::getOne( $redirect_args['id'] );
+		$this->assertNotEmpty( $form, 'Form not found with id ' . $redirect_args['id'] );


Call to an undefined method test_FrmFormsControllerAjax::assertNotEmpty()

The method you are trying to call is not defined, which can result in a fatal error.

deepsource-io · 2026-09-13T23:49:28Z

+
+		$form = FrmForm::getOne( $redirect_args['id'] );
+		$this->assertNotEmpty( $form, 'Form not found with id ' . $redirect_args['id'] );
+		$this->assertSame( 'VIVI_TEST_MARKER', $form->options['before_html'], 'frm_setup_new_form_vars did not affect the created form.' );


Call to an undefined method test_FrmFormsControllerAjax::assertSame()

The method you are trying to call is not defined, which can result in a fatal error.

franky-the-going-merry

Verified the root cause and fix directly, not just the PR's own description.

Verification performed (real WP-core PHPUnit harness, ~/Claude/test-sites/formidable/wordpress-develop, since CI is skipped on this bot-authored PR — confirmed bot-author gap, not new):

Traced build_new_form() → FrmForm::create() and confirmed the fold-back loop's premise: create() only ever reads default option values from $values['options'], never top-level — the fix is structurally correct.
Ran the new test (test_build_new_form_applies_frm_setup_new_form_vars_filter) against this branch: passes (2/2, 15 assertions).
Reverted FrmFormsController.php to origin/master (keeping the new test): the same test fails as expected — assertSame mismatch, filtered value never reaches the created form. Genuine red/green, not just claimed.
Ran test_FrmForm.php (9/10 passing consistently across repeat runs) and test_FrmFormsController.php (10/10) — no regressions from this diff.
test_FrmForm::test_duplicate did fail once during testing but was not reproducible on repeat runs of the exact same code (both with and without this diff) — a pre-existing flake unrelated to this change, not something this PR introduced or something that reproduces reliably enough to hold this up.
PHPStan/Psalm both green on the actual checks. DeepSource's "PHP" check failure is 3 inline false positives (assertNotEmpty/assertSame "undefined method" on test_FrmFormsControllerAjax, which extends WP_Ajax_UnitTestCase → WP_UnitTestCase → PHPUnit's own TestCase) — confirmed by actually running the suite, both assertions execute fine.

Two non-blocking notes below on the shape of the new filter application — nothing here blocks merging.

Approve.

franky-the-going-merry · 2026-09-14T00:01:35Z

+		 *
+		 * @param array $values Form values.
+		 */
+		$new_values = apply_filters( 'frm_setup_new_form_vars', $new_values );


Non-blocking: filter fires with a different $values shape here than at its only other call site.

FrmFormsHelper::setup_new_vars() (the pre-existing, only other place that fires this documented filter) calls fill_default_opts() first, so every get_default_opts() key (submit_value, success_msg, custom_style, before_html, after_html, submit_html, etc.) already exists at the top level of $values by the time the filter runs. Here, $new_values has none of those keys set at this point (only antispam/on_submit_migrated, nested under options) — so a callback written against the documented/long-standing contract (e.g. one that reads an existing key before modifying it, like $values['before_html'] .= '...') would hit an undefined-array-key notice on this call path specifically, even though it works fine on the original path.

Checked the one real-world consumer in-repo context (FrmProFormsController::setup_new_vars → FrmProFormsHelper::setup_new_vars): it only assigns ($values[$var] = FrmAppHelper::get_param(...)), never reads first, so it's unaffected today. But the filter is documented/public, so a third-party callback isn't guaranteed to follow that same write-only pattern.

Suggestion (non-blocking): populate $new_values from FrmFormsHelper::get_default_opts() (or call fill_default_opts()) before firing frm_setup_new_form_vars, so the filter's $values shape is consistent across both call sites regardless of what a given callback happens to do with it.

Fixed: build_new_form() now calls FrmFormsHelper::fill_default_opts( $new_values, false, $new_values ) right before firing this filter, matching setup_new_vars()'s shape. New regression test (test_build_new_form_frm_setup_new_form_vars_callback_can_read_existing_key) covers a callback that reads-then-modifies an existing key. Verified red on unpatched code, green with the fix; full test_FrmForm/test_FrmFormsController/test_FrmFormsControllerAjax suite still green (9 tests, 114 assertions), no regressions.

franky-the-going-merry · 2026-09-14T00:01:35Z

+		 */
+		$new_values = apply_filters( 'frm_setup_new_form_vars', $new_values );
+
+		// FrmForm::create() reads default option values from $new_values['options'], not top level.


Non-blocking: this loop also newly activates the older frm_new_form_values filter's top-level assignments, not just the new one.

Before this diff, FrmForm::create() never read anything from $new_values at the top level for option defaults — only from $new_values['options']. So any existing frm_new_form_values callback (available since 5.4) that happened to set a top-level key matching a get_default_opts() name (e.g. $new_values['before_html'] = ...) was a silent no-op.

This loop runs unconditionally on $new_values after both filters, so it now folds in top-level keys set by either filter — meaning a pre-existing frm_new_form_values callback with that shape starts actually taking effect on form creation, not just a new frm_setup_new_form_vars one. Likely a net improvement (completes a filter that was silently broken) rather than a regression, but worth calling out explicitly as a behavior change in scope beyond "apply the new filter," in case any existing integration relied on that assignment being inert.

Acknowledged, no change here. This is inherent to the fold-back loop itself, not something the new fill_default_opts() call adds — confirmed by re-checking against the prior commit. Agree it is a behavior completion rather than a regression: FrmForm::create() already re-defaults every option key via fill_form_options() regardless, so a frm_new_form_values callback setting a top-level default key now reaches the same place get_default_opts() would have put it anyway.

…path Matches the shape FrmFormsHelper::setup_new_vars() gives this filter at its other call site, so a callback reading an existing key (e.g. before_html) before modifying it works the same regardless of which path fires it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

deepsource-io · 2026-09-14T12:06:34Z

+
+		remove_filter( 'frm_setup_new_form_vars', array( $this, '_append_to_before_html' ) );
+
+		$this->assertNotEmpty( $response['redirect'] ?? '', 'build_new_form did not return a redirect URL.' );


Call to an undefined method test_FrmFormsControllerAjax::assertNotEmpty()

The method you are trying to call is not defined, which can result in a fatal error.

deepsource-io · 2026-09-14T12:06:34Z

+		parse_str( (string) wp_parse_url( $response['redirect'], PHP_URL_QUERY ), $redirect_args );
+
+		$form = FrmForm::getOne( $redirect_args['id'] );
+		$this->assertNotEmpty( $form, 'Form not found with id ' . $redirect_args['id'] );


Call to an undefined method test_FrmFormsControllerAjax::assertNotEmpty()

The method you are trying to call is not defined, which can result in a fatal error.

deepsource-io · 2026-09-14T12:06:34Z

+		$form = FrmForm::getOne( $redirect_args['id'] );
+		$this->assertNotEmpty( $form, 'Form not found with id ' . $redirect_args['id'] );
+		$expected = FrmFormsHelper::get_default_html( 'before' ) . '_APPENDED';
+		$this->assertSame( $expected, $form->options['before_html'], 'Callback could not read the existing before_html default.' );


Call to an undefined method test_FrmFormsControllerAjax::assertSame()

The method you are trying to call is not defined, which can result in a fatal error.

franky-the-going-merry

Re-review at 6119dea (was 327cc97 at my last Approve).

Both non-blocking notes from the prior review are resolved:

The $new_values shape issue: fixed via FrmFormsHelper::fill_default_opts( $new_values, false, $new_values ) right before firing frm_setup_new_form_vars, matching setup_new_vars()'s own call (self::fill_default_opts( $values, false, $post_values ) at line 384) — confirmed by reading both call sites directly. New regression test (test_build_new_form_frm_setup_new_form_vars_callback_can_read_existing_key) covers a callback that reads an existing key before modifying it.
The frm_new_form_values top-level-key activation note: acknowledged as a behavior completion, not a regression — agree with the reasoning given (FrmForm::create() re-defaults every option key via fill_form_options() regardless, so this doesn't introduce a new unguarded path).

Other checks on this pass:

DeepSource: PHP shows fail (critical: "Call to an undefined method test_FrmFormsControllerAjax::assertNotEmpty()/assertSame()"). False positive — assertNotEmpty/assertSame are inherited from WP_Ajax_UnitTestCase/PHPUnit's TestCase (via FrmAjaxUnitTest extends WP_Ajax_UnitTestCase) and are already used identically elsewhere in this same file and across the suite (e.g. tests/phpunit/test_ajax.php, tests/phpunit/misc/test_FrmAppController.php) with no complaint — DeepSource's analyzer just doesn't resolve the full WP-core test-lib inheritance chain. PHPStan and Psalm both pass clean on this diff, which corroborates.
Real CI is skipped on this bot-authored PR (confirmed pre-existing gap, not new). Verified the FrmFormsHelper::fill_default_opts/setup_new_vars shape claim and the fold-back loop by reading source directly; did not re-execute the PHPUnit suite locally (no local WP-core test harness set up in this session) — relying on the PR description's stated red/green run plus the source-level trace above, not an independent execution.

No new findings. Approving.

vivi-the-going-merry Bot added 2 commits September 13, 2026 17:38

Address review: trim docblock, add @SInCE, use trigger_action helper …

327cc97

…and response-derived form id in test

vivi-the-going-merry Bot added the franky-review label Sep 13, 2026

deepsource-io Bot reviewed Sep 13, 2026

View reviewed changes

franky-the-going-merry Bot approved these changes Sep 14, 2026

View reviewed changes

franky-the-going-merry Bot added vivi-pickup and removed franky-review labels Sep 14, 2026

vivi-the-going-merry Bot added vivi-working Vivi is actively working this and removed vivi-pickup labels Sep 14, 2026

vivi-the-going-merry Bot added franky-review and removed vivi-working Vivi is actively working this labels Sep 14, 2026

deepsource-io Bot reviewed Sep 14, 2026

View reviewed changes

franky-the-going-merry Bot approved these changes Sep 14, 2026

View reviewed changes

franky-the-going-merry Bot removed the franky-review label Sep 14, 2026


		remove_filter( 'frm_setup_new_form_vars', array( $this, '_set_custom_before_html' ) );

		$this->assertNotEmpty( $response['redirect'] ?? '', 'build_new_form did not return a redirect URL.' );

Conversation

vivi-the-going-merry Bot commented Sep 13, 2026

What was broken

What changed

How verified

Uh oh!

coderabbitai Bot commented Sep 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

deepsource-io Bot commented Sep 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DeepSource Code Review

PR Report Card

Code Review Summary

Uh oh!

deepsource-io Bot Sep 13, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFormsControllerAjax::assertNotEmpty()

Uh oh!

deepsource-io Bot Sep 13, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFormsControllerAjax::assertNotEmpty()

Uh oh!

deepsource-io Bot Sep 13, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFormsControllerAjax::assertSame()

Uh oh!

franky-the-going-merry Bot left a comment

Choose a reason for hiding this comment

Uh oh!

franky-the-going-merry Bot Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

vivi-the-going-merry Bot Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

franky-the-going-merry Bot Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

vivi-the-going-merry Bot Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

deepsource-io Bot Sep 14, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFormsControllerAjax::assertNotEmpty()

Uh oh!

deepsource-io Bot Sep 14, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFormsControllerAjax::assertNotEmpty()

Uh oh!

deepsource-io Bot Sep 14, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFormsControllerAjax::assertSame()

Uh oh!

franky-the-going-merry Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

0 participants

coderabbitai Bot commented Sep 13, 2026 •

edited

Loading

deepsource-io Bot commented Sep 13, 2026 •

edited

Loading