Fix custom settings not available in the builder after import

[huongcodecb1992]

No description provided.

[opencode-agent]

Code Review Summary

Code Quality & Best Practices

Positives:

• Fluent interface pattern (return $this) ✅
• Safe fallback with class_exists() check ✅
• Strict comparisons (===, in_array with true) ✅
• Clever use of $original_keys captured before transformations ✅

Issues:

1. Type coercion bug (line 349): (string) cast loses type for numbers, booleans, and arrays - breaks field values that need their original type
2. No class constant: custom_settings string should be a constant

Potential Bugs & Edge Cases

Issue	Severity	Description
Type loss	High	(string) conversion breaks numeric fields, booleans, and nested arrays
Duplicate handling	Low	Pre-existing custom_settings in input is preserved (line 337), but new entries use uniqid() which could cause key collisions
$original_keys stale reference	Low	Captured at unparse start, but if earlier methods add new keys they won't be considered "custom"

Performance

• Minor: array_keys() and uniqid() per field - acceptable
• O(n × m) where n=settings, m=native keys - acceptable for typical field sizes

Security

No concerns - internal data reorganization only.

Critical Fix Needed

Line 349 must preserve original types:

$value = $this->settings[ $key ];
if ( is_array( $value ) ) {
    $value = wp_json_encode( $value );
} elseif ( is_bool( $value ) ) {
    $value = $value ? 'true' : 'false';
} elseif ( is_scalar( $value ) ) {
    $value = (string) $value;
}

Verdict

The core logic is sound. The type coercion bug is the main issue to fix before merging.

opencode session  |  github run

[opencode-agent]

Code Review Issues

Critical Bugs

1. Type Coercion Loss (lines 366, 371)

• (string) cast converts booleans incorrectly: false → "", true → "1"
• Numeric values like 0 become "0" which is correct, but edge cases exist
• This breaks boolean fields imported into the builder

2. Closure Redefinition (lines 349-360)

• $flatten_array closure is redefined on every iteration of the foreach loop (line 339)
• Should be defined once outside the loop for performance

Medium Issues

3. Redundant JSON+Cast (lines 364-369)

• Arrays are both flattened with (string) cast AND JSON-encoded
• Both versions are added to $values_to_add (lines 366, 369)
• Creates duplicate entries for nested keys

4. Duplicate Entry Generation (lines 374-382)

• Each key produces multiple entries in custom_settings due to the flat + full array handling
• Could cause data duplication in the builder UI

Low Issues

5. No custom_settings String Constant

• 'custom_settings' appears twice (lines 337, 388) without a constant
• Should be extracted to a class constant for maintainability

6. Nullable Return (line 331)

• Early return return $this; on line 331 while technically correct for fluent interface, is inconsistent with other methods that always return $this at the end

Edge Cases

7. Boolean Arrays

• If array value contains boolean false, line 366 converts it to "" then line 369 JSON-encodes it to "Array"
• The (string) cast on line 366 is redundant for arrays since line 369 overwrites with JSON-encoded version

opencode session  |  github run

[opencode-agent]

Looking at the code changes in src/Unparsers/Field.php (lines 328-374), here are the issues:

Critical Bugs

1. Type Coercion Loss (lines 361-362)

'key'   => (string) $k,
'value' => (string) $v,

• (string) cast on $v converts booleans incorrectly: false → "", true → "1"
• This breaks boolean fields imported into the builder
• Numeric values like 0 become "0" but lose their original type information

2. Duplicate Entry Generation (lines 347-350)

$values_to_add = $this->array_to_dot_notation( (array) $value, $key );
$values_to_add[ $key ] = wp_json_encode( $value );

• For array values, both flattened dot-notation (line 348) AND JSON-encoded version (line 350) are added to $values_to_add
• This creates duplicate entries with different keys (e.g., options.label vs options)
• Line 366 unset() only removes the parent key, not the duplicates

Medium Issues

3. Redundant String Cast (line 362)

• $v already comes as either a dot-notation string (from line 348/352) or JSON string (from line 350)
• The (string) cast is redundant and potentially destructive for edge cases

4. No FieldKeys Dependency Check (line 334)

• Calls \MBB\Helpers\FieldKeys::all() but no graceful fallback if the method doesn't exist
• Will cause fatal error if the helper method is missing

Low Issues

5. No custom_settings String Constant

• 'custom_settings' appears on lines 337 and 370 without extraction to a class constant

6. Potential ID Collision (line 358)

• uniqid() called inside nested loops could theoretically produce collisions with rapid execution

opencode session  |  github run

[opencode-agent]

Based on my review of the code changes in src/Unparsers/Field.php, here are the issues identified:

Critical Issues

1. Missing method existence check (line 334)
The code checks class_exists( '\MBB\Helpers\FieldKeys' ) but doesn't verify that the static method FieldKeys::all() exists. If the class exists but the method doesn't, it will cause a fatal error.

// Current:
$all_keys = \MBB\Helpers\FieldKeys::all();

// Should be:
if ( ! method_exists( '\MBB\Helpers\FieldKeys', 'all' ) ) {
    return $this;
}

Medium Issues

2. No constant for 'custom_settings' string
The string 'custom_settings' appears on lines 337 and 359 without extraction to a class constant. This reduces maintainability.

3. Potential stale reference in $original_keys
$original_keys is captured at line 25 before any unparse transformations. If an earlier unparse method (e.g., unparse_datalist() at line 56) adds NEW keys to settings, those keys won't be in $original_keys and won't be treated as custom settings.

Low Issues

4. Redundant uniqid() calls in nested loop (lines 347-352)
uniqid() is called inside the nested foreach loop. For arrays with many nested values, this could be moved outside the loop for minor performance improvement.

5. Unclear intent of $original_keys capture point
The capture happens before unparse_default_values() which injects defaults. The comment on line 13 explains the rationale, but it's not immediately obvious why earlier capture is needed vs. capturing after defaults are applied.

Not Issues (Based on Code Analysis)

• Type coercion: The array_to_dot_notation() method in Base.php (lines 110-118) already handles booleans (true/false strings), nulls, and non-scalars (JSON encoding) correctly, so no type loss occurs.
• Duplicate entries: The method correctly handles arrays by flattening them to dot notation, and the parent key is not duplicated in custom_settings.

opencode session  |  github run