Justin's agent here (Claude Code), reviewing on behalf of @jtsternberg.

PR #1574: Fix repeatable fields dropping 0 values on save

Head 65d78ab, base develop. Reviewed in a worktree (cmb2-pr-1574) with its own wp-env (port 2634, PHP 8.3, WP latest).

Briefing

• Where it comes from: the same bug class as #1535 (fixed by #1572), but in the repeatable save path. No issue was filed.
• Root cause: CMB2_Field::sanitization_cb() strips empty repeatable entries with a callback-less array_filter(), which also drops 0/'0'. CMB2_Sanitize::_check_repeat() drops them a second time with a truthiness check. The diagnosis is correct for the save path.
• The fix: use CMB2_Utils::filter_empty() / notempty() in those two places and in the text_money repeatable loop, and add convention entry C14.
• Trade-offs: a '0' entry now reaches each type's sanitizer, the custom sanitization_cb, and cmb2_sanitize_{$type}. Some types turn it into a junk entry (false, '', 0). The render path still drops 0, so in the admin UI the saved value disappears again on the next save.

Verdict: REQUEST CHANGES

One blocker (F1). The PR's own repro ("add rows a, 0, b, save, reload") still shows only a and b after the fix. The 0 is in the database, but the next save of the post deletes it. The other findings are should-fix and nit items that can land in the same pass.

Findings

F1 — Scope/correctness · High · blocking — includes/CMB2_Types.php:358, :364

CMB2_Types::repeatable_rows() builds the edit-screen rows with two callback-less array_filter() calls, so a stored '0' row is never rendered. The fix therefore stores the 0 only until someone next saves the post through the admin UI, and that save removes it. Programmatic and REST writes keep the 0, but the admin round trip still loses it. The description's repro is not fixed.

Evidence: probe Test_Probe_1574::test_probe_render_round_trip (save, render, re-submit the rendered rows):

[PROBE] stored after save 1: array ( 0 => 'a', 1 => '0', 2 => 'b', )
[PROBE] rows rendered on edit screen: array ( 0 => 'a', 1 => 'b', )
[PROBE] stored after re-saving rendered form: array ( 0 => 'a', 1 => 'b', )
1) Test_Probe_1574::test_probe_render_round_trip — Failed asserting that an array contains '0'.

This affects repeatable sub-fields in groups too, because they render through the same method. The repeatable text_date_timestamp epoch-0 row (F4) is also hidden by it.

Recommendation: switch both filters in repeatable_rows() to CMB2_Utils::filter_empty(). Add a regression test that saves array( 'a', '0', 'b' ), renders the field, and asserts that a value="0" row is present outside the hidden empty-row. Note: repeatable_rows() probably needs the same "only entry is '0'" decision as F2 (today array( '0' ) renders as one default row).

F2 — Design · Medium · should-fix — CONVENTIONS.md (C14 canonical examples), includes/CMB2_Sanitize.php:125, :616

C14 and the PR description say "a repeatable field whose only entry is '0' is still removed by _is_empty_array()". That is true only for types that go through default_sanitization() (text, select, hidden, …). Types with their own sanitizer never reach that check, so with this PR alone a '0'-only field behaves differently by type:

[PROBE] ['0'] only, text:                exists=false: ''
[PROBE] ['0'] only, select:              exists=false: ''
[PROBE] ['0'] only, textarea:            exists=true: array ( 0 => '0', )
[PROBE] ['0'] only, textarea_code:       exists=true: array ( 0 => '0', )
[PROBE] ['0'] only, text_money:          exists=true: array ( 0 => '0.00', )
[PROBE] ['0'] only, text_date_timestamp: exists=true: array ( 0 => 0, )
[PROBE] ['0'] only, text_url:            exists=true: array ( 0 => false, )
[PROBE] ['0'] only, text_email:          exists=true: array ( 0 => '', )

On develop every one of these is exists=false. #1572 changes _is_empty_array() to filter_empty(), which removes the split.

Recommendation: #1572 is now merged into develop, so rebase onto it — that removes the split. Then correct the C14 sentence so it describes what the code actually does, and add a zero-only repeatable test (array( '0' ) → array( '0' )).

F3 — Correctness · Medium · should-fix / question — includes/CMB2_Sanitize.php:259

text_money keeps the scalar early return if ( ! $this->value ) return '';. With this PR, a non-repeatable '0' money field is removed while a repeatable '0' entry is saved as '0.00':

[PROBE] non-repeatable money "0" metadata_exists: false
[PROBE] repeatable money ["0"] stored: array ( 0 => '0.00', )

That contradicts C14's "Not staged" reasoning that "non-repeatable fields already keep it". For text_money they don't. History: jrfnl's 2167d89 (PR #577) guarded only '' === $this->value, to stop empty money fields being saved as 0.00. The later commit 29e3248 (@jtsternberg, "A little cleanup and give props") widened that guard to truthiness, which also catches '0'. Neither the commit nor PR #577 gives a reason to drop 0.

Question for the author: should scalar money become CMB2_Utils::isempty(), which keeps jrfnl's empty guard and saves '0' as '0.00'? Or should repeatable money match the scalar behavior? Either choice is defensible, but the two paths shouldn't disagree. Whatever is chosen, fix the C14 claim.

F4 — Back-compat · Low · should-fix — CONVENTIONS.md C14 "Blast radius" / missing CHANGELOG

Who now sees a '0' that they didn't see before (confirmed by probe on head vs develop):

• Custom sanitization_cb and cmb2_sanitize_text now receive array( 0=>'a', 1=>'0', 2=>'b' ) instead of array( 0=>'a', 2=>'b' ).
• sanitization_cb => false now stores array( 'a', '0', 'b' ).
• Stored data shape: junk entries now appear where nothing used to be. Repeatable text_url gets false, text_email gets '', and text_date_timestamp gets 0 (epoch). For example [ 'http://a.test', false ]. Anything that reads the data directly sees them: get_post_meta() callers, REST output, and the metadata_exists() result for '0'-only fields. C14 lists these, but the CHANGELOG has to say so too.
• Not affected: hook firing counts and database write counts. save_field() makes the same single update_data() / remove_data() call.

A claim from an earlier private review, that this PR moved the filter ahead of those seams, is refuted: The filter already ran before sanitization_cb, the false path, and cmb2_sanitize_{$type} on develop (array_filter sits in the same place, from 106f91e in 2014). Only which entries survive it changed. The contract half of the claim holds.

C3: Justin confirmed this doesn't need staging. It restores data the user typed, and the observers are filters, not a stored-value reinterpretation. But C14's "No configuration relies on a saved 0 vanishing" can't be verified. Reword it to the actual argument: silent data loss, plus the group precedent in dba5776.

Recommendation: add the CHANGELOG entry before merge (the author deferred it until #1572 landed; it has now). It should name the three sanitizer seams that now receive '0'. Also consider whether a repeatable text_url/text_email should drop entries that sanitize to false/'' instead of storing them.

F5 — Testing · Medium · should-fix — tests/test-cmb-field.php:302-336

The PR's tests are real: all three fail with includes/ reverted to develop. With only CMB2_Sanitize.php reverted, two fail (textarea_code and text_money). The description mentions only textarea_code. Coverage that's missing:

• Render round trip (F1).
• Empty-only repeatable money. Probe: array( '', '' ) → metadata_exists false, so 2167d89's fence holds. Pin it with a test next to the explicit-zero money test.
• sanitization_cb => false and a custom callback receiving '0' (F4).
• Datetime: a repeatable text_datetime_timestamp epoch-0 row is now stored as 0. My probe got array( 0 => 0, 1 => 1577959200 ); on develop that row is dropped. No test covers it.

The fcb112c repeatable timezone-in-group fence is still covered by its own Test_CMB2_Types::test_save_group, which passes on head.

F6 — Clarity · nit — CONVENTIONS.md:415

#1571 also adds ## C14 (taxonomy value_field). Whichever PR lands second needs to be renumbered to C15.

F7 — Clarity · nit — includes/CMB2_Sanitize.php:275

After filter_empty() runs upstream, the notempty( $val ) guard in the money loop only matters for callers that build CMB2_Sanitize directly. That's fine to keep. No change needed unless you want to note it inline.

Fences checked

• array_filter( $meta_value ) in sanitization_cb() → 106f91e (2014, original tables-to-classes conversion). Never revisited for 0: 7e521ed (#135) moved the scalar checks to is_null, and dba5776 (2.2.2) fixed groups only. Changed. Its empty-removal need is kept.
• if ( $this->value ) / ! empty( $val ) in _check_repeat() → fcb112c (#330: repeatable datetime saving the current time). It exists to skip blank rows and failed parses (''/false). Losing 0 was collateral: I checked the commit message, the diff, and the #330 body, and none of them mention zero. Changed. The skip for blank rows and failed parses is kept. The UTC sidecar stays aligned, because a '0' entry returns '' before $utc_values is pushed.
• if ( $val ) in the text_money repeatable loop → 2167d89 (jrfnl, PR #577, '' === $val), widened to truthiness in 29e3248 (cleanup). Changed back toward the original intent. The empty guard is kept (probe: ['',''] stores nothing).
• if ( ! $this->value ) return ''; scalar money early return → same pair of commits. Kept, which creates the F3 inconsistency.
• array_filter() in CMB2_Types::repeatable_rows() → 934078f / 4d11c11 (2014, and older code before that). It skips blank rows and falls back to default. Not touched (F1).
• _is_empty_array()'s array_filter → 2.0.6 era. Not touched. #1572 changes it (F2).

Not covered: PHP 7.4 wasn't run locally (C7). I read the diff by eye and it uses no syntax newer than 5.x. Full suite on head: OK (331 tests, 1076 assertions).

Justin's agent here (Claude Code), reviewing on behalf of @jtsternberg.