Skip to content

Apply frm_setup_new_form_vars on the modal new-form flow #3333

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vivi-the-going-merry wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Apply frm_setup_new_form_vars on the modal new-form flow #3333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
859510e
Apply frm_setup_new_form_vars on the modal new-form flow
vivi-the-going-merry[bot] Sep 13, 2026
327cc97
Address review: trim docblock, add @since, use trigger_action helper …
vivi-the-going-merry[bot] Sep 13, 2026
6119dea
Fill default opts before firing frm_setup_new_form_vars on the modal …
vivi-the-going-merry[bot] Sep 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions classes/controllers/FrmFormsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1066,6 +1066,27 @@ public static function build_new_form() {
*/
$new_values = apply_filters( 'frm_new_form_values', $new_values );

// Match the shape FrmFormsHelper::setup_new_vars() gives this filter, so a callback can
// safely read an existing key (e.g. before_html) before modifying it either way.
$new_values = FrmFormsHelper::fill_default_opts( $new_values, false, $new_values );

/**
* Allows overriding a new form's default option values (before_html, submit_html, etc).
*
* @since 6.36
*
* @param array $values Form values.
*/
$new_values = apply_filters( 'frm_setup_new_form_vars', $new_values );

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// FrmForm::create() reads default option values from $new_values['options'], not top level.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

foreach ( FrmFormsHelper::get_default_opts() as $var => $default ) {
if ( isset( $new_values[ $var ] ) ) {
$new_values['options'][ $var ] = $new_values[ $var ];
}
unset( $var, $default );
}

$form_id = FrmForm::create( $new_values );
/**
* @since 5.3
Expand Down
65 changes: 65 additions & 0 deletions tests/phpunit/forms/test_FrmFormsControllerAjax.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,4 +96,69 @@ private function _check_updated_values( $form_id ) {
$this->assertSame( $posted_val, $actual_val, 'The default value was not updated correctly for field ' . $field->field_key . '.' );
}
}

/**
* @covers FrmFormsController::build_new_form
* with ajax
*/
public function test_build_new_form_applies_frm_setup_new_form_vars_filter() {
add_filter( 'frm_setup_new_form_vars', array( $this, '_set_custom_before_html' ) );

$_POST = array(
'action' => 'frm_install_form',
'nonce' => wp_create_nonce( 'frm_ajax' ),
'name' => 'Vivi Setup New Form Vars Test',
'desc' => '',
);
$_REQUEST = $_POST;

$response = json_decode( $this->trigger_action( 'frm_install_form' ), true );

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.' );

@deepsource-io deepsource-io Bot Sep 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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'] );

@deepsource-io deepsource-io Bot Sep 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$this->assertSame( 'VIVI_TEST_MARKER', $form->options['before_html'], 'frm_setup_new_form_vars did not affect the created form.' );

@deepsource-io deepsource-io Bot Sep 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

public function _set_custom_before_html( $values ) {
$values['before_html'] = 'VIVI_TEST_MARKER';
return $values;
}

/**
* @covers FrmFormsController::build_new_form
* with ajax
*/
public function test_build_new_form_frm_setup_new_form_vars_callback_can_read_existing_key() {
add_filter( 'frm_setup_new_form_vars', array( $this, '_append_to_before_html' ) );

$_POST = array(
'action' => 'frm_install_form',
'nonce' => wp_create_nonce( 'frm_ajax' ),
'name' => 'Vivi Append Before Html Test',
'desc' => '',
);
$_REQUEST = $_POST;

$response = json_decode( $this->trigger_action( 'frm_install_form' ), true );

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.' );

@deepsource-io deepsource-io Bot Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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'] );

@deepsource-io deepsource-io Bot Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$expected = FrmFormsHelper::get_default_html( 'before' ) . '_APPENDED';
$this->assertSame( $expected, $form->options['before_html'], 'Callback could not read the existing before_html default.' );

@deepsource-io deepsource-io Bot Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

public function _append_to_before_html( $values ) {
$values['before_html'] .= '_APPENDED';
return $values;
}
}
Loading