Retire the last server-rendered views, shrink Globals, and unify the PHP→JS bridge - #297
Closed
HugoFara wants to merge 9 commits into
Closed
Retire the last server-rendered views, shrink Globals, and unify the PHP→JS bridge#297HugoFara wants to merge 9 commits into
HugoFara wants to merge 9 commits into
Conversation
Every file header claimed "PHP version 8.1" while composer.json has required ^8.2 and CI has tested 8.2-8.5 for some time. The headers were the only thing still naming 8.1, so they were actively misleading anyone reading for the supported baseline. Mechanical: the docblock line only, in the files where it is the sole change.
Three leftovers from before the reading and review screens moved to client-side rendering: - TextReadingService (333 lines) echoed the reading pane word span by word span. It has no callers; text_renderer.ts took the job over. The only surviving mention was a comment in that file pointing back at it, which now describes the underscore attribute contract on its own terms. - Review's header.php, header_content.php and footer.php are reachable only through ReviewController::header(), and no route registers it. Removing the method takes five tests with it, four of which asserted nothing beyond assertTrue(true) around an include that was expected to fail. - PageLayoutHelper::renderFramesetHeader() has no callers; the app has no framesets left.
Globals had grown into a grab-bag where only some of the contents did
anything. Four things came out:
- table() was the identity function — `return $tableName;` — left over from
a configurable table prefix that no longer exists. It read as though it
transformed the name, and HomeFacade even carried a comment claiming it
returned "properly prefixed" names. Its 185 call sites are now plain
strings, and the literal concatenations that produced ('DELETE FROM ' .
'words') are folded into single literals.
- query() forwarded one line to QueryBuilder::table(). Callers now say so.
- The error-display flag was set to false by initialize() and never set
true by anything; its single reader passed it straight to
setErrorReporting(). That reader now passes false outright, which is what
it always got. initialize() had nothing left to do and is gone.
- The mysqli handle lived in both Connection::$instance and
Globals::$dbConnection, with getInstance() resyncing from the second on
every call and setInstance() writing both. Connection now owns it alone
and Globals::get/setDbConnection() delegate. The accessors stay because
291 call sites use them as a "is the database up?" probe; the duplicated
storage was the problem, not the name.
One behaviour change falls out: Connection::reset() now genuinely clears the
handle instead of leaving a copy for the next getInstance() to recover.
testResetClearsInstance was asserting the opposite of its own name, so it now
asserts the clear and restores the shared connection for the rest of the run.
Also drops 29 Globals imports left unused by the above, and documents on the
class that the user context is a security boundary QueryBuilder depends on
rather than an ordinary setting.
…one reader
The CSP build of Alpine cannot evaluate inline expressions, so server values
reach a page as a JSON island rather than through x-data. That pattern was
sound; the plumbing around it was not. Thirty-five islands each hand-rolled
their own script tag and escaping flags, and eight pages hand-rolled their own
reader, so the two ends of each contract could drift with nothing to catch it.
Adds ConfigIsland (PHP) and readPageConfig/hasPageConfig (TypeScript), and
routes all of it through them. What that fixes concretely:
- Six islands emitted json_encode with no escaping flags at all, so a value
containing </script> would close the element early and the rest of the blob
would parse as markup.
- preferences.php hand-spliced a pre-JSON-encoded value into a literal
{"currentLanguageCode": ...}, which is only valid as long as the controller
remembers to encode it. It now passes the plain string and the controller
stops encoding.
- starter_vocab.ts parsed without a try/catch, so a malformed blob threw
during init() and left the page an inert shell. readPageConfig never throws:
missing element, empty blob and bad JSON all fall back to the caller's
defaults, merged shallowly with null treated as absent.
JsonScriptBlockEscapingTest now pins a stronger invariant than the old flag
scan. One test feeds the emitter a payload that tries to close its own script
element and checks the rendered output; the other asserts no file outside
ConfigIsland emits a config block itself, so a new view cannot quietly
reintroduce the hand-rolled form.
The guide still routed work to src/backend/Views, src/backend/Services and src/backend/Router, none of which exist — what remains under src/backend is the REST API and the entry point. It also named getSettingWithDefault(), Globals::table(), `global $var`, the archivedtexts and textitems2 tables, and a frontend laid out as pgm.ts / text_events.ts / audio_controller.ts. All of those are gone; archived texts are a TxArchivedAt column on texts and textitems2 is word_occurrences. Corrects the request flow, directory tree, table list and frontend layout to match, adds the Activity and Book modules, and states the 8.2 docblock floor. Two additions rather than corrections: the request-context section now says outright that QueryBuilder reads the user context to scope every query, so a repository that looks unscoped is filtered and clearing the context is a security event; and a new Config Islands section documents ConfigIsland and readPageConfig as the only sanctioned way to move values from PHP to a page.
Two gaps in GET /texts/{id}/annotation, both found while moving the
annotated display view onto it:
- Items carried no romanization, so a client could only get one by querying
per word. Added, resolved for the whole text in a single query rather than
one lookup per term — the annotated view renders every word at once, so the
naive form would be hundreds of round trips on a normal text.
- Blank rows came back as words with empty text. Splitting the stored
annotation on newlines yields an entry for any trailing or repeated newline,
and the endpoint reported those as terms; the annotated print view has been
rendering two empty <ruby> elements per text as a result. They are now
skipped, matching the rule TextDisplayService::parseAnnotationItem() has
always applied.
/text/{id}/display was the last page still building content in PHP: it
parsed the stored annotation and echoed a <ruby> per term. It now ships a
shell and fetches GET /api/v1/texts/{id}/annotation, which the print view
already used, so display_main.php, display_header.php and display_text.php
collapse into one display_alpine.php.
The generated markup is deliberately identical to what PHP emitted —
.anntermruby for the term inside <rb>, .anntransruby2 for the translation
inside <rt>, romanization as a title attribute, and a paragraph marker
closing and reopening <p> at the language's text size with the same 1.35
then 1.3 line heights. annotation_toggle.ts drives the show/hide buttons by
querying those classes at click time, so anything else would break them
silently. Verified against both annotated texts in the dev database: same
paragraph count, same term count, same line heights, toggles still working.
Prev/next navigation stays server-rendered. It reads the request's language
filter, search query and tag selection out of the session, none of which has
an endpoint, and it is chrome rather than content.
The suggestion panels on /texts/new and the home page request a difficulty preview for every book they list, and each preview downloaded the whole book again: one call measured at 5.4 s, all of it the fetch, against milliseconds for the tokenizing and vocabulary lookup that follow. Nothing was cached, so opening the page refetched the same handful of books every time and pinned a PHP worker per download for the duration. Adds a small file-backed FileCache and puts the fetched source document behind it, keyed by URL with a 24 h TTL matching the Gutenberg suggestion cache. Same call now: 5.4 s cold, 0.02 s warm. Only the document is shared — coverage and difficulty are still computed per user on top of it, so the figures stay personal. FileCache degrades to a miss on every failure it can hit: an unwritable temp directory, an unreadable entry, an expired one. The cost of a miss is a refetch, and a cache that can throw on a page load would be worse than no cache at all. Writes go to a temp file and rename, so a reader never sees a half-written entry and two writers cannot interleave.
Three specs in 05-texts.cy.ts failed, and the symptoms pointed at the
application: text creation "did not redirect", and /text/edit failed to load
with ESOCKETTIMEDOUT. Neither was true. The POST succeeded, the redirect to
/text/{id}/read was issued, and the browser was already requesting it — the
request was simply queued behind other work and arrived after Cypress's 10 s
assertion timeout. Measured on the documented setup: a page that takes 11 ms
on an idle server took 28.5 s while difficulty previews were in flight.
The cause is `php -S`, which serves one request at a time unless
PHP_CLI_SERVER_WORKERS is set. Any slow request — a text import, an outbound
fetch — stalls the whole suite behind it, and the failure surfaces somewhere
unrelated. Adds `npm run serve` as the one way to start the dev server, with
workers, and points CLAUDE.md and both copies of the contributing guide at it
with the reason.
With that and the source-document cache, the full suite goes from 4 failures
in 6:49 to 304 passing in 3:03 from a cold cache.
Owner
Author
|
Closing in favour of a reviewable stack — 592 files in one PR was not a fair thing to ask anyone to read. Same nine commits, unchanged, split into six PRs that each do one thing and stack in order:
Each is based on the one before it, so its diff shows only its own change. Merge in order and each base retargets to |
Owner
Author
|
Stack is up, with the real numbers:
Each is based on the one before it, so its diff shows only its own change. Merge in order and GitHub retargets each base to |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Started as an audit — can more PHP views become pure frontend, and how is global
state passed around — and turned into the cleanup that audit called for.
The short answer to the first question was "less than you'd think": of 65 views,
only six still contained a server-side
foreach, and all six are<option>lists. The data screens had already moved. What was left was dead code, a
Globalsclass where half the members did nothing, and a PHP-to-JS bridge thathad drifted apart at 35 separate call sites.
Dead code
TextReadingService(333 lines) echoed the reading pane word span by wordspan. No callers;
text_renderer.tstook the job over. The only survivingmention was a comment pointing back at it.
header.php,header_content.php,footer.phpare reachableonly via
ReviewController::header(), which no route registers. Removing themethod takes five tests with it — four asserted nothing beyond
assertTrue(true)around an include expected to fail.PageLayoutHelper::renderFramesetHeader()— no callers, no framesets.Globals— 505 lines down to 353table()was the identity function.return $tableName;, left from aconfigurable table prefix that no longer exists. It read as though it
transformed the name, and
HomeFacadeeven had a comment claiming it returned"properly prefixed" names. Its 185 call sites are now plain strings.
query()forwarded one line toQueryBuilder::table().falsebyinitialize()and never settrue; its single reader passed it straight through.
initialize()then hadnothing left to do.
Connection::$instanceandGlobals::$dbConnection— withgetInstance()resyncing from the second onevery call.
Connectionnow owns it alone. TheGlobalsaccessors staybecause 291 call sites use them as an "is the database up?" probe; the
duplicated storage was the problem, not the name.
One behaviour change falls out:
Connection::reset()now genuinely clears thehandle.
testResetClearsInstancewas asserting the opposite of its own name.What's left is documented as what it is: request-scoped user context that
QueryBuilderreads to scope every query, so a repository that looksunscoped is filtered. That is a security boundary, not a setting.
Config islands
Server values reach a page as a JSON island because the CSP build of Alpine
cannot evaluate inline expressions. The pattern was sound; the plumbing was not
— 35 islands each hand-rolled their own escaping flags, and eight pages
hand-rolled their own reader. Now one
ConfigIslandemitter and onereadPageConfigreader. That fixes:json_encodewith no escaping flags at all, so avalue containing
</script>closed the element early.preferences.phphand-spliced a pre-encoded value into a literal{"currentLanguageCode": …}, valid only while the controller remembered toencode it.
starter_vocab.tsparsed without atry/catch, so a malformed blobthrew during
init()and left an inert shell.JsonScriptBlockEscapingTestnow pins a stronger invariant than the old flagscan: one test feeds the emitter a payload that tries to close its own script
element and checks the output; the other asserts no file outside
ConfigIslandemits a config block, so a new view cannot quietly reintroduce the old form.
The one real conversion
/text/{id}/displaywas the last page building content in PHP. Three viewscollapse into one shell that fetches
GET /api/v1/texts/{id}/annotation— theendpoint the print view already used. The generated markup is deliberately
identical, because
annotation_toggle.tsfinds terms by the.anntermruby/.anntransruby2classes at click time and anything else would break theshow/hide buttons silently.
That endpoint had two gaps, fixed first:
— a lookup per term is hundreds of round trips on a normal text.
yields an entry for any trailing newline, and those were reported as terms.
The annotated print view has been rendering two empty
<ruby>elements pertext as a result — a pre-existing bug, not one introduced here.
Prev/next navigation stays server-rendered: it reads the language filter, query
and tag selection out of the session, none of which has an endpoint.
E2E: three failures that were not application bugs
Two specs reported that text creation "did not redirect"; a third failed with
ESOCKETTIMEDOUT. None of that was true. The POST returned 200, the redirectwas issued, the browser was already fetching it — the request was queued and
landed after Cypress's 10 s timeout. A page taking 11 ms on an idle server took
28.5 s while difficulty previews were in flight.
Two causes:
preview for every book listed, and each downloaded the whole book from
Gutenberg — 5.4 s, essentially all network — with no cache, on every page
load. A
FileCachenow holds the fetched document: 5.4 s cold, 0.02 swarm. Only the document is shared; coverage is still computed per user, so
the figures stay personal. This helps production, not just tests.
php -Sserves one request at a time unlessPHP_CLI_SERVER_WORKERSis set, which is what turned one slow request into asuite-wide stall. Added
npm run serveas the one way to start it, documentedin
CLAUDE.mdand both copies of the contributing guide.I verified each half alone: caching alone still failed, workers alone passed.
Both are needed — the cache for the real cost, workers so one slow request
cannot starve everything.
Full suite: 304/304 passing, 23/23 specs, 3:03 from a cold cache. Previously
4 failures in 6:49 (
99-screenshotsalso failed and now clears).Docs
CLAUDE.mddescribed a codebase that no longer exists:src/backend/Views,ServicesandRouter,getSettingWithDefault(),global $var,Globals::table(), thearchivedtextsandtextitems2tables, and a frontendlaid out as
pgm.ts/text_events.ts. Corrected, plus the missing Activityand Book modules and a new Config Islands section.
Also normalised 530 file docblocks that claimed
PHP version 8.1whilecomposer.jsonhas required^8.2and CI has tested 8.2–8.5 for some time.Verification
Psalm 0 errors · PHPCS 0 errors · 9037 PHP tests · 4359 frontend tests ·
304 E2E · typecheck and lint clean · assets build.
The display conversion was checked against both annotated texts in a real
database rather than trusting the tests: same paragraph count, same term count,
same line heights, toggles still working.
Reviewing this
The nine commits are meant to be read in order and are independently sensible;
the 530-file docblock commit is mechanical and worth skipping. The two changes
that most deserve a second opinion are
Connection::reset()now actuallyresetting, and keeping the
Globals::get/setDbConnection()accessors ratherthan rewriting 291 call sites.