Resolve Halcyon DB template mtimes without a query per check

[LukeTowers]

Problem

Halcyon\Builder::getCached() validates every warm cache hit by calling isCacheBusted(), which calls datasource->lastModified(). For database-backed templates that is a full SELECT * per template per request, purely to read updated_at:

select * from `cms_theme_templates` where `source` = ? and `deleted_at` is null and `path` = ? limit 1

So the Halcyon object cache removes zero database traffic for DB-backed templates — it only saves the parse. On a site taking heavy bot traffic this was the second-largest consumer of database time (717.8K calls, 2h 01m, 10.08ms avg — most of that cost being the content blob transfer).

Fix

getAvailablePaths() already builds a rememberForever manifest of the paths held by the datasource in a single query, and AutoDatasource invalidates it on every insert/update/delete. Recording each live record's modification time in that manifest lets consumers resolve mtimes with no additional query.

Timestamps are truthy, so the existence/deletion contract of the map is unchanged and every existing consumer keeps working (!$paths[$path], array_filter, isset). The paths cache key is versioned (-v2-) so stale boolean payloads are rebuilt rather than misread, and the documented halcyon.datasource.db.beforeGetAvailablePaths event may still return booleans.

Also narrows lastModified() to select('updated_at') instead of the whole row, for the paths that still query. The surrounding try/catch only existed to swallow the null-property error on a missing record, so it is replaced with an explicit check.

Docblock

getAvailablePaths() previously documented (and inherited) a boolean-only contract. The return shape is now widened, so both DatasourceInterface and this implementation describe it explicitly as array<string, int|bool>. The halcyon.datasource.db.beforeGetAvailablePaths event may still supply plain booleans, so the timestamps are an optimization consumers must not rely on being present.

Keeping the manifest's truthiness invariant

Consumers decide whether a path can be handled by testing the manifest value for truthiness — AutoDatasource::getDatasourceForPath() treats a falsy value as deleted, getValidPaths() filters on it, selectOne() retries on it. Storing timestamps breaks that for exactly one value: a live record whose updated_at is the Unix epoch yields 0.

Caught in review. Before the fix, such a record vanished:

	before fix	after fix
manifest value	0	true
in select() listing	no	yes
selectOne()	null	returns content
lastModified()	0	0 (live query)

Note lastModified() still returned 0 while everything else treated the record as deleted — because it tests with is_int() while the others test truthiness. That asymmetry between a type test and a truthiness test is the actual defect.

Falling back to true for the epoch restores the invariant, so false is once again the only falsy value the map can hold, and such records simply resolve their mtime with a query as before. DatasourceInterface now documents the requirement so other implementations don't reintroduce it.

Out of scope: nullable updated_at

updated_at is nullable, and Carbon::parse(null) resolves to now in selectOne() (which supplies the cached mtime), lastModified(), and the manifest alike. Those three never agree for such a record, so isCacheBusted() is always true and the cache is busted on every request — before and after this change. Verified:

selectOne mtime : 1786348933
manifest (t0)   : 1786348933
lastModified(t1): 1786348934

This is pre-existing and deliberately not addressed here. Records with a null updated_at cannot be produced through insert() or update(), which always set the column; they only arise from direct SQL or seeding. Fixing it properly means agreeing on one truthy sentinel across all three call sites (0 would read as "deleted" in the manifest), which is a separate concern from this PR.

Tests

Neither datasource had any coverage before this branch, so both are now covered properly rather than only the lines that moved — 68 tests across the two.

tests/Halcyon/DbDatasourceTest.php (36) — selectOne/select with column, extension and fileMatch filters; insert/update/delete/forceDelete including renames, extension changes and reviving soft-deleted records; lastModified; the paths manifest; cache keys; and the DB-specific behaviours: soft deletion, source scoping, and the beforeInsert / beforeUpdate / extendQuery / beforeGetAvailablePaths events.

tests/Halcyon/FileDatasourceTest.php (32) — the same interface surface, plus nested directory creation, path traversal handling, maxDepth recursion and live mtimes.

The one that matters for reviewers

FileDatasource::getAvailablePaths() deliberately still returns true, not a timestamp, and testGetAvailablePathsReturnsTrueForEveryPath pins that. Reporting timestamps there would be actively harmful — unlike a database round trip, a filesystem mtime is a cheap local stat, and freezing it into the forever-cached manifest would mean template edits on disk (a deploy, say) go unnoticed until the manifest is rebuilt. AutoDatasource depends on the true value to fall through to a live filemtime().

Verified by patching FileDatasource to return filemtime() — the test fails immediately.

Regression guards

Run against the pre-change source (fd673f4f), three fail and the other 33 pass, so the rest genuinely characterize existing behaviour:

1) testGetAvailablePathsReturnsTimestampsForLiveRecords
   Failed asserting that true is identical to 1559390400.
2) testLastModifiedDoesNotSelectTheContentColumn
   Failed asserting that 'select * from "halcyon_tester_templates" where "source" = ? and "deleted_at" is null and "path" = ? limit 1' does not contain "*".
3) testGetPathsCacheKeyIsVersionedAndScoped
   Failed asserting that two strings are identical.

Two pre-existing behaviours are asserted as they actually are rather than as they might be assumed: FileDatasource::selectOne() swallows the traversal exception along with every other read error, so escaping the base path returns null rather than throwing; and DbDatasource::delete() throws for an already soft-deleted record, because its query excludes them.

Full suite: 840 tests. The 1 error / 4 failures that remain are pre-existing (ConfigWriterTest, ArrayFileTest — PHP config-file formatting) and are present on develop without this change.

Companion PR

Requires wintercms/winter#1508, which consumes the manifest in Cms\Classes\AutoDatasource::lastModified(). Land together.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Improved handling of missing records by returning no modification date instead of an error.
  • Available paths now show accurate update timestamps for active records.
  • Deleted paths continue to be identified correctly.
  • Database lookups are properly scoped and avoid loading unnecessary content.

• Tests

  • Added coverage for timestamps, deleted records, source scoping, event filtering, and missing records.

[Copilot]

Pull request overview

This PR optimizes Halcyon’s DB-backed template cache validation by avoiding heavyweight row loads when resolving modification times, and by enriching the DB datasource “available paths” manifest so consumers can resolve mtimes without per-template queries (with a companion PR in wintercms/winter consuming the new manifest payload).

Changes:

• Narrow DbDatasource::lastModified() to select only updated_at (instead of SELECT *) and avoid exception-driven control flow for missing records.
• Version the DB paths cache key (-v2-) and change getAvailablePaths() to map live records to their updated_at timestamps (deleted records remain false).
• Add new PHPUnit coverage for DB datasource path manifests, source scoping, event contract compatibility, and query shape.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
tests/Halcyon/DbDatasourceTest.php	Adds regression tests for DB datasource path manifests, event behavior, and query-column selection.
src/Halcyon/Datasource/DbDatasource.php	Updates mtime lookup to avoid SELECT *, versions the paths cache key, and stores timestamps in the available-paths manifest.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[LukeTowers]

Thanks both. Addressed one of the two, and deliberately not the other.

Docblock / @inheritDoc — fixed

Fair catch. getAvailablePaths() inherited a docblock that explicitly documents a boolean map, complete with an example, so the widened return shape was genuinely misleading for phpstan and IDEs. Both DatasourceInterface and the DbDatasource implementation now describe it as array<string, int|bool>.

Worth noting for anyone consuming this: the halcyon.datasource.db.beforeGetAvailablePaths event may still return plain booleans, and FileDatasource returns true, so the timestamps are an optimization that consumers must not rely on being present. That is now stated in both docblocks.

Nullable updated_at — real, but pre-existing and out of scope

The underlying defect is real, though it is worse than described. It is not just that lastModified() and the manifest can drift; selectOne() also resolves Carbon::parse(null) to now, and that is what supplies the cached mtime that isCacheBusted() compares against. So all three disagree:

selectOne mtime : 1786348933
manifest (t0)   : 1786348933
lastModified(t1): 1786348934

The consequence is that a record with a null updated_at has its cache busted on every request and never converges — both before and after this PR. So this is not a regression introduced here.

Not fixing it in this PR, for two reasons:

1. Such records cannot be produced through the datasource API. Both insert() and update() always set updated_at; nulls only arise from direct SQL or seeding.
2. A correct fix has to apply one shared sentinel across all three call sites, not just the two flagged. It also has to be truthy — 0 or null would read as "cannot be handled" in the manifest and make the path look deleted, which is precisely the contract this PR is careful to preserve.

That is a self-contained change with its own reasoning and test surface, so it belongs in its own PR rather than being bolted onto this one.

@coderabbitai one correction: the manifest does not "return different mtimes across reads without a database update". It is built once and held in Cache::rememberForever, so it is stable for the lifetime of that entry. It is only rebuilt per-request when app.debug is enabled, because AutoDatasource::fetchPathCache() intentionally bypasses the cache in debug mode — dev-only, and pre-existing.

I have also removed the "incidental fix" claim from the PR description, which overstated what freezing the manifest value achieves.