FIX: prevent clearing stale mapped fields in Insert Post action https://github.com/Crocoblock/issues-tracker/issues/19370

[ObitoTM]

No description provided.

[github-actions]

🤖 AI PR Review

Risk level: medium

Review

Summary

• The PR changes how meta box fields are collected for the Insert Post action in modules/actions-v2/insert-post/traits/process-meta-boxes-trait.php. Instead of using the raw $modifier->fields_map, it builds a filtered map of only currently submitted/parsed fields and then filters meta box fields by present values to avoid clearing fields that are no longer present in the current form submission.

What I like

• The change addresses a real bug: mapped fields that are not present in a submission may be unintentionally cleared by JetEngine. Restricting the set of meta fields to those present in the current request/parsers is the correct approach for preventing stale-field clearing, and the approach is efficient (array_intersect_key).

Files / functions touched

• modules/actions-v2/insert-post/traits/process-meta-boxes-trait.php
  • get_meta_box_fields()
  • new: get_current_fields_map()
  • new: filter_fields_map_by_field_names()
  • new: filter_meta_box_fields_by_values()

Security

• No direct security issues introduced (no new DB queries or output rendering). However, incoming keys are still coming from user data (request & parser names) and are used as array keys. While not a direct vulnerability, it is good practice to normalize/sanitize field keys (e.g. sanitize_key) to avoid surprises or different behavior for odd input keys.

Backward compatibility / behavioural concerns

• Behavior change: Previously get_meta_box_fields() used the full $modifier->fields_map. Now it only uses fields that appear in the current request or in parser names. This is the desired fix for the bug, but it changes behavior for any legitimate usage where a mapping is expected to run even when no corresponding request values exist. Examples to consider:
  • Programmatic calls where $this->value is not an array (the new filter_meta_box_fields_by_values() returns an empty array when $this->value is not an array). That will skip all meta box fields and may change the outcome in edge cases. Recommend adding a comment clarifying intent and/or handling cases explicitly.
  • Integrations that provide data through other channels may rely on the full map. The code attempts to include parser names (iterate_parsers_list(false)), which mitigates some cases, but please double-check all parser sources are covered.

Potential runtime warnings / robustness

• $modifier->fields_map is used directly and assumed to be an array. If for any reason fields_map is not set or not an array, this can trigger warnings. Suggest casting to (array) or checking is_array before passing into filter_fields_map_by_field_names.
• When building the list of field names you append parser->get_name() values. Ensure get_name() always exists and returns a string. Consider normalizing these names (sanitize_key) before using them as array keys.

Performance

• The introduced filtering uses array_intersect_key and array_fill_keys which are O(n) and efficient. No heavy DB operations are introduced. This is performant for large forms.

Multistep forms / conditional logic

• This change is beneficial for multi-step forms: fields absent in the current step will not be considered for clearing. Good intended effect.

Test coverage

• I do not see tests accompanying this change. Please add unit/integration tests covering at least these cases:
  1. A mapped meta field that is not present in the current request should NOT be cleared after processing.
  2. Parser-only fields are still included when appropriate (i.e. iterate_parsers_list entries are considered).
  3. Cases where $modifier->fields_map is missing or not an array should not trigger warnings and should behave gracefully.

Recommendations / small code improvements

• In get_current_fields_map(): cast fields_map to array to avoid warnings: $fields_map = (array) ($modifier->fields_map ?? []);
• Sanitize/normalize keys used as array keys: use sanitize_key() on $field_names before array_fill_keys(). This prevents weird keys from breaking array behavior.
• In filter_meta_box_fields_by_values(): add a brief comment explaining why an empty array is returned when $this->value is not an array. Consider the alternative of returning the original $meta_box_fields if you want to preserve legacy behavior for non-form contexts — but returning empty is probably the safer default to avoid accidental clearing; just document the choice and add a test.
• Add defensive checks that iterate_parsers_list(false) returns iterable and that parser->get_name() is string-compatible.

Minor code-style notes

• Keep code formatted per WPCS (spacing/PHPCS should be fine). Consider adding type hints where possible and PHPDoc for the new methods for clarity.

Summary of required follow-ups before merge

• Add unit/integration tests for the behavior changes (see above).
• Harden the code against non-array fields_map and unexpected parser names (cast arrays, sanitize_key on field names).
• Add a short inline comment in filter_meta_box_fields_by_values() explaining the choice for returning an empty array when $this->value is not an array.

Overall

• Conceptually correct fix for the reported issue (prevent clearing stale mapped fields). With the small hardening changes, documentation of the behaviour, and tests added this can be merged.

Suggested changelog entry

- FIX: prevent clearing stale mapped fields in Insert Post action