Fix/unparse conditional logic

[huongcodecb1992]

No description provided.

[opencode-agent]

Based on my review of the pull request, here are my findings:

---

Code Review: Fix/unparse conditional logic

Code Quality & Best Practices

Good:

• Well-documented fixes with Vietnamese comments explaining the reasoning
• Clean separation with unparse_custom_settings() method
• Proper use of hash map (accessed_keys) for O(1) lookups

Issues:

1. Incomplete STRUCTURAL_KEYS whitelist (Field.php:229-234)

   • Only 17 keys listed, but Meta Box fields have many more native properties (e.g., label_description, before, after, save_format, query_args, etc.)
   • Any missed native key will be incorrectly classified as "custom"
   • Recommendation: Consider expanding this list or relying solely on accessed_keys tracking

2. Filter signature issue (Field.php:244)

   • apply_filters('mbb_parser_known_field_keys', self::STRUCTURAL_KEYS) - the filter function signature shown in docs (fn($k) => array_merge($k, [...])) is correct, but if someone mistakenly returns false or null, it will break

3. Non-deterministic output (Base.php:148, Field.php:253)

   • uniqid() produces different values on each run, causing unnecessary diff churn in version control
   • Consider using a hash of the content or sequential index instead

---

Potential Bugs & Edge Cases

1. Data loss with break (Base.php:168)

   • Now only the first action ('visible' OR 'hidden') is preserved; if both exist, 'hidden' is lost
   • Verify this matches the original parser behavior and intended UI

2. Text limiter default (Field.php:150-151)

   • Adding ?? 0 default when only limit_type exists may not preserve original intent - if user explicitly wanted no limit, this might change semantics

3. JSON encoding edge cases (Field.php:257-258)

   • wp_json_encode() produces different formatting than original JSON
   • Numeric array keys [0 => 'a', 1 => 'b'] become JSON arrays ["a","b"] losing key information

4. Read-only key tracking (SettingsTrait.php)

   • All accesses (read via __get or write via __set) are tracked equally - a key that's only read but not modified might still be incorrectly moved to custom_settings if not in STRUCTURAL_KEYS

---

Security Considerations

• No direct security concerns found
• wp_json_encode() is used correctly for JSON serialization
• No user input flows into uniqid() in a way that could cause issues

---

Performance

• Good: Uses hash map for O(1) lookups
• Good: array_diff_key is efficient
• Minor: uniqid() has minimal overhead but called many times in loops

---

Summary: The core fixes (conditional logic loop, array-to-CSV conversion, custom settings detection) address real bugs. Main concerns are the fragility of the STRUCTURAL_KEYS whitelist and potential data loss when both 'visible' and 'hidden' conditions exist.

opencode session  |  github run