Drop blank options when using Bulk Edit Options - #3345

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

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:

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.

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.

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.

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.

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.

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.

Blocking — PHPStan is red on this line. strlen(): int, not bool — phpstan-strict-rules (installed, per this repo's CI log) rejects it as an array_filter callback even though PHP's own truthy/falsy coercion makes it work correctly at runtime (strlen('') === 0 → falsy, any non-empty string → truthy, so '0' is correctly kept per the intent noted in the prior review round).

Fixed — replaced the strlen callback with an explicit closure returning bool ('' !== $value), same semantics, '0' still survives.

Blocking — CI: PHPCS is red on this line at the current head (1b77750).

FOUND 1 ERROR AFFECTING 1 LINE
423 | ERROR | [x] Blank lines found between consecutive simple assignments.

It was green at the prior head (911bb43) — the only file that changed between those two commits is this one, and this array_filter block (converting the 'strlen' callback to an explicit closure) is the only edit. phpcbf clears it automatically.

Fixed at fb2c862 (phpcbf-clean, current head) — the assignment block was reformatted along with the other round-3 changes.

Blocking — this only prevents the original bug when the field has a placeholder configured. dropdown-field.php's skip logic:

if ( $placeholder && $opt == '' && ! $skipped ) {
	$skipped = true;
	continue;
}

only swallows the blank option when $placeholder (from FrmFieldsController::add_placeholder_to_select()) is truthy — and that's false whenever the field has no placeholder option set and no default-value-from-name, which is the common case for a plain select with no "Please Select" text. In that case the leading blank option this method now re-inserts renders as a real <option value="">, and FrmAppHelper::check_selected( $field['value'], '' ) loosely matches an unset field value against it — reproducing the exact empty-row/selected-by-default bug this PR (and formidable-pro#3385) is about, for the majority of selects that don't bother configuring a placeholder.

Suggested fix: only set $keep_leading_blank when the field actually has a placeholder configured (mirror add_placeholder_to_select()'s own condition), or handle the no-placeholder case in dropdown-field.php directly. Source-verified only, not exercised in a live browser this round — worth a live check with screen options/no-placeholder select before merging.

Fixed. select_has_placeholder() now mirrors add_placeholder_to_select()'s own condition (both delegate to a shared get_select_placeholder() helper), so the leading blank only survives when a placeholder is actually configured. No-placeholder selects drop it like radio/checkbox. Covered by test_import_options_drops_leading_blank_for_select_without_placeholder and the _with_placeholder counterpart.

Blocking — this drops a select's leading-blank placeholder row entirely when "separate values" is on, unlike the plain-line path above. For a separate-value select where the textarea starts with "|" (blank label, blank value) meant as the placeholder row: parse_bulk_edit_opts()'s keep_leading_blank check only tests '' === $opts[0] (line 423), but at that point $opts[0] is the string "|", not '', so the leading-blank carve-out never triggers for it. After the label/value split (lines 361-372) it becomes ['label' => '', 'value' => ''], which this method then strips unconditionally — it doesn't check $field_type at all, so there's no equivalent carve-out here for select.

Failure scenario: select field, "separate values" checked, textarea = "|\nYes|1\nNo|0" → the intended blank placeholder row disappears instead of surviving as the (skippable) first option, same as the plain-line case now does.

Fixed. remove_blank_separated_values() now takes the same keep_leading_blank flag and keeps a leading blank label/value pair on the same placeholder-configured condition, unless it's the only line. Covered by test_import_options_keeps_leading_blank_pair_for_separate_value_select_with_placeholder.

Non-blocking: before this PR, explode( "\n", rtrim( $opts, "\n" ) ) on an empty textarea always produced [''] — one blank option — so a radio/checkbox field always kept at least one option after Bulk Edit Options, even a blank one. Now array_filter strips it and the leading-blank exception is select-only, so a fully-cleared textarea on a radio/checkbox field saves zero options instead of one. FrmFieldsHelper::show_single_option()'s foreach over an empty options array just renders nothing, no error — silent, not loud. Might be intentional given this PR's whole point is dropping blanks, but worth a one-line confirmation since no docblock mentions the zero-options case specifically.

Confirmed intentional and now explicit: parse_bulk_edit_opts() drops to zero options for a wholly-blank textarea even when keep_leading_blank is true, since a leading blank only makes sense as the first row of a real list. Covered by test_parse_bulk_edit_opts_wholly_blank_keeps_nothing_even_when_flagged.

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.

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.

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.

Blocking — CI: Rector is red on this round's new code. get_select_placeholder() was extracted in fb2c862; Rector's dry-run flags it (CompleteMissingIfElseBracketRector, ReturnEarlyIfVariableRector) and wants the early-return form:

Rector passed at the previous head, so this is PR-introduced.

Call to an undefined method test_FrmFieldsController::assertFalse()

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

Call to an undefined method test_FrmFieldsController::assertSame()

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

Call to an undefined method test_FrmFieldsController::assertSame()

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

Call to an undefined method test_FrmFieldsController::assertSame()

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

Call to an undefined method test_FrmFieldsController::assertTrue()

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

Call to an undefined method test_FrmFieldsController::assertSame()

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

Call to an undefined method test_FrmFieldsController::assertSame()

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

Call to an undefined method test_FrmFieldsController::assertSame()

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

Call to an undefined method test_FrmFieldsController::assertSame()

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

Call to an undefined method test_FrmFieldsController::assertSame()

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