From b8370b7ae32dd417ffd9a156a965cf2653295db1 Mon Sep 17 00:00:00 2001 From: "vivi-the-going-merry[bot]" <308115520+vivi-the-going-merry[bot]@users.noreply.github.com> Date: Wed, 16 Sep 2026 13:33:49 -0600 Subject: [PATCH 1/8] Add regression test for blank bulk-edit options --- .../fields/test_FrmFieldsController.php | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/phpunit/fields/test_FrmFieldsController.php b/tests/phpunit/fields/test_FrmFieldsController.php index 1f5c3aca2b..accc9fdf80 100644 --- a/tests/phpunit/fields/test_FrmFieldsController.php +++ b/tests/phpunit/fields/test_FrmFieldsController.php @@ -36,6 +36,30 @@ 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" ); + + $this->assertSame( array( 'One', 'Two', 'Three' ), array_values( $opts ) ); + } + + 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" ); + + $this->assertSame( array( '0', 'One' ), array_values( $opts ) ); + } + + private function parse_bulk_edit_opts( $opts ) { + return $this->run_private_method( array( 'FrmFieldsController', 'parse_bulk_edit_opts' ), array( $opts ) ); + } + /** * @covers FrmFieldsController::pull_custom_error_body_from_custom_html */ From c57881634c4e36a66ec1cf6b6adaeef840579536 Mon Sep 17 00:00:00 2001 From: "vivi-the-going-merry[bot]" <308115520+vivi-the-going-merry[bot]@users.noreply.github.com> Date: Wed, 16 Sep 2026 13:34:24 -0600 Subject: [PATCH 2/8] Drop blank lines when parsing Bulk Edit Options --- classes/controllers/FrmFieldsController.php | 23 +++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/classes/controllers/FrmFieldsController.php b/classes/controllers/FrmFieldsController.php index b201300252..c1bf545a86 100644 --- a/classes/controllers/FrmFieldsController.php +++ b/classes/controllers/FrmFieldsController.php @@ -351,8 +351,7 @@ public static function import_options() { $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 ); $separate = FrmAppHelper::get_param( 'separate', '', 'post', 'sanitize_text_field' ); $field['separate_value'] = $separate === 'true'; @@ -394,6 +393,26 @@ public static function import_options() { 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). + * + * @since 6.36 + * + * @param string $opts + * + * @return array + */ + private static function parse_bulk_edit_opts( $opts ) { + $opts = explode( "\n", rtrim( $opts, "\n" ) ); + $opts = array_map( 'trim', $opts ); + + return array_filter( $opts, 'strlen' ); + } + /** * @since 4.0 * From 9c598b92f7e59abcc43680888ed3e758b61149ef Mon Sep 17 00:00:00 2001 From: "vivi-the-going-merry[bot]" <308115520+vivi-the-going-merry[bot]@users.noreply.github.com> Date: Wed, 16 Sep 2026 13:39:39 -0600 Subject: [PATCH 3/8] Also drop blank separate-value halves; simplify and reindex parse_bulk_edit_opts --- classes/controllers/FrmFieldsController.php | 30 ++++++++++++-- .../fields/test_FrmFieldsController.php | 41 ++++++++++++++++++- 2 files changed, 66 insertions(+), 5 deletions(-) diff --git a/classes/controllers/FrmFieldsController.php b/classes/controllers/FrmFieldsController.php index c1bf545a86..b155e91812 100644 --- a/classes/controllers/FrmFieldsController.php +++ b/classes/controllers/FrmFieldsController.php @@ -368,6 +368,8 @@ public static function import_options() { } unset( $opt_key, $opt ); } + + $opts = self::remove_blank_separated_values( $opts ); } // Keep other options after bulk update. @@ -407,10 +409,32 @@ public static function import_options() { * @return array */ private static function parse_bulk_edit_opts( $opts ) { - $opts = explode( "\n", rtrim( $opts, "\n" ) ); - $opts = array_map( 'trim', $opts ); + $opts = array_map( 'trim', explode( "\n", $opts ) ); + + return array_values( array_filter( $opts, 'strlen' ) ); + } - return array_filter( $opts, 'strlen' ); + /** + * 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). + * + * @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']; + } + ) + ); } /** diff --git a/tests/phpunit/fields/test_FrmFieldsController.php b/tests/phpunit/fields/test_FrmFieldsController.php index accc9fdf80..c73186d607 100644 --- a/tests/phpunit/fields/test_FrmFieldsController.php +++ b/tests/phpunit/fields/test_FrmFieldsController.php @@ -46,20 +46,57 @@ public function test_parse_bulk_edit_opts_drops_blank_lines() { // and renders as selected by default (formidable-pro#3385). $opts = $this->parse_bulk_edit_opts( "One\n\nTwo\n \nThree" ); - $this->assertSame( array( 'One', 'Two', 'Three' ), array_values( $opts ) ); + $this->assertSame( array( 'One', 'Two', 'Three' ), $opts ); } 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" ); - $this->assertSame( array( '0', 'One' ), array_values( $opts ) ); + $this->assertSame( array( '0', 'One' ), $opts ); } private function parse_bulk_edit_opts( $opts ) { return $this->run_private_method( array( 'FrmFieldsController', 'parse_bulk_edit_opts' ), array( $opts ) ); } + /** + * @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 + ); + } + + 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 */ From ecd34d3421f7da9cccdbe323451664c72985ad07 Mon Sep 17 00:00:00 2001 From: "vivi-the-going-merry[bot]" <308115520+vivi-the-going-merry[bot]@users.noreply.github.com> Date: Wed, 16 Sep 2026 23:56:05 -0600 Subject: [PATCH 4/8] Preserve a select's leading blank option, drop blank-label separate values, add wiring coverage Franky round-1 review: parse_bulk_edit_opts() dropped a select field's legitimate leading blank option (dropdown-field.php's own placeholder/skip handling treats it as a manual "please select" default) along with genuine bugs on radio/checkbox. Now preserves a single leading blank only for select. remove_blank_separated_values() also drops a blank-label half (mirroring the existing blank-value check) and import_options() itself gets integration coverage via the real frm_import_options AJAX action, not just the private helpers in isolation. --- classes/controllers/FrmFieldsController.php | 35 ++++-- tests/phpunit/fields/test_FrmFieldsAjax.php | 105 ++++++++++++++++++ .../fields/test_FrmFieldsController.php | 56 +++++++++- 3 files changed, 183 insertions(+), 13 deletions(-) diff --git a/classes/controllers/FrmFieldsController.php b/classes/controllers/FrmFieldsController.php index b155e91812..a1ed33e121 100644 --- a/classes/controllers/FrmFieldsController.php +++ b/classes/controllers/FrmFieldsController.php @@ -348,10 +348,11 @@ public static function import_options() { 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 = self::parse_bulk_edit_opts( $opts ); + $opts = self::parse_bulk_edit_opts( $opts, $field_type ); $separate = FrmAppHelper::get_param( 'separate', '', 'post', 'sanitize_text_field' ); $field['separate_value'] = $separate === 'true'; @@ -402,23 +403,39 @@ public static function import_options() { * 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 ) { + private static function parse_bulk_edit_opts( $opts, $field_type ) { $opts = array_map( 'trim', explode( "\n", $opts ) ); - return array_values( array_filter( $opts, 'strlen' ) ); + $keep_leading_blank = 'select' === $field_type && isset( $opts[0] ) && '' === $opts[0]; + + $opts = array_values( array_filter( $opts, 'strlen' ) ); + + 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). + * Drops a separate-value bulk-edit option ("label|value") whose label or + * 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). * * @since 6.36 * @@ -431,7 +448,7 @@ private static function remove_blank_separated_values( $opts ) { array_filter( $opts, function ( $opt ) { - return ! is_array( $opt ) || '' !== $opt['value']; + return ! is_array( $opt ) || ( '' !== $opt['value'] && '' !== $opt['label'] ); } ) ); diff --git a/tests/phpunit/fields/test_FrmFieldsAjax.php b/tests/phpunit/fields/test_FrmFieldsAjax.php index f1b988dd21..59a92d61c6 100644 --- a/tests/phpunit/fields/test_FrmFieldsAjax.php +++ b/tests/phpunit/fields/test_FrmFieldsAjax.php @@ -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 ); @@ -98,6 +103,106 @@ 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 ) ); + } + + /** + * @covers FrmFieldsController::import_options + */ + public function test_import_options_separate_value_drops_blank_half_without_disturbing_other_key() { + $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, + '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 ) ); + } + + /** + * @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 ) ); + } + /** * Get a field object by key. * diff --git a/tests/phpunit/fields/test_FrmFieldsController.php b/tests/phpunit/fields/test_FrmFieldsController.php index c73186d607..804c1569e0 100644 --- a/tests/phpunit/fields/test_FrmFieldsController.php +++ b/tests/phpunit/fields/test_FrmFieldsController.php @@ -44,20 +44,38 @@ public function test_parse_bulk_edit_opts_drops_blank_lines() { // 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" ); + $opts = $this->parse_bulk_edit_opts( "One\n\nTwo\n \nThree", 'radio' ); $this->assertSame( array( 'One', 'Two', 'Three' ), $opts ); } 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" ); + $opts = $this->parse_bulk_edit_opts( "0\nOne", 'checkbox' ); $this->assertSame( array( '0', 'One' ), $opts ); } - private function parse_bulk_edit_opts( $opts ) { - return $this->run_private_method( array( 'FrmFieldsController', 'parse_bulk_edit_opts' ), array( $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 ) ); } /** @@ -93,6 +111,36 @@ public function test_remove_blank_separated_values_drops_blank_value() { ); } + public function test_remove_blank_separated_values_drops_blank_label() { + // A "|value" line with nothing before the separator produces a + // blank label half - equally droppable as a blank value half. + $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', + ), + ), + $opts + ); + } + + /** + * @param array $opts + */ private function remove_blank_separated_values( $opts ) { return $this->run_private_method( array( 'FrmFieldsController', 'remove_blank_separated_values' ), array( $opts ) ); } From 911bb43879a4eb524d9d768e7d6013b25a4a105b Mon Sep 17 00:00:00 2001 From: "vivi-the-going-merry[bot]" <308115520+vivi-the-going-merry[bot]@users.noreply.github.com> Date: Thu, 17 Sep 2026 00:02:32 -0600 Subject: [PATCH 5/8] Don't drop blank-label separate-value options after all Self-review caught this: FrmAppHelper::check_selected() only ever compares an option's value half, never its label, so a blank label with a real value doesn't reproduce #3385's collision - and dropdown-field.php already renders a blank label as a real, selectable option. Reverted that part of the previous commit; kept the value-half check and the select leading-blank preservation. Also drops the now-dead isset() check in parse_bulk_edit_opts() (explode() always returns at least one element). --- classes/controllers/FrmFieldsController.php | 15 +++++++++------ tests/phpunit/fields/test_FrmFieldsAjax.php | 6 ++++-- tests/phpunit/fields/test_FrmFieldsController.php | 13 ++++++++++--- 3 files changed, 23 insertions(+), 11 deletions(-) diff --git a/classes/controllers/FrmFieldsController.php b/classes/controllers/FrmFieldsController.php index a1ed33e121..fe74bc43e7 100644 --- a/classes/controllers/FrmFieldsController.php +++ b/classes/controllers/FrmFieldsController.php @@ -420,7 +420,7 @@ public static function import_options() { private static function parse_bulk_edit_opts( $opts, $field_type ) { $opts = array_map( 'trim', explode( "\n", $opts ) ); - $keep_leading_blank = 'select' === $field_type && isset( $opts[0] ) && '' === $opts[0]; + $keep_leading_blank = 'select' === $field_type && '' === $opts[0]; $opts = array_values( array_filter( $opts, 'strlen' ) ); @@ -432,10 +432,13 @@ private static function parse_bulk_edit_opts( $opts, $field_type ) { } /** - * Drops a separate-value bulk-edit option ("label|value") whose label or - * 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). + * 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 * @@ -448,7 +451,7 @@ private static function remove_blank_separated_values( $opts ) { array_filter( $opts, function ( $opt ) { - return ! is_array( $opt ) || ( '' !== $opt['value'] && '' !== $opt['label'] ); + return ! is_array( $opt ) || '' !== $opt['value']; } ) ); diff --git a/tests/phpunit/fields/test_FrmFieldsAjax.php b/tests/phpunit/fields/test_FrmFieldsAjax.php index 59a92d61c6..55f4942295 100644 --- a/tests/phpunit/fields/test_FrmFieldsAjax.php +++ b/tests/phpunit/fields/test_FrmFieldsAjax.php @@ -148,7 +148,7 @@ public function test_import_options_drops_blanks_without_disturbing_other_key() /** * @covers FrmFieldsController::import_options */ - public function test_import_options_separate_value_drops_blank_half_without_disturbing_other_key() { + 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, @@ -168,6 +168,8 @@ public function test_import_options_separate_value_drops_blank_half_without_dist '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', ); @@ -175,7 +177,7 @@ public function test_import_options_separate_value_drops_blank_half_without_dist $response = $this->trigger_action( 'frm_import_options' ); preg_match_all( '/\[label\]" value="([^"]*)"/', $response, $matches ); - $this->assertSame( array( 'One', 'Two' ), array_slice( $matches[1], 1 ) ); + $this->assertSame( array( 'One', '', 'Two' ), array_slice( $matches[1], 1 ) ); } /** diff --git a/tests/phpunit/fields/test_FrmFieldsController.php b/tests/phpunit/fields/test_FrmFieldsController.php index 804c1569e0..55d8944b4e 100644 --- a/tests/phpunit/fields/test_FrmFieldsController.php +++ b/tests/phpunit/fields/test_FrmFieldsController.php @@ -111,9 +111,12 @@ public function test_remove_blank_separated_values_drops_blank_value() { ); } - public function test_remove_blank_separated_values_drops_blank_label() { - // A "|value" line with nothing before the separator produces a - // blank label half - equally droppable as a blank value half. + 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( @@ -133,6 +136,10 @@ public function test_remove_blank_separated_values_drops_blank_label() { 'label' => 'One', 'value' => '1', ), + array( + 'label' => '', + 'value' => 'no-label', + ), ), $opts ); From 1b7775021241a6665130c5a38cee0ab8b8be0a98 Mon Sep 17 00:00:00 2001 From: "vivi-the-going-merry[bot]" <308115520+vivi-the-going-merry[bot]@users.noreply.github.com> Date: Sat, 19 Sep 2026 21:58:02 -0600 Subject: [PATCH 6/8] Fix PHPStan: replace 'strlen' callback with explicit bool closure array_filter()'s $callback is typed callable(string): bool, but strlen() returns int - PHPStan rejects the implicit int-to-bool coercion even though it happens to work at runtime. Same '' !== $value semantics, '0' still survives (falsy but not blank). --- classes/controllers/FrmFieldsController.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/classes/controllers/FrmFieldsController.php b/classes/controllers/FrmFieldsController.php index fe74bc43e7..aeec76787e 100644 --- a/classes/controllers/FrmFieldsController.php +++ b/classes/controllers/FrmFieldsController.php @@ -422,7 +422,14 @@ private static function parse_bulk_edit_opts( $opts, $field_type ) { $keep_leading_blank = 'select' === $field_type && '' === $opts[0]; - $opts = array_values( array_filter( $opts, 'strlen' ) ); + $opts = array_values( + array_filter( + $opts, + static function ( $value ) { + return '' !== $value; + } + ) + ); if ( $keep_leading_blank ) { array_unshift( $opts, '' ); From 68bfdbe74fb7a5fd6c157454aff27901adff8c79 Mon Sep 17 00:00:00 2001 From: "vivi-the-going-merry[bot]" <308115520+vivi-the-going-merry[bot]@users.noreply.github.com> Date: Sat, 19 Sep 2026 22:49:33 -0600 Subject: [PATCH 7/8] Round 3: gate leading-blank preservation on an actual placeholder, fix separate-value regression Franky's round-3 review found three blocking issues: - PHPCS red on this round's own PHPStan fix (blank line between consecutive simple assignments) - phpcbf-equivalent restructuring, no blank line left. - Keeping a select's leading blank option only prevents the check_selected() collision when a placeholder is configured. dropdown-field.php's skip logic never fires without one, so the kept option rendered for real and reproduced the exact bug this PR fixes for the common no-placeholder case. - Separate-value selects lost their leading blank/blank ("|") placeholder row entirely - remove_blank_separated_values() had no equivalent leading-blank carve-out to parse_bulk_edit_opts()'s own. Fix: compute whether the select would actually render its own placeholder (select_has_placeholder(), mirroring add_placeholder_to_select()'s own truthy check without its markup side effect) once at the call site, and thread that single $keep_leading_blank flag through both filtering methods instead of a bare field-type string. --- classes/controllers/FrmFieldsController.php | 68 ++++++++--- tests/phpunit/fields/test_FrmFieldsAjax.php | 65 ++++++++++- .../fields/test_FrmFieldsController.php | 106 +++++++++++++++--- 3 files changed, 206 insertions(+), 33 deletions(-) diff --git a/classes/controllers/FrmFieldsController.php b/classes/controllers/FrmFieldsController.php index aeec76787e..d33d1f75ea 100644 --- a/classes/controllers/FrmFieldsController.php +++ b/classes/controllers/FrmFieldsController.php @@ -348,11 +348,12 @@ public static function import_options() { return; } - $field_type = $field->type; - $field = FrmFieldsHelper::setup_edit_vars( $field ); + $field_type = $field->type; + $field = FrmFieldsHelper::setup_edit_vars( $field ); + $keep_leading_blank = 'select' === $field_type && self::select_has_placeholder( $field ); $opts = FrmAppHelper::get_param( 'opts', '', 'post', 'wp_kses_post' ); - $opts = self::parse_bulk_edit_opts( $opts, $field_type ); + $opts = self::parse_bulk_edit_opts( $opts, $keep_leading_blank ); $separate = FrmAppHelper::get_param( 'separate', '', 'post', 'sanitize_text_field' ); $field['separate_value'] = $separate === 'true'; @@ -370,7 +371,7 @@ public static function import_options() { unset( $opt_key, $opt ); } - $opts = self::remove_blank_separated_values( $opts ); + $opts = self::remove_blank_separated_values( $opts, $keep_leading_blank ); } // Keep other options after bulk update. @@ -403,24 +404,24 @@ public static function import_options() { * 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. + * A leading blank line is only kept when $keep_leading_blank says so + * (select field, placeholder configured - see select_has_placeholder()). + * With a placeholder, dropdown-field.php's own $placeholder/$skipped + * handling absorbs this option into the placeholder it already renders, + * so keeping it is harmless. Without one, nothing skips it - it would + * render for real and reproduce the exact collision this method exists + * to prevent, so it's dropped like any other blank line. * * @since 6.36 * * @param string $opts - * @param string $field_type + * @param bool $keep_leading_blank * * @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]; + private static function parse_bulk_edit_opts( $opts, $keep_leading_blank ) { + $opts = array_map( 'trim', explode( "\n", $opts ) ); + $keep_leading_blank = $keep_leading_blank && '' === $opts[0]; $opts = array_values( array_filter( @@ -447,23 +448,54 @@ static function ( $value ) { * half, and dropdown-field.php explicitly supports rendering a * blank-label option as a real, selectable choice. * + * A leading "|" line (blank label and blank value) is the separate-value + * equivalent of parse_bulk_edit_opts()'s leading blank line, and is kept + * on the same $keep_leading_blank condition for the same reason. + * * @since 6.36 * * @param array $opts + * @param bool $keep_leading_blank * * @return array */ - private static function remove_blank_separated_values( $opts ) { + private static function remove_blank_separated_values( $opts, $keep_leading_blank ) { return array_values( array_filter( $opts, - function ( $opt ) { + function ( $opt, $key ) use ( $keep_leading_blank ) { + if ( $keep_leading_blank && 0 === $key && is_array( $opt ) && '' === $opt['label'] && '' === $opt['value'] ) { + return true; + } + return ! is_array( $opt ) || '' !== $opt['value']; - } + }, + ARRAY_FILTER_USE_BOTH ) ); } + /** + * Whether a select field would render its own placeholder option, per + * add_placeholder_to_select()'s own truthy check - called directly since + * that method's job is echoing markup, not answering this. + * + * @since 6.36 + * + * @param array $field + * + * @return bool + */ + private static function select_has_placeholder( $field ) { + $placeholder = FrmField::get_option( $field, 'placeholder' ); + + if ( ! $placeholder ) { + $placeholder = self::get_default_value_from_name( $field ); + } + + return '' !== $placeholder; + } + /** * @since 4.0 * diff --git a/tests/phpunit/fields/test_FrmFieldsAjax.php b/tests/phpunit/fields/test_FrmFieldsAjax.php index 55f4942295..2d4969d914 100644 --- a/tests/phpunit/fields/test_FrmFieldsAjax.php +++ b/tests/phpunit/fields/test_FrmFieldsAjax.php @@ -183,7 +183,12 @@ public function test_import_options_separate_value_drops_blank_value_but_keeps_b /** * @covers FrmFieldsController::import_options */ - public function test_import_options_keeps_leading_blank_for_select() { + public function test_import_options_drops_leading_blank_for_select_without_placeholder() { + // With no placeholder configured, dropdown-field.php's own skip + // logic never fires (it only skips when $placeholder is truthy), so + // a kept leading blank option would render for real and reproduce + // the exact check_selected() collision this PR fixes (formidable-pro#3385) - + // same as any other blank line, it has to drop. $field = $this->factory->field->create_and_get( array( 'form_id' => $this->form_id, @@ -201,10 +206,68 @@ public function test_import_options_keeps_leading_blank_for_select() { $response = $this->trigger_action( 'frm_import_options' ); + preg_match_all( '/\[label\]" value="([^"]*)"/', $response, $matches ); + $this->assertSame( array( 'One', 'Two' ), array_slice( $matches[1], 1 ) ); + } + + /** + * @covers FrmFieldsController::import_options + */ + public function test_import_options_keeps_leading_blank_for_select_with_placeholder() { + // With a placeholder configured, dropdown-field.php's own + // $placeholder/$skipped handling absorbs this option into the + // placeholder it already renders, so keeping it here is harmless. + $field = $this->factory->field->create_and_get( + array( + 'form_id' => $this->form_id, + 'type' => 'select', + 'field_options' => array( 'placeholder' => 'Choose one' ), + ) + ); + + $_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 ) ); } + /** + * @covers FrmFieldsController::import_options + */ + public function test_import_options_keeps_leading_blank_pair_for_separate_value_select_with_placeholder() { + // The separate-value equivalent of the plain-line case above: a + // leading "|" (blank label, blank value) is the placeholder row and + // has to survive the label|value split the same way. + $field = $this->factory->field->create_and_get( + array( + 'form_id' => $this->form_id, + 'type' => 'select', + 'field_options' => array( 'placeholder' => 'Choose one' ), + ) + ); + + $_POST = array( + 'action' => 'frm_import_options', + 'nonce' => wp_create_nonce( 'frm_ajax' ), + 'field_id' => $field->id, + 'opts' => "|\nYes|1\nNo|0", + 'separate' => 'true', + ); + + $response = $this->trigger_action( 'frm_import_options' ); + + preg_match_all( '/\[label\]" value="([^"]*)"/', $response, $matches ); + $this->assertSame( array( '', 'Yes', 'No' ), array_slice( $matches[1], 1 ) ); + } + /** * Get a field object by key. * diff --git a/tests/phpunit/fields/test_FrmFieldsController.php b/tests/phpunit/fields/test_FrmFieldsController.php index 55d8944b4e..a883ca9388 100644 --- a/tests/phpunit/fields/test_FrmFieldsController.php +++ b/tests/phpunit/fields/test_FrmFieldsController.php @@ -44,38 +44,39 @@ public function test_parse_bulk_edit_opts_drops_blank_lines() { // 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' ); + $opts = $this->parse_bulk_edit_opts( "One\n\nTwo\n \nThree", false ); $this->assertSame( array( 'One', 'Two', 'Three' ), $opts ); } 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' ); + $opts = $this->parse_bulk_edit_opts( "0\nOne", false ); $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' ); + public function test_parse_bulk_edit_opts_keeps_leading_blank_when_flagged() { + // $keep_leading_blank is the caller's decision (select field with a + // placeholder configured - see select_has_placeholder()) that a + // blank first line is a legitimate manual placeholder option rather + // than the bug this method otherwise drops blank lines for. + $opts = $this->parse_bulk_edit_opts( "\nOne\n\nTwo", true ); $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' ); + public function test_parse_bulk_edit_opts_drops_leading_blank_when_not_flagged() { + $opts = $this->parse_bulk_edit_opts( "\nOne\nTwo", false ); $this->assertSame( array( 'One', 'Two' ), $opts ); } /** * @param string $opts - * @param string $field_type + * @param bool $keep_leading_blank */ - private function parse_bulk_edit_opts( $opts, $field_type ) { - return $this->run_private_method( array( 'FrmFieldsController', 'parse_bulk_edit_opts' ), array( $opts, $field_type ) ); + private function parse_bulk_edit_opts( $opts, $keep_leading_blank ) { + return $this->run_private_method( array( 'FrmFieldsController', 'parse_bulk_edit_opts' ), array( $opts, $keep_leading_blank ) ); } /** @@ -145,11 +146,88 @@ public function test_remove_blank_separated_values_keeps_blank_label_with_real_v ); } + public function test_remove_blank_separated_values_keeps_leading_blank_pair_when_flagged() { + // A "|" line (blank label and blank value) at position 0 is the + // separate-value equivalent of parse_bulk_edit_opts()'s leading + // blank line - kept only when $keep_leading_blank says so. + $opts = $this->remove_blank_separated_values( + array( + array( + 'label' => '', + 'value' => '', + ), + array( + 'label' => 'Yes', + 'value' => '1', + ), + ), + true + ); + + $this->assertSame( + array( + array( + 'label' => '', + 'value' => '', + ), + array( + 'label' => 'Yes', + 'value' => '1', + ), + ), + $opts + ); + } + + public function test_remove_blank_separated_values_drops_leading_blank_pair_when_not_flagged() { + $opts = $this->remove_blank_separated_values( + array( + array( + 'label' => '', + 'value' => '', + ), + array( + 'label' => 'Yes', + 'value' => '1', + ), + ) + ); + + $this->assertSame( + array( + array( + 'label' => 'Yes', + 'value' => '1', + ), + ), + $opts + ); + } + /** * @param array $opts + * @param bool $keep_leading_blank + */ + private function remove_blank_separated_values( $opts, $keep_leading_blank = false ) { + return $this->run_private_method( array( 'FrmFieldsController', 'remove_blank_separated_values' ), array( $opts, $keep_leading_blank ) ); + } + + /** + * @covers FrmFieldsController::select_has_placeholder + */ + public function test_select_has_placeholder_true_when_placeholder_set() { + $this->assertTrue( $this->select_has_placeholder( array( 'placeholder' => 'Choose one' ) ) ); + } + + public function test_select_has_placeholder_false_when_no_placeholder() { + $this->assertFalse( $this->select_has_placeholder( array( 'placeholder' => '' ) ) ); + } + + /** + * @param array $field */ - private function remove_blank_separated_values( $opts ) { - return $this->run_private_method( array( 'FrmFieldsController', 'remove_blank_separated_values' ), array( $opts ) ); + private function select_has_placeholder( $field ) { + return $this->run_private_method( array( 'FrmFieldsController', 'select_has_placeholder' ), array( $field ) ); } /** From fb2c862cc4cbfbfbca1f9be931bcede84880181f Mon Sep 17 00:00:00 2001 From: "vivi-the-going-merry[bot]" <308115520+vivi-the-going-merry[bot]@users.noreply.github.com> Date: Sat, 19 Sep 2026 22:58:22 -0600 Subject: [PATCH 8/8] Self-review: dedupe placeholder resolution, guard wholly-blank textarea, trim test comments - Extract get_select_placeholder() so add_placeholder_to_select() and select_has_placeholder() share one resolution instead of two copies that could drift. - A wholly-blank textarea (or a lone separate-value "|" line) now keeps zero options even when $keep_leading_blank is true, instead of leaving one phantom blank option behind - the leading blank only makes sense as the first row of a real list. - Trim redundant WHY comments in the new tests down to a pointer at the docblock that already states it, and extract the repeated POST-and-scrape-labels pattern in the new AJAX tests into one helper. --- classes/controllers/FrmFieldsController.php | 50 ++++++++++---- tests/phpunit/fields/test_FrmFieldsAjax.php | 67 +++++++++---------- .../fields/test_FrmFieldsController.php | 34 ++++++++-- 3 files changed, 93 insertions(+), 58 deletions(-) diff --git a/classes/controllers/FrmFieldsController.php b/classes/controllers/FrmFieldsController.php index d33d1f75ea..366bc0d436 100644 --- a/classes/controllers/FrmFieldsController.php +++ b/classes/controllers/FrmFieldsController.php @@ -412,6 +412,10 @@ public static function import_options() { * render for real and reproduce the exact collision this method exists * to prevent, so it's dropped like any other blank line. * + * A wholly-blank textarea keeps nothing at all, even when + * $keep_leading_blank is true - the leading blank only makes sense as + * the first row of a real option list, not as the entire result. + * * @since 6.36 * * @param string $opts @@ -432,7 +436,7 @@ static function ( $value ) { ) ); - if ( $keep_leading_blank ) { + if ( $keep_leading_blank && $opts ) { array_unshift( $opts, '' ); } @@ -450,7 +454,13 @@ static function ( $value ) { * * A leading "|" line (blank label and blank value) is the separate-value * equivalent of parse_bulk_edit_opts()'s leading blank line, and is kept - * on the same $keep_leading_blank condition for the same reason. + * on the same $keep_leading_blank condition for the same reason - unless + * it's the only line, matching that method's wholly-blank case. $opts + * here is whatever import_options() built from parse_bulk_edit_opts()'s + * own output, so a caller passing $keep_leading_blank without that same + * upstream filtering would need its own '' !== $opts[0] equivalent + * check; there isn't one here because a raw "|" line already survives + * parse_bulk_edit_opts() unfiltered (it isn't the empty string). * * @since 6.36 * @@ -460,6 +470,8 @@ static function ( $value ) { * @return array */ private static function remove_blank_separated_values( $opts, $keep_leading_blank ) { + $keep_leading_blank = $keep_leading_blank && count( $opts ) > 1; + return array_values( array_filter( $opts, @@ -487,13 +499,7 @@ function ( $opt, $key ) use ( $keep_leading_blank ) { * @return bool */ private static function select_has_placeholder( $field ) { - $placeholder = FrmField::get_option( $field, 'placeholder' ); - - if ( ! $placeholder ) { - $placeholder = self::get_default_value_from_name( $field ); - } - - return '' !== $placeholder; + return '' !== self::get_select_placeholder( $field ); } /** @@ -967,22 +973,38 @@ public static function get_default_value_from_name( $field ) { } /** - * Maybe add a blank placeholder option before any options - * in a dropdown. + * Resolves a select field's own placeholder text, falling back to + * get_default_value_from_name(). Shared between add_placeholder_to_select() + * and select_has_placeholder() so the two can't drift. * - * @since 4.04 + * @since 6.36 * * @param array|object $field * - * @return bool True if placeholder was added. + * @return string */ - public static function add_placeholder_to_select( $field ) { + private static function get_select_placeholder( $field ) { $placeholder = FrmField::get_option( $field, 'placeholder' ); if ( ! $placeholder ) { $placeholder = self::get_default_value_from_name( $field ); } + return $placeholder; + } + + /** + * Maybe add a blank placeholder option before any options + * in a dropdown. + * + * @since 4.04 + * + * @param array|object $field + * + * @return bool True if placeholder was added. + */ + public static function add_placeholder_to_select( $field ) { + $placeholder = self::get_select_placeholder( $field ); $use_placeholder = $placeholder; $autocomplete = FrmField::get_option( $field, 'autocom' ); diff --git a/tests/phpunit/fields/test_FrmFieldsAjax.php b/tests/phpunit/fields/test_FrmFieldsAjax.php index 2d4969d914..85ca4cbee5 100644 --- a/tests/phpunit/fields/test_FrmFieldsAjax.php +++ b/tests/phpunit/fields/test_FrmFieldsAjax.php @@ -184,11 +184,7 @@ public function test_import_options_separate_value_drops_blank_value_but_keeps_b * @covers FrmFieldsController::import_options */ public function test_import_options_drops_leading_blank_for_select_without_placeholder() { - // With no placeholder configured, dropdown-field.php's own skip - // logic never fires (it only skips when $placeholder is truthy), so - // a kept leading blank option would render for real and reproduce - // the exact check_selected() collision this PR fixes (formidable-pro#3385) - - // same as any other blank line, it has to drop. + // No placeholder configured - see select_has_placeholder()'s docblock. $field = $this->factory->field->create_and_get( array( 'form_id' => $this->form_id, @@ -196,27 +192,16 @@ public function test_import_options_drops_leading_blank_for_select_without_place ) ); - $_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' ); + $labels = $this->import_options_labels( $field->id, "\nOne\nTwo", 'false' ); - preg_match_all( '/\[label\]" value="([^"]*)"/', $response, $matches ); - $this->assertSame( array( 'One', 'Two' ), array_slice( $matches[1], 1 ) ); + $this->assertSame( array( 'One', 'Two' ), $labels ); } /** * @covers FrmFieldsController::import_options */ public function test_import_options_keeps_leading_blank_for_select_with_placeholder() { - // With a placeholder configured, dropdown-field.php's own - // $placeholder/$skipped handling absorbs this option into the - // placeholder it already renders, so keeping it here is harmless. + // Placeholder configured - see select_has_placeholder()'s docblock. $field = $this->factory->field->create_and_get( array( 'form_id' => $this->form_id, @@ -225,27 +210,18 @@ public function test_import_options_keeps_leading_blank_for_select_with_placehol ) ); - $_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' ); + $labels = $this->import_options_labels( $field->id, "\nOne\nTwo", 'false' ); - preg_match_all( '/\[label\]" value="([^"]*)"/', $response, $matches ); - $this->assertSame( array( '', 'One', 'Two' ), array_slice( $matches[1], 1 ) ); + $this->assertSame( array( '', 'One', 'Two' ), $labels ); } /** * @covers FrmFieldsController::import_options */ public function test_import_options_keeps_leading_blank_pair_for_separate_value_select_with_placeholder() { - // The separate-value equivalent of the plain-line case above: a - // leading "|" (blank label, blank value) is the placeholder row and - // has to survive the label|value split the same way. + // Separate-value equivalent of the plain-line case above - a leading + // "|" (blank label, blank value) is the same placeholder row, see + // remove_blank_separated_values()'s docblock. $field = $this->factory->field->create_and_get( array( 'form_id' => $this->form_id, @@ -254,18 +230,35 @@ public function test_import_options_keeps_leading_blank_pair_for_separate_value_ ) ); + $labels = $this->import_options_labels( $field->id, "|\nYes|1\nNo|0", 'true' ); + + $this->assertSame( array( '', 'Yes', 'No' ), $labels ); + } + + /** + * Runs frm_import_options for a field and returns the rendered option + * labels, dropping the hidden "New Option" template row. + * + * @param int $field_id + * @param string $opts + * @param string $separate + * + * @return array + */ + private function import_options_labels( $field_id, $opts, $separate ) { $_POST = array( 'action' => 'frm_import_options', 'nonce' => wp_create_nonce( 'frm_ajax' ), - 'field_id' => $field->id, - 'opts' => "|\nYes|1\nNo|0", - 'separate' => 'true', + 'field_id' => $field_id, + 'opts' => $opts, + 'separate' => $separate, ); $response = $this->trigger_action( 'frm_import_options' ); preg_match_all( '/\[label\]" value="([^"]*)"/', $response, $matches ); - $this->assertSame( array( '', 'Yes', 'No' ), array_slice( $matches[1], 1 ) ); + + return array_slice( $matches[1], 1 ); } /** diff --git a/tests/phpunit/fields/test_FrmFieldsController.php b/tests/phpunit/fields/test_FrmFieldsController.php index a883ca9388..f461ced609 100644 --- a/tests/phpunit/fields/test_FrmFieldsController.php +++ b/tests/phpunit/fields/test_FrmFieldsController.php @@ -57,10 +57,7 @@ public function test_parse_bulk_edit_opts_keeps_zero_value() { } public function test_parse_bulk_edit_opts_keeps_leading_blank_when_flagged() { - // $keep_leading_blank is the caller's decision (select field with a - // placeholder configured - see select_has_placeholder()) that a - // blank first line is a legitimate manual placeholder option rather - // than the bug this method otherwise drops blank lines for. + // $keep_leading_blank is the caller's decision - see this method's docblock. $opts = $this->parse_bulk_edit_opts( "\nOne\n\nTwo", true ); $this->assertSame( array( '', 'One', 'Two' ), $opts ); @@ -71,6 +68,14 @@ public function test_parse_bulk_edit_opts_drops_leading_blank_when_not_flagged() $this->assertSame( array( 'One', 'Two' ), $opts ); } + public function test_parse_bulk_edit_opts_wholly_blank_keeps_nothing_even_when_flagged() { + // A wholly-cleared textarea saves zero options, not a single + // leftover blank one - the leading blank only makes sense as the + // first row of a real list. + $opts = $this->parse_bulk_edit_opts( "\n\n", true ); + $this->assertSame( array(), $opts ); + } + /** * @param string $opts * @param bool $keep_leading_blank @@ -147,9 +152,7 @@ public function test_remove_blank_separated_values_keeps_blank_label_with_real_v } public function test_remove_blank_separated_values_keeps_leading_blank_pair_when_flagged() { - // A "|" line (blank label and blank value) at position 0 is the - // separate-value equivalent of parse_bulk_edit_opts()'s leading - // blank line - kept only when $keep_leading_blank says so. + // $keep_leading_blank is the caller's decision - see this method's docblock. $opts = $this->remove_blank_separated_values( array( array( @@ -204,6 +207,23 @@ public function test_remove_blank_separated_values_drops_leading_blank_pair_when ); } + public function test_remove_blank_separated_values_wholly_blank_keeps_nothing_even_when_flagged() { + // Same reasoning as parse_bulk_edit_opts()'s wholly-blank case: a + // lone "|" line with nothing else isn't a real option list with a + // placeholder row, so it doesn't get to keep the placeholder either. + $opts = $this->remove_blank_separated_values( + array( + array( + 'label' => '', + 'value' => '', + ), + ), + true + ); + + $this->assertSame( array(), $opts ); + } + /** * @param array $opts * @param bool $keep_leading_blank