From 5b4e31678eea04dde0dbde1b076a3e1d96352f02 Mon Sep 17 00:00:00 2001 From: Mischa Date: Wed, 8 Jul 2026 17:06:17 +0200 Subject: [PATCH] refactor(entry-actions): dedup scope guard + guard publish cache-flush Address code-review findings on the trip publish/unpublish toggle: - Extract EntryScopeGuard::resolveChildOf() so resolveActiveDailyChild and resolveTripChild share one find() + parent-route-assert body instead of two copies that could drift (P1 maintainability). - Wrap setTripPublished's post-save cache invalidation in try/catch. save() has already persisted the published flag to disk, so a flush failure now logs a loud reconciliation warning (and still returns success + the audit line) rather than bubbling to a bare 500 that reads as "nothing happened" (P2 reliability / adversarial). Behavior-preserving; PHP lint clean; trip-publish Playwright suite 8/8. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Mpdu3Dt1iVoozHwAMyjrbn --- .../cache-on-save/classes/EntryScopeGuard.php | 51 +++++++++---------- .../classes/EntryActionsApiController.php | 24 +++++++-- 2 files changed, 42 insertions(+), 33 deletions(-) diff --git a/plugins/cache-on-save/classes/EntryScopeGuard.php b/plugins/cache-on-save/classes/EntryScopeGuard.php index 2fa21d5..cabb09f 100644 --- a/plugins/cache-on-save/classes/EntryScopeGuard.php +++ b/plugins/cache-on-save/classes/EntryScopeGuard.php @@ -97,19 +97,17 @@ class EntryScopeGuard } /** - * Resolve a folder segment to the page that is a DIRECT child of the active - * trip's dailies container, or null when the segment is unsafe, no active trip - * is set, the page does not exist, or its parent is not the active dailies. + * Resolve a safe segment to the page that is a DIRECT child of $parentRoute, + * or null when the segment is unsafe, the page does not exist, or its parent + * is not exactly $parentRoute. Resolving via $pages->find() + a parent-route + * assertion (never raw path concatenation) is what closes the traversal hole; + * both public resolvers below share this one body so they cannot drift. */ - public static function resolveActiveDailyChild(Grav $grav, string $segment): ?PageInterface + private static function resolveChildOf(Grav $grav, string $parentRoute, string $segment): ?PageInterface { if (!self::isSafeSegment($segment)) { return null; } - $dailies = self::dailiesRoute($grav); - if ($dailies === null) { - return null; - } $pages = $grav['pages']; // In the API request context the page tree is lazily disabled; enable it // so find() can resolve (mirrors the api plugin's own resolvePageByRoute). @@ -117,17 +115,31 @@ class EntryScopeGuard if (method_exists($pages, 'enablePages')) { $pages->enablePages(); } - $page = $pages->find($dailies . '/' . $segment); + $page = $pages->find($parentRoute . '/' . $segment); if ($page === null) { return null; } $parent = $page->parent(); - if ($parent === null || $parent->route() !== $dailies) { + if ($parent === null || $parent->route() !== $parentRoute) { return null; } return $page; } + /** + * Resolve a folder segment to the page that is a DIRECT child of the active + * trip's dailies container, or null when the segment is unsafe, no active trip + * is set, the page does not exist, or its parent is not the active dailies. + */ + public static function resolveActiveDailyChild(Grav $grav, string $segment): ?PageInterface + { + $dailies = self::dailiesRoute($grav); + if ($dailies === null) { + return null; + } + return self::resolveChildOf($grav, $dailies, $segment); + } + /** * Resolve a slug to the trip page that is a DIRECT child of /trips, or null * when the segment is unsafe, the page does not exist, or its parent is not @@ -140,23 +152,6 @@ class EntryScopeGuard */ public static function resolveTripChild(Grav $grav, string $slug): ?PageInterface { - if (!self::isSafeSegment($slug)) { - return null; - } - $pages = $grav['pages']; - // In the API request context the page tree is lazily disabled; enable it - // so find() can resolve (mirrors resolveActiveDailyChild). Idempotent. - if (method_exists($pages, 'enablePages')) { - $pages->enablePages(); - } - $page = $pages->find('/trips/' . $slug); - if ($page === null) { - return null; - } - $parent = $page->parent(); - if ($parent === null || $parent->route() !== '/trips') { - return null; - } - return $page; + return self::resolveChildOf($grav, '/trips', $slug); } } diff --git a/plugins/entry-actions/classes/EntryActionsApiController.php b/plugins/entry-actions/classes/EntryActionsApiController.php index 61be38a..f9d536b 100644 --- a/plugins/entry-actions/classes/EntryActionsApiController.php +++ b/plugins/entry-actions/classes/EntryActionsApiController.php @@ -209,12 +209,26 @@ class EntryActionsApiController extends AbstractApiController // store (deleteAll → APCu flushAll) AND apcu_clear_cache() directly to be // certain, clear the compiled files, and reset the in-memory tree so the // next request rebuilds from disk and re-reads the published flag. - $this->grav['cache']->deleteAll(); - if (function_exists('apcu_clear_cache')) { - apcu_clear_cache(); + // save() above is already persisted to disk. If any invalidation call + // throws, do NOT let it bubble to a plain 500 (which reads to the owner as + // "nothing happened") and skip the audit line: the on-disk flag DID change. + // Log a loud reconciliation warning instead so an operator knows to clear + // cache manually, then still report success. + try { + $this->grav['cache']->deleteAll(); + if (function_exists('apcu_clear_cache')) { + apcu_clear_cache(); + } + $this->grav['pages']->reset(); + $this->grav['cache']->clearCache('standard'); + } catch (\Throwable $e) { + $this->grav['log']->error(sprintf( + 'entry-actions: trip "%s" published=%s SAVED to disk but cache invalidation failed (%s) — clear cache manually', + $slug, + $published ? 'true' : 'false', + $e->getMessage() + )); } - $this->grav['pages']->reset(); - $this->grav['cache']->clearCache('standard'); // Audit trail: publish state is owner-only and changes site-wide // visibility — record who flipped which trip to what. $this->grav['log']->info(sprintf('entry-actions: owner "%s" set trip "%s" published=%s', $user->username, $slug, $published ? 'true' : 'false'));