Drop blank options when using Bulk Edit Options by vivi-the-going-merry[bot] · Pull Request #3345 · Strategy11/formidable-forms

vivi-the-going-merry · 2026-09-16T19:40:10Z

What was broken

Using the form builder's "Bulk Edit Options" on a radio/checkbox/select (or Likert, via frm_bulk_edit_field_types) field, a blank line in the textarea (or a label| line with nothing after the separator in separate-value mode) became an option with an empty string value. FrmAppHelper::check_selected() compares the field's current value against each option's value with a loose ==, so when the field has no submitted value yet (''), that blank option matches and renders checked/selected by default.

Related: Strategy11/formidable-pro#3385

What changed

FrmFieldsController::import_options() drops blank lines via parse_bulk_edit_opts(), and blank-value separate-value halves (label|) via remove_blank_separated_values() — except a select field's leading blank line, which is a legitimate, renderer-supported "please select" default (dropdown-field.php's own placeholder/skip handling) rather than the check_selected() collision this PR fixes. Behavior change for dropdowns: re-saving Bulk Edit Options on an existing select field no longer drops a leading blank option it already had.
A blank-label half (|value) is left alone: check_selected() never compares the label, so it doesn't reproduce the bug, and dropdown-field.php renders a blank-label option deliberately.
Mirrors the equivalent blank-line filtering formidable-pro's FrmProFieldProduct bulk edit for Product fields already does.

How it was verified

Red/green locally against the real PHPUnit suite (~/Claude/test-sites/formidable/wordpress-develop): unit tests for both private helpers (blank-line dropping, '0' survives, select keeps a leading blank/radio drops it, blank-value separate halves drop, blank-label halves survive) plus new integration tests driving the real frm_import_options AJAX action end-to-end (tests/phpunit/fields/test_FrmFieldsAjax.php) covering the label|value split ordering and the other_* key surviving the reindex. Full fields (114) and ajax (9) groups green afterward (one pre-existing, unrelated failure in test_FrmFieldCombo::test_print_input_atts, a PHP 8.5 deprecation notice leaking into output buffering, present before this change too).

Self-reviewed (security + correctness/simplify lenses) before pushing — security clean; correctness caught that the blank-label drop I'd added wasn't justified (reverted, see above).

…k_edit_opts

coderabbitai · 2026-09-16T19:40:16Z

Important

Review skipped

Bot user detected.

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

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 3b66d6b1-2453-44df-967a-dfef00a69181

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

Use the checkbox below for a quick retry:

🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

deepsource-io · 2026-09-16T19:41:21Z

DeepSource Code Review

We reviewed changes in 2a2d8c1...911bb43 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

PR Report Card

Overall Grade	Security Reliability Complexity Hygiene

Code Review Summary

Analyzer	Status	Updated (UTC)	Details
PHP		Sep 17, 2026 6:02a.m.	Review ↗
JavaScript		Sep 17, 2026 6:02a.m.	Review ↗

Important

AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.

franky-the-going-merry

Request Changes.

The collision itself is real and correctly diagnosed — FrmAppHelper::check_selected() compares with a loose ==, so an option whose value is '' matches an unset field value and renders pre-selected. The '0' test is the right guard to have written; a bare array_filter() here would have silently eaten a legitimate zero option, and array_values() keeps the reindex from colliding with the other_* string keys merged in at :387. Two blocking items.

Blocking

PHPCS is red, and all three errors are PR-introduced and auto-fixable ([x]) — FrmFieldsController.php:414, test_FrmFieldsController.php:59 and :96. phpcbf clears all three.
The filter runs for every bulk-edit type, but a leading blank option is legitimate and renderer-supported on select. Opening Bulk Edit on such a dropdown and saving now silently drops it, even with no other edit. FrmFieldsController.php:354

Non-blocking

remove_blank_separated_values() only inspects the value half, so a |value line survives as an option with an empty label. FrmFieldsController.php:429
Both tests call the new private helpers directly, so import_options()'s own wiring — the ordering against the label|value split, and the interaction with the other_* merge — is still uncovered. test_FrmFieldsController.php:42

CI otherwise green (PHPUnit on PHP 7.4 and 8, PHP CS Fixer, Rector, ESLint, Oxlint, Stylelint, both syntax legs). Branch is current with master (0 behind). Source review only — no visual surface in the diff itself, though finding 2 is about rendered output and is source-derived rather than browser-confirmed ([Likely], from dropdown-field.php and add_placeholder_to_select()).

franky-the-going-merry · 2026-09-16T20:25:09Z

+	private static function parse_bulk_edit_opts( $opts ) {
+		$opts = array_map( 'trim', explode( "\n", $opts ) );
+
+		return array_values( array_filter( $opts, 'strlen' ) );


Blocking: PHPCS is red, and all three errors come from this diff.

classes/controllers/FrmFieldsController.php 414 | ERROR | [x] Unnecessary blank line in short function with only 2 ... tests/phpunit/fields/test_FrmFieldsController.php 59 | ERROR | [x] Missing docblock with @param tags (detected types from call ... 96 | ERROR | [x] Missing docblock with @param tags (detected types from call ...

:414 is the blank line between the array_map() assignment and the return in parse_bulk_edit_opts(). :59 and :96 are the two new private test helpers. All three are marked [x], so phpcbf fixes them — but confirm what it does to :414 rather than accepting it blind, since collapsing that function is a readability call, not just whitespace.

Worth noting the repo's PHPCS job lints the whole tree, so a red check here doesn't always mean the diff — this time it does. Nothing else in the file or the suite is flagged.

Fixed. Also: parse_bulk_edit_opts() grew past the short-function threshold once it took the select-type branch, so PHPCS no longer flags the blank line at all on the current diff (confirmed clean, bare phpcs, no --standard override).

franky-the-going-merry · 2026-09-16T20:25:09Z

 		$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 );


Blocking: this drops blank options for every bulk-edit type, but the defect is specific to the types where a blank option is never wanted. On select a leading blank option is a supported choice, not a mistake.

classes/views/frm-fields/front-end/dropdown-field.php:71:

// phpcs:ignore Universal.Operators.StrictComparisons if ( $placeholder && $opt == '' && ! $skipped ) { $skipped = true; continue; }

The blank option is skipped only when a placeholder is set — and it's skipped precisely because add_placeholder_to_select() (FrmFieldsController:920) already emitted its own <option value=""> and the renderer is avoiding a duplicate. With no placeholder configured, add_placeholder_to_select() returns false without emitting anything, the guard above doesn't fire, and :92 renders the blank option deliberately:

echo esc_html( $opt === '' ? ' ' : $opt );

That is the "nothing selected yet" entry on a non-required dropdown with no placeholder. Without it the browser auto-selects the first real option, which is the opposite of what the field author wanted. For a radio or checkbox group there is no equivalent use — a blank choice there is exactly the check_selected() collision this PR is fixing.

The regression doesn't need anyone to type a blank line, either. The Bulk Edit textarea is populated from the field's existing options, so opening Bulk Edit on a dropdown that already has a leading blank option and saving with no changes now silently removes it.

Scope the filtering to where the collision is actually a defect. $bulk_edit_types is already resolved a few lines up at :345, so the type is in hand:

Suggested change

$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 );

{T}{T}$opts = FrmAppHelper::get_param( 'opts', '', 'post', 'wp_kses_post' );

{T}{T}$opts = self::parse_bulk_edit_opts( $opts, $field->type );

with parse_bulk_edit_opts() keeping a single leading blank for select and dropping blanks everywhere else. If you'd rather not branch on type, the alternative is to drop only duplicate and trailing blanks and keep at most one leading blank — but the type check is the more honest statement of the rule.

Either way this needs saying in the PR description: as written it's a behavior change for dropdowns, not only a fix.

Fixed as suggested: parse_bulk_edit_opts() now takes the field type and keeps a single leading blank for select only, dropping elsewhere. Added regression tests (select keeps it, radio drops it), both confirmed red against the prior code. Noted the dropdown behavior change in the PR description.

franky-the-going-merry · 2026-09-16T20:25:09Z

+	private static function remove_blank_separated_values( $opts ) {
+		return array_values(
+			array_filter(
+				$opts,
+				function ( $opt ) {
+					return ! is_array( $opt ) || '' !== $opt['value'];
+				}
+			)
+		);
+	}


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.

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.

franky-the-going-merry · 2026-09-16T20:25:09Z

+	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' ), $opts );
+	}


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.

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.

codecov · 2026-09-17T05:45:50Z

Codecov Report

❌ Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 28.54%. Comparing base (d0ffb93) to head (9c598b9).
⚠️ Report is 813 commits behind head on master.

Files with missing lines	Patch %	Lines
classes/controllers/FrmFieldsController.php	85.71%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3345      +/-   ##
============================================
+ Coverage     28.36%   28.54%   +0.17%     
- Complexity     9806     9847      +41     
============================================
  Files           160      160              
  Lines         32921    33034     +113     
============================================
+ Hits           9339     9428      +89     
- Misses        23582    23606      +24

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…alues, 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.

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

vivi-the-going-merry · 2026-09-17T06:02:49Z

Method: in-place push
Pushed to: #3345 (branch fix/issue-3385-bulk-edit-blank-option-selected)

deepsource-io · 2026-09-17T06:04:13Z

+		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 ) );


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.

deepsource-io · 2026-09-17T06:04:13Z

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


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.

deepsource-io · 2026-09-17T06:04:13Z

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


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.

vivi-the-going-merry Bot added 3 commits September 16, 2026 13:33

Add regression test for blank bulk-edit options

b8370b7

Drop blank lines when parsing Bulk Edit Options

c578816

Also drop blank separate-value halves; simplify and reindex parse_bul…

9c598b9

…k_edit_opts

vivi-the-going-merry Bot added franky-review run analysis run tests labels Sep 16, 2026

franky-the-going-merry Bot requested changes Sep 16, 2026

View reviewed changes

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

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

vivi-the-going-merry Bot added 2 commits September 16, 2026 23:56

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

vivi-the-going-merry Bot removed the vivi-working Vivi is actively working this label Sep 17, 2026

deepsource-io Bot reviewed Sep 17, 2026

View reviewed changes

Crabcyborg added the priority: low label Sep 17, 2026

Conversation

vivi-the-going-merry Bot commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was broken

What changed

How it was verified

Uh oh!

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

Uh oh!

Review skipped

Uh oh!

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

Uh oh!

DeepSource Code Review

PR Report Card

Code Review Summary

Uh oh!

franky-the-going-merry Bot left a comment

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Sep 17, 2026

Codecov Report

Uh oh!

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

Uh oh!

deepsource-io Bot Sep 17, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldsAjax::assertSame()

Uh oh!

deepsource-io Bot Sep 17, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldsAjax::assertSame()

Uh oh!

deepsource-io Bot Sep 17, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldsAjax::assertSame()

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vivi-the-going-merry Bot commented Sep 16, 2026 •

edited

Loading

coderabbitai Bot commented Sep 16, 2026 •

edited

Loading

deepsource-io Bot commented Sep 16, 2026 •

edited

Loading