fix(review): land cleanCoordinate server-side guard; surface geocode failures

Two code-review follow-ups.

cleanCoordinate() — the server-side coordinate sanitizer the design doc and
plan both describe as already shipped — had never actually been committed; it
existed only as uncommitted work in another checkout, so this branch had no
server-side validation of lat/lng at all (the blueprint fields are plain
`type: text` with no `validate:` key). Landing it here makes the spec's stated
safety net real. Also corrected its onAdminSave comment, which justified that
hook by saying the public form's lat/lng inputs are CSS-hidden and GPS-filled
— true before this feature, inverted by it. Both hooks are needed: this branch
makes /post the primary hand-entry path, not Admin2.

The geocode lookup swallowed every failure and reset the button, leaving the
DOM byte-identical to the pre-click state — a traveller on flaky mobile data
could not distinguish a failed lookup from a broken button. It now shows a
distinct hint, and checks r.ok first so a 4xx/5xx body no longer parses as
"no results" and tells the traveller their city does not exist. R8's actual
guarantee (fields untouched on failure) is preserved.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
2026-07-24 21:48:20 +02:00
co-authored by Claude Opus 5
parent a5993b2091
commit e873a9cb23
3 changed files with 127 additions and 30 deletions
+85
View File
@@ -3,6 +3,7 @@ namespace Grav\Plugin;
use Grav\Common\Cache;
use Grav\Common\Data\ValidationException;
use Grav\Common\Page\Interfaces\PageInterface;
use Grav\Common\Plugin;
use RocketTheme\Toolbox\Event\Event;
@@ -43,6 +44,16 @@ class CacheOnSavePlugin extends Plugin
// (priority 0) has created the page and copied the uploaded files —
// we reorder those files, then clear the page-tree cache.
'onFormProcessed' => ['onFormProcessed', -100],
// Fired by the api plugin (PagesController) on every Admin2-driven
// create/update/translate, right before $page->save().
// onFormValidationProcessed (above) does not fire for Admin2 saves,
// so BOTH hooks are needed to cover every path lat/lng can arrive
// through: this one for Admin2/API, that one for the public /post
// form. (Until the location-override panel shipped, the public
// form's lat/lng inputs were CSS-hidden and GPS-filled, making
// Admin2 the only realistic hand-entry route. They are now visible
// and directly editable, so the /post path is the primary one.)
'onAdminSave' => ['onAdminSave', 0],
];
}
@@ -74,6 +85,7 @@ class CacheOnSavePlugin extends Plugin
}
$form->setData('parent', $this->resolveDailiesParent($activeTrip));
$this->sanitizeCoordinates($form);
// One shared /post form drives both create and edit (KTD1). add-page-by-form
// reads overwrite_mode from the /post page header's pageconfig (not form
@@ -149,6 +161,79 @@ class CacheOnSavePlugin extends Plugin
return '/' . $trip . '/dailies';
}
/**
* Strip invisible Unicode formatting characters (bidi marks, zero-width
* joiners, etc. — Unicode category "Cf") from a pasted lat/lng value, then
* validate the result is a real coordinate.
*
* Root cause this guards against: coordinates copied from a maps app can
* carry an invisible mark (e.g. U+200E LEFT-TO-RIGHT MARK) that neither
* JS `.trim()` nor PHP's numeric-string coercion strip. Twig's
* `number_format` filter then silently float-casts the corrupted string
* to 0.0 (PHP does not raise a warning), placing the entry at Null Island
* instead of failing loudly.
*
* Coordinates are optional (some entries intentionally have none), so a
* blank value cleans to '' with no error — only a non-blank value that
* still fails to parse as an in-range decimal after cleaning is rejected.
*
* @return string the cleaned value to write back
* @throws ValidationException if non-blank but still invalid after cleaning
*/
private function cleanCoordinate(string $field, string $raw, float $bound): string
{
$clean = trim(preg_replace('/\p{Cf}/u', '', $raw) ?? $raw);
if ($clean === '') {
return $clean;
}
if (!is_numeric($clean) || abs((float) $clean) > $bound) {
throw new ValidationException(sprintf(
'%s "%s" is not a valid coordinate — check for stray characters from pasting.',
$field === 'lat' ? 'Latitude' : 'Longitude',
$raw
));
}
return $clean;
}
/** Post-form entry point (see cleanCoordinate) — 'lat'/'lng' as top-level form fields. */
private function sanitizeCoordinates($form): void
{
foreach (['lat' => 90.0, 'lng' => 180.0] as $field => $bound) {
$raw = $form->value($field);
if (!is_string($raw)) {
continue;
}
$form->setData($field, $this->cleanCoordinate($field, $raw, $bound));
}
}
/**
* Admin2/API entry point (see cleanCoordinate) — 'header.lat'/'header.lng'
* on the Page object the api plugin is about to save. Fires on every
* create/update/translate (PagesController::create/update/translatePage),
* so this also re-validates already-clean values on every subsequent edit
* — harmless, since a clean value round-trips unchanged.
*/
public function onAdminSave(Event $event): void
{
$page = $event['page'] ?? $event['object'] ?? null;
if (!$page instanceof PageInterface) {
return;
}
$header = $page->header();
if (!$header) {
return;
}
foreach (['lat' => 90.0, 'lng' => 180.0] as $field => $bound) {
$raw = $header->{$field} ?? null;
if (!is_string($raw)) {
continue; // unset, null, or already a native number — nothing to clean
}
$header->{$field} = $this->cleanCoordinate($field, $raw, $bound);
}
}
/**
* The photo order the user arranged in the form, sent explicitly by
* post-form.js as a JSON array of filenames in the dedicated
File diff suppressed because one or more lines are too long
+15 -3
View File
@@ -719,7 +719,14 @@ function initLocationDetails() {
// button stuck disabled on "Searching…" forever.
var controller = new AbortController();
var timeoutId = setTimeout(function () { controller.abort(); }, 10000);
fetch(url, { signal: controller.signal }).then(function (r) { return r.json(); }).then(function (data) {
fetch(url, { signal: controller.signal }).then(function (r) {
// Without this a 4xx/5xx body (rate limit, upstream error) parses as
// JSON with no `results` key and the traveller is told their city
// does not exist — sending them off to hunt a spelling mistake that
// isn't there. Route real failures to the catch instead.
if (!r.ok) throw new Error('geocode http ' + r.status);
return r.json();
}).then(function (data) {
var list = (data && data.results) || [];
if (!list.length) {
setHint('No matches — try adding a country, or drag the pin on the map.');
@@ -746,8 +753,13 @@ function initLocationDetails() {
}
showResults(list);
}).catch(function () {
// R8: network failure (including our own timeout abort) degrades
// silently — fields untouched.
// R8 revised: the fields stay untouched on failure (that part of R8
// is the actual guarantee), but the failure is no longer invisible.
// Silence left the DOM byte-identical to the pre-click state — empty
// hint, enabled button — so a traveller on flaky mobile data could
// not tell "lookup failed" from "the button is broken". Distinct
// from the no-match message above, which means the service answered.
setHint('Couldnt reach the lookup service — check your connection and try again, or drag the pin on the map.');
}).then(function () {
clearTimeout(timeoutId);
lookupBtn.disabled = false;