#2566 - Ability To Create Custom Geocoded Location Fields

[kodinkat]

• fixes: Ability to create custom geocoded location fields. #2566

---

--- --- --- ---

[github-actions]

Code Review

High

Silent data corruption in delete_location_grid_meta when custom and built-in fields share the same lat/lng

Files: dt-mapping/location-grid-meta.php (the grid_meta_id delete case) and dt-posts/posts.php (individual-delete path).

The PR introduces a new early-return path in add_location_grid_meta that, when a post already has a dt_location_grid_meta row for a given lat/lng, links that existing grid_meta_id to the new field's postmeta key:

// new code added by PR
if ( \! in_array( (int) $grid_meta_id, $ids, true ) ) {
    add_post_meta( $post_id, $location_meta_post_key, $grid_meta_id );
}
return $grid_meta_id;

This means that if a post has London in the built-in location_grid_meta field (grid_meta_id=5) and the user then adds London to a custom my_address field (same coordinates), both fields now share row 5 in dt_location_grid_meta.

When the user later removes London from my_address, the call becomes:

Location_Grid_Meta::delete_location_grid_meta( $post_id, 'grid_meta_id', 5, $existing_post, 'my_address' );

Inside that function (as updated by the PR):

$wpdb->delete( $wpdb->dt_location_grid_meta, [ 'grid_meta_id' => 5 ] ); // shared row deleted
delete_post_meta( $post_id, 'location_grid_meta', 5 );   // built-in field pointer removed\!
delete_post_meta( $post_id, 'my_address', 5 );           // correct

London disappears from the built-in location_grid_meta field and the DB row is gone, even though the user only removed it from my_address. The post_references_grid_meta_id orphan guard was added to the force_values path but not to the individual delete: true path.

Two changes are needed in the grid_meta_id delete case of delete_location_grid_meta:

1. Before deleting the DB row, apply the same post_references_grid_meta_id check (or equivalent) to confirm no other field still references it.
2. Only call delete_post_meta( $post_id, $location_meta_field_key, $value ) — drop the unconditional delete_post_meta( $post_id, 'location_grid_meta', $value ) when $location_meta_field_key \!== 'location_grid_meta'.

---

Medium

post_references_grid_meta_id guard only wired to force_values, not individual deletes

dt-posts/posts.php — individual delete path at the delete: true branch inside update_location_grid_fields.

The new private helper correctly prevents orphaning during force_values, but the per-item delete path is not protected. Removing a location entry one at a time triggers delete_location_grid_meta directly without first checking whether the shared DB row is still referenced by another field. This is the same root cause as the High finding; even after fixing the postmeta cleanup, the DB row itself should not be deleted while another field holds a reference.

---

Overall: The feature intent is sound and the force_values path is handled correctly, but the individual-delete path in delete_location_grid_meta was not updated to account for the new cross-field row-sharing behaviour introduced by the same PR, creating a silent data-loss path that should be resolved before merging.

[github-actions]

Code Review

Note: the existing automated review raised a High about individual-delete not passing $field_settings, and a Medium about the guard being missing from per-item deletes. Both findings are factually incorrect — the diff clearly updates the individual-delete call site to pass $field_key and $field_settings (line ~1966 in dt-posts/posts.php), and the post_references_grid_meta_id guard fires for both the force_values and individual-delete paths. Setting those aside, here are the genuine issues.

---

Medium

1. Activity-message translation strings change their msgid, orphaning existing translations

dt-posts/posts.php lines 568–571 and 581–588 (current file, both location and location_meta blocks).

The PR replaces:

_x( '%1$s removed from locations', 'Location1 removed from locations', 'disciple_tools' )
_x( '%1$s added to locations',   'Location1 added to locations',   'disciple_tools' )

with:

_x( '%1$s removed from %2$s', 'Location name removed from field label ...', 'disciple_tools' )
_x( '%1$s added to %2$s',   'Location name added to field label ...',   'disciple_tools' )

Because gettext uses the msgid (the first argument to _x) as the translation key, all existing translations of the old strings become unused. Any site already translated into a non-English language will silently fall back to English for these activity log entries until translators update the .po files.

If these strings appear in the official disciple_tools translation catalogue, new translation work will be needed before shipping. Coordinate with translators or open an issue to track the re-translation work.

---

2. delete_post_meta( $post_id, 'location_grid_meta', $grid_meta_id ) always runs when field_settings is omitted, making the function a footgun for future callers

dt-mapping/location-grid-meta.php, tail of the grid_meta_id switch case.

When $location_meta_field_settings is null (the default), the post_references_grid_meta_id guard is entirely skipped, and the hardcoded cleanup runs unconditionally:

delete_post_meta( $post_id, 'location_grid_meta', $grid_meta_id );

If a future caller passes a custom field key ($location_meta_field_key = 'my_address') without field_settings — e.g. a plugin using the 5-arg form — this line will silently remove the built-in location_grid_meta postmeta entry even though the caller only intended to remove from their custom field. All current PR callers correctly pass $field_settings, so there is no bug today, but the API is easy to misuse.

Suggested fix: guard the final delete_post_meta so it only runs when $location_meta_field_key === 'location_grid_meta', or add a doc-comment warning that the 5th and 6th parameters must be used together.

---

Summary

The core architecture of sharing a single dt_location_grid_meta row between multiple fields while protecting against cross-field deletion is sound, and both the force_values and individual-delete paths correctly pass field_settings to the guard. The main pre-merge concern is the translation-key break (finding 1), which affects all existing non-English deployments; the footgun risk (finding 2) is lower priority but worth a note in the docblock.

[kodinkat]

@corsacca review ready....