Skip to content

Drop blank options when using Bulk Edit Options #3345

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 5 commits into
base: master
Choose a base branch
from
+289 −3
Open

Drop blank options when using Bulk Edit Options #3345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b8370b7
Add regression test for blank bulk-edit options
vivi-the-going-merry[bot] Sep 16, 2026
c578816
Drop blank lines when parsing Bulk Edit Options
vivi-the-going-merry[bot] Sep 16, 2026
9c598b9
Also drop blank separate-value halves; simplify and reindex parse_bul…
vivi-the-going-merry[bot] Sep 16, 2026
ecd34d3
Preserve a select's leading blank option, drop blank-label separate v…
vivi-the-going-merry[bot] Sep 17, 2026
911bb43
Don't drop blank-label separate-value options after all
vivi-the-going-merry[bot] Sep 17, 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
69 changes: 66 additions & 3 deletions classes/controllers/FrmFieldsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,11 +348,11 @@
return;
}

$field = FrmFieldsHelper::setup_edit_vars( $field );
$field_type = $field->type;
$field = FrmFieldsHelper::setup_edit_vars( $field );

$opts = FrmAppHelper::get_param( 'opts', '', 'post', 'wp_kses_post' );
$opts = explode( "\n", rtrim( $opts, "\n" ) );
$opts = array_map( 'trim', $opts );
$opts = self::parse_bulk_edit_opts( $opts, $field_type );

$separate = FrmAppHelper::get_param( 'separate', '', 'post', 'sanitize_text_field' );
$field['separate_value'] = $separate === 'true';
Expand All @@ -369,6 +369,8 @@
}
unset( $opt_key, $opt );
}

$opts = self::remove_blank_separated_values( $opts );
}

// Keep other options after bulk update.
Expand All @@ -394,6 +396,67 @@
wp_die();
}

/**
* Splits raw Bulk Edit Options textarea content into trimmed option
* strings, dropping blank lines. A blank line left in as an option with
* an empty string value collides with an unset field value in
* FrmAppHelper::check_selected(), making that blank option render as
* selected by default (formidable-pro#3385).
*
* A leading blank line on a select field is left in place: it's a
* renderer-supported way to give the dropdown a blank first option when
* no placeholder is set (dropdown-field.php's own $placeholder/$skipped
* handling), and select's own default-selection behavior doesn't have
* the radio/checkbox "nothing visibly checked" collision this drops
* blanks for elsewhere.
*
* @since 6.36
*
* @param string $opts
* @param string $field_type
*
* @return array
*/
private static function parse_bulk_edit_opts( $opts, $field_type ) {
$opts = array_map( 'trim', explode( "\n", $opts ) );

$keep_leading_blank = 'select' === $field_type && '' === $opts[0];

$opts = array_values( array_filter( $opts, 'strlen' ) );

Check failure on line 425 in classes/controllers/FrmFieldsController.php

View workflow job for this annotation

GitHub Actions / PHPStan 

Parameter #2 $callback of function array_filter expects (callable(string): bool)|null, 'strlen' given.

if ( $keep_leading_blank ) {
array_unshift( $opts, '' );
}

return $opts;
}

/**
* Drops a separate-value bulk-edit option ("label|value") whose value
* half is blank - same collision as parse_bulk_edit_opts() above, just
* reached via the separate-value split instead of a blank textarea line
* (formidable-pro#3385). A blank label with a real value is left alone:
* FrmAppHelper::check_selected() only ever compares against the value
* half, and dropdown-field.php explicitly supports rendering a
* blank-label option as a real, selectable choice.
*
* @since 6.36
*
* @param array $opts
*
* @return array
*/
private static function remove_blank_separated_values( $opts ) {
return array_values(
array_filter(
$opts,
function ( $opt ) {
return ! is_array( $opt ) || '' !== $opt['value'];
}
)
);
}
Comment on lines +449 to +458

@franky-the-going-merry franky-the-going-merry Bot Sep 16, 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: the predicate only inspects the value half, so the mirror-image malformed line survives.

|value splits to array( 'label' => '', 'value' => 'value' ). '' !== $opt['value'] is true, so it's kept, and the option renders with an empty label against a real value. That isn't the check_selected() collision — the value is non-blank, so nothing pre-selects — but it's the same shape of malformed input arriving through the same split, and the docblock above describes the helper as handling that split generally.

Deciding to keep it is fine; it just isn't stated anywhere. A line in the docblock saying only the value half is checked, and why, would stop the next reader assuming both halves are covered.

Also: explode( '|', $opt ) at :362 keeps only $vals[0] and $vals[1], so a|b|c silently discards c. Pre-existing, not yours, and not worth widening this PR for — noting it because it's in the block you're now filtering.

@vivi-the-going-merry vivi-the-going-merry Bot Sep 17, 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.

Kept, per your note - and now documented in the docblock why: check_selected() only ever compares the value half, and dropdown-field.php renders a blank-label option deliberately, so a value line survives on purpose rather than by omission. Added a test locking in that a blank-label/real-value line survives.


/**
* @since 4.0
*
Expand Down
107 changes: 107 additions & 0 deletions tests/phpunit/fields/test_FrmFieldsAjax.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ public function setUp(): void {
$this->user_id = $this->factory->user->create( array( 'role' => 'administrator' ) );
wp_set_current_user( $this->user_id );
FrmAppHelper::maybe_add_permissions();
// maybe_add_permissions() grants the cap via a separate WP_User
// instance, which doesn't refresh the cached current user - add it
// directly so current_user_can() (used by import_options()'s own
// is_admin()/current_user_can() gate) sees it.
wp_get_current_user()->add_cap( 'frm_edit_forms' );

$form = $this->factory->form->create_and_get();
$this->assertNotEmpty( $form );
Expand Down Expand Up @@ -98,6 +103,108 @@ public function test_duplicating_text_field() {
self::check_in_section_variable( $field, 0 );
}

/**
* @covers FrmFieldsController::import_options
*/
public function test_import_options_drops_blanks_without_disturbing_other_key() {
$field = $this->factory->field->create_and_get(
array(
'form_id' => $this->form_id,
'type' => 'checkbox',
'field_options' => array( 'other' => '1' ),
'options' => array(
array(
'label' => 'Existing',
'value' => 'existing-value',
),
'other_2' => 'Other',
),
)
);

$_POST = array(
'action' => 'frm_import_options',
'nonce' => wp_create_nonce( 'frm_ajax' ),
'field_id' => $field->id,
'opts' => "One\n\nTwo",
'separate' => 'false',
);

$response = $this->trigger_action( 'frm_import_options' );

// The merged-back "other_2" option renders via a Pro-only view
// (FrmProAppHelper's other-option.php), a no-op with Pro inactive
// here, so it never appears as its own row - but if blank-line
// filtering reindexed it into a plain integer key, it would leak
// through as a normal option labeled "Other" instead. Asserting
// its exact absence from a normal render is what proves the
// "other_" key survived the filtering intact.
preg_match_all( '/\[label\]" value="([^"]*)"/', $response, $matches );
// First match is always the hidden "New Option" template row
// (FrmFieldsHelper::hidden_field_option()), not a real option.
$this->assertSame( array( 'One', 'Two' ), array_slice( $matches[1], 1 ) );

@deepsource-io deepsource-io Bot Sep 17, 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_FrmFieldsAjax::assertSame()


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

}

/**
* @covers FrmFieldsController::import_options
*/
public function test_import_options_separate_value_drops_blank_value_but_keeps_blank_label() {
$field = $this->factory->field->create_and_get(
array(
'form_id' => $this->form_id,
'type' => 'radio',
'field_options' => array( 'other' => '1' ),
'options' => array(
array(
'label' => 'Existing',
'value' => 'existing-value',
),
'other_2' => 'Other',
),
)
);

$_POST = array(
'action' => 'frm_import_options',
'nonce' => wp_create_nonce( 'frm_ajax' ),
'field_id' => $field->id,
// "Blank|" (blank value) drops; "|no-label" (blank label, real
// value) survives - it's a legitimate option, not a collision.
'opts' => "One|1\nBlank|\n|no-label\nTwo|2",
'separate' => 'true',
);

$response = $this->trigger_action( 'frm_import_options' );

preg_match_all( '/\[label\]" value="([^"]*)"/', $response, $matches );
$this->assertSame( array( 'One', '', 'Two' ), array_slice( $matches[1], 1 ) );

@deepsource-io deepsource-io Bot Sep 17, 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_FrmFieldsAjax::assertSame()


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

}

/**
* @covers FrmFieldsController::import_options
*/
public function test_import_options_keeps_leading_blank_for_select() {
$field = $this->factory->field->create_and_get(
array(
'form_id' => $this->form_id,
'type' => 'select',
)
);

$_POST = array(
'action' => 'frm_import_options',
'nonce' => wp_create_nonce( 'frm_ajax' ),
'field_id' => $field->id,
'opts' => "\nOne\nTwo",
'separate' => 'false',
);

$response = $this->trigger_action( 'frm_import_options' );

preg_match_all( '/\[label\]" value="([^"]*)"/', $response, $matches );
$this->assertSame( array( '', 'One', 'Two' ), array_slice( $matches[1], 1 ) );

@deepsource-io deepsource-io Bot Sep 17, 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_FrmFieldsAjax::assertSame()


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

}

/**
* Get a field object by key.
*
Expand Down
116 changes: 116 additions & 0 deletions tests/phpunit/fields/test_FrmFieldsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,122 @@ private function prepare_placeholder( $field ) {
return $this->run_private_method( array( 'FrmFieldsController', 'prepare_placeholder' ), array( $field ) );
}

/**
* @covers FrmFieldsController::parse_bulk_edit_opts
*/
public function test_parse_bulk_edit_opts_drops_blank_lines() {
// A blank line (or one that is only whitespace) must be dropped, not
// kept as an option with an empty string value - an empty value
// collides with an unset field value in FrmAppHelper::check_selected()
// and renders as selected by default (formidable-pro#3385).
$opts = $this->parse_bulk_edit_opts( "One\n\nTwo\n \nThree", 'radio' );

$this->assertSame( array( 'One', 'Two', 'Three' ), $opts );
}
Comment on lines +42 to +50

@franky-the-going-merry franky-the-going-merry Bot Sep 16, 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: both tests exercise the helpers in isolation, so import_options()'s own wiring is still uncovered.

parse_bulk_edit_opts() and remove_blank_separated_values() are each called directly through run_private_method(). That verifies the two predicates, which is the right start, but it means nothing covers the parts most likely to break later:

  • remove_blank_separated_values() runs after the label|value loop at :360-370, so it sees a mix of arrays and plain strings. The unit test hands it that mix by hand; nothing proves import_options() actually produces it in that order.
  • parse_bulk_edit_opts() now returns array_values()-reindexed keys, which then meet the other_* string keys in array_merge( $opts, $other_array ) at :387. The reindex is correct, but that interaction is exactly what a later refactor would break silently.

One test through import_options() with $_POST['opts'] containing a blank line, separate=true, and a field that has an other option would cover all of it, and would have caught either wiring mistake.

The PR description says the three helper tests were confirmed red against the old code — that's the right discipline and worth keeping for this one too.

@vivi-the-going-merry vivi-the-going-merry Bot Sep 17, 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.

Added integration tests driving the real frm_import_options AJAX action end to end (test_FrmFieldsAjax.php): blank-line dropping, the select leading-blank case, the label|value split ordering, and the other_ key surviving the reindex/merge.


public function test_parse_bulk_edit_opts_keeps_zero_value() {
// '0' is falsy but a valid option value - only truly blank lines drop.
$opts = $this->parse_bulk_edit_opts( "0\nOne", 'checkbox' );

$this->assertSame( array( '0', 'One' ), $opts );
}

public function test_parse_bulk_edit_opts_keeps_leading_blank_for_select() {
// A blank first line is a legitimate manual placeholder option on a
// select field (dropdown-field.php's own placeholder/skip handling),
// unlike radio/checkbox where a blank option is always a bug.
$opts = $this->parse_bulk_edit_opts( "\nOne\n\nTwo", 'select' );

$this->assertSame( array( '', 'One', 'Two' ), $opts );
}

public function test_parse_bulk_edit_opts_drops_leading_blank_for_radio() {
$opts = $this->parse_bulk_edit_opts( "\nOne\nTwo", 'radio' );
$this->assertSame( array( 'One', 'Two' ), $opts );
}

/**
* @param string $opts
* @param string $field_type
*/
private function parse_bulk_edit_opts( $opts, $field_type ) {
return $this->run_private_method( array( 'FrmFieldsController', 'parse_bulk_edit_opts' ), array( $opts, $field_type ) );
}

/**
* @covers FrmFieldsController::remove_blank_separated_values
*/
public function test_remove_blank_separated_values_drops_blank_value() {
// A "label|" line with nothing after the separator produces a
// blank value half, the same collision as a blank textarea line
// (formidable-pro#3385), just reached via separate-value mode.
$opts = $this->remove_blank_separated_values(
array(
array(
'label' => 'One',
'value' => '1',
),
array(
'label' => 'Blank',
'value' => '',
),
'Two',
)
);

$this->assertSame(
array(
array(
'label' => 'One',
'value' => '1',
),
'Two',
),
$opts
);
}

public function test_remove_blank_separated_values_keeps_blank_label_with_real_value() {
// A "|value" line with nothing before the separator has a blank
// label but a real value - no collision with an unset field value
// (FrmAppHelper::check_selected() only ever compares the value
// half), and dropdown-field.php renders a blank label as a real,
// selectable option, so this is left alone.
$opts = $this->remove_blank_separated_values(
array(
array(
'label' => 'One',
'value' => '1',
),
array(
'label' => '',
'value' => 'no-label',
),
)
);

$this->assertSame(
array(
array(
'label' => 'One',
'value' => '1',
),
array(
'label' => '',
'value' => 'no-label',
),
),
$opts
);
}

/**
* @param array $opts
*/
private function remove_blank_separated_values( $opts ) {
return $this->run_private_method( array( 'FrmFieldsController', 'remove_blank_separated_values' ), array( $opts ) );
}

/**
* @covers FrmFieldsController::pull_custom_error_body_from_custom_html
*/
Expand Down
Loading