diff --git a/classes/controllers/FrmFieldsController.php b/classes/controllers/FrmFieldsController.php index b201300252..366bc0d436 100644 --- a/classes/controllers/FrmFieldsController.php +++ b/classes/controllers/FrmFieldsController.php @@ -348,11 +348,12 @@ public static function import_options() { return; } - $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 = explode( "\n", rtrim( $opts, "\n" ) ); - $opts = array_map( 'trim', $opts ); + $opts = self::parse_bulk_edit_opts( $opts, $keep_leading_blank ); $separate = FrmAppHelper::get_param( 'separate', '', 'post', 'sanitize_text_field' ); $field['separate_value'] = $separate === 'true'; @@ -369,6 +370,8 @@ public static function import_options() { } unset( $opt_key, $opt ); } + + $opts = self::remove_blank_separated_values( $opts, $keep_leading_blank ); } // Keep other options after bulk update. @@ -394,6 +397,111 @@ 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). + * + * 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. + * + * 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 + * @param bool $keep_leading_blank + * + * @return array + */ + 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( + $opts, + static function ( $value ) { + return '' !== $value; + } + ) + ); + + if ( $keep_leading_blank && $opts ) { + 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. + * + * 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 - 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 + * + * @param array $opts + * @param bool $keep_leading_blank + * + * @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, + 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 ) { + return '' !== self::get_select_placeholder( $field ); + } + /** * @since 4.0 * @@ -865,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 f1b988dd21..85ca4cbee5 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,164 @@ 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_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 ) ); + } + + /** + * @covers FrmFieldsController::import_options + */ + public function test_import_options_drops_leading_blank_for_select_without_placeholder() { + // No placeholder configured - see select_has_placeholder()'s docblock. + $field = $this->factory->field->create_and_get( + array( + 'form_id' => $this->form_id, + 'type' => 'select', + ) + ); + + $labels = $this->import_options_labels( $field->id, "\nOne\nTwo", 'false' ); + + $this->assertSame( array( 'One', 'Two' ), $labels ); + } + + /** + * @covers FrmFieldsController::import_options + */ + public function test_import_options_keeps_leading_blank_for_select_with_placeholder() { + // Placeholder configured - see select_has_placeholder()'s docblock. + $field = $this->factory->field->create_and_get( + array( + 'form_id' => $this->form_id, + 'type' => 'select', + 'field_options' => array( 'placeholder' => 'Choose one' ), + ) + ); + + $labels = $this->import_options_labels( $field->id, "\nOne\nTwo", 'false' ); + + $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() { + // 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, + 'type' => 'select', + 'field_options' => array( 'placeholder' => 'Choose one' ), + ) + ); + + $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' => $opts, + 'separate' => $separate, + ); + + $response = $this->trigger_action( 'frm_import_options' ); + + preg_match_all( '/\[label\]" value="([^"]*)"/', $response, $matches ); + + return 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 1f5c3aca2b..f461ced609 100644 --- a/tests/phpunit/fields/test_FrmFieldsController.php +++ b/tests/phpunit/fields/test_FrmFieldsController.php @@ -36,6 +36,220 @@ 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", 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", false ); + + $this->assertSame( array( '0', 'One' ), $opts ); + } + + public function test_parse_bulk_edit_opts_keeps_leading_blank_when_flagged() { + // $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 ); + } + + 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 ); + } + + 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 + */ + 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 ) ); + } + + /** + * @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 + ); + } + + public function test_remove_blank_separated_values_keeps_leading_blank_pair_when_flagged() { + // $keep_leading_blank is the caller's decision - see this method's docblock. + $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 + ); + } + + 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 + */ + 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 select_has_placeholder( $field ) { + return $this->run_private_method( array( 'FrmFieldsController', 'select_has_placeholder' ), array( $field ) ); + } + /** * @covers FrmFieldsController::pull_custom_error_body_from_custom_html */