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.
chore: state the real PHP floor in file docblocks
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.
perf/test: cache library previews and serve E2E with workers
Every version repeats "Added", "Changed", "Fixed" and friends, so 46 of them collided and markdownlint's MD024 flagged 40 duplicates. The file already had the fix for its older entries — "### Added in 2.9.0-fork" — but the 3.x sections dropped it. Applies the existing convention to the 15 sections that lacked it, which also gives each one a usable anchor. Regenerates docs-src/changelog.md, which had drifted 322 lines behind and was missing the 3.6.0-fork notes entirely.
ParseCoverage took pre-digested counts, so each of its three call sites decided for itself what it was counting: the reading view measured the raw text, the check page and the API summed token lengths. The class exists to be the one place that decides a parse came out empty, and it could not be, because the same text crossed the floor on one surface and not another (#289). It now accumulates the text token by token and does its own counting, so a caller hands over material and never a measurement. That also allows a better signal than word-per-character density: the share of a text's letters that end up inside words, which does not move with how spaced-out a script is and reads the same at any length. Two things the density rule got wrong follow from it. A text holding no letters at all -- 123, 2024, ?!..., a price list -- was told "the language's Word Characters setting does not match this text", a confident falsehood about a correctly configured language; it is now silent, and NO_WORDS fires only when letters are present and none of them matched. And the 200-character exemption is gone, so a short non-Latin text no longer falls through it in silence: a 122-character Chinese text on a Latin language warned about nothing, which is the experience #278 reports. Verified against the 13 texts of a real database -- French, German, Chinese in both the character-split and non-split configurations, Japanese, Korean, Thai and Hebrew -- with no verdict changing.
LgRegexpWordCharacters is the field that should hold a word-characters regex. Historically it could instead hold the literal MECAB, which said two unrelated things at once: tokenize with MeCab, and this language has no spaces between words. LgParserType says the first properly, but nothing said the second, so every site that needed either fact asked about the magic word -- twelve of them, each spelling the comparison out by hand (#288). WordSpacing is the home that was missing. SentenceService now asks whether a language separates its words with spaces rather than what its word-characters field happens to contain, and the one place that still recognises the marker is the one place that has to change when the migration retires it. Spelling a comparison out twelve times went wrong three times, all silently. GetPhoneticReading tested !== "mecab" in lowercase, so an install holding MECAB in any other casing got its input back with no reading and no error. ExternalParser had the mirror bug, comparing against uppercase only, so the lowercase mecab the form itself writes was taken for a regex and compiled as the character class [mecab] -- making exactly the letters m, e, c, a and b that language's word characters. LanguageApiHandler picked the TTS reading mode with the lowercase version of the same test. All three now agree with everyone else. The form's "MeCab (recommended)" option is gone with them. It wrote the literal over whatever regex the field held, and only appeared once a language was already named Japanese -- by which point the parser dropdown already carried the choice. Nothing writes the magic word now; every reader still accepts it, so existing installs are untouched until the migration lands. Behaviour is otherwise preserved: all 32 combinations of removeSpaces, splitEachChar and the word-characters field give the verdict the two old expressions gave. Verified with MeCab installed, which turns the phonetic- reading fix from a code change into a visible one.
The marker in LgRegexpWordCharacters said two things at once, and the previous commit gave both a home to be read from. This hands them over for real: a migration sets LgParserType = 'mecab' and LgRemoveSpaces = 1, then puts the Japanese preset's regex back in the field, so a migrated language ends up looking like a freshly created one (#288). The readers had to move first. Anything choosing a tokenizer now asks LgParserType through ParserSelection, falling back to the marker only for a row the migration has not reached -- TextParsing, SentenceService, ExpressionService, GetPhoneticReading, LanguageApiHandler and Maintenance's lookup. Clearing the field under the old readers would have dropped Japanese onto the regex tokenizer: the text still opens, and every word in it is wrong. Writing the before/after comparison the issue asks for turned up that this had already happened, without any migration. ParserRegistry sent a language naming 'mecab' to MecabParser, a second tokenizer shelling out to the same binary as the built-in JapaneseTextParser, and MecabParser read MeCab's character-type column with the test inverted: every word came out a non-word and every 。 came out a word. Twelve words parsed to three. Nothing hit it only because the magic word sent legacy languages back to the built-in pipeline -- but a language made from the Japanese preset names 'mecab' and carries a real regex, so it went straight there and parsed into almost nothing. Fixed, and MeCab now always stays on the built-in pipeline: one tokenizer, so there is nothing left to drift. Verified against a fixture install: the same Japanese text parsed before and after the migration, across all three legacy flag combinations, gives identical tokens, sentence split and phonetic reading -- 0 unexpected changes. The migration is idempotent (applied twice), writes exactly the langdefs value with the backslashes MySQL needs, and the app applied it for real during the E2E run.
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.
Sixteen commits on
developsince 3.6.0-fork. No version bump yet — if thisis meant to ship as 3.7.0-fork, that commit still needs to go on top
(
ApplicationInfo.php,package.json,package-lock.json, and moving[Unreleased]in the changelog). Two new classes already carry@since 3.7.0.Parsing: two issues on the same bug family
#289 — the parse-coverage banner. Fixes all three edge cases, which shared
one root cause:
ParseCoverage::assess()took pre-digested counts, so each ofits three call sites decided for itself what it was counting — the reading view
measured the raw text, the check page and the API summed token lengths. It now
accumulates the text token by token and does its own counting, so the single-
source-of-truth property is structural rather than conventional.
That also allowed a better signal than word-per-character density: the share of
a text's letters that end up inside words, which doesn't move with how
spaced-out a script is and reads the same at any length.
123,2024,3.14,?!..., a pricelist — was told "the language's Word Characters setting does not match this
text" and sent to change a setting that was working. It's now silent, and
NO_WORDSfires only when letters are present and none matched.through it in silence — the case Simplified Chinese doesn't split words or allow splitting words #278 reports.
Verified against 13 real texts (French, German, Chinese in both configurations,
Japanese, Korean, Thai, Hebrew): 0 verdict changes.
#288 — the
MECABmagic word.LgRegexpWordCharactersshould hold aword-characters regex; it could instead hold the literal
MECAB, saying twounrelated things at once — tokenize with MeCab, and this language has no
spaces between words. Twelve sites spelled the comparison out by hand.
WordSpacingandParserSelectionanswer the two questions; a migrationhands the marker's jobs to
LgParserTypeandLgRemoveSpacesand puts thepreset's real regex back in the field.
and only appeared once a language was already named Japanese, by which point
the parser dropdown had the choice covered.
Steps 4 and 5 of the issue's plan (deprecation logging, then deleting the
fallback readers) are deliberately left for a later release, so #288 stays
open.
Three silent bugs found on the way
GetPhoneticReadingcompared!== "mecab"unnormalized, so an installholding
MECABin any other casing got its input back with no reading and noerror.
LanguageApiHandlerhad the same test for the TTS reading mode.ExternalParserhad the mirror bug — uppercase only — so the lowercasemecabthe form itself wrote was taken for a regex and compiled as thecharacter class
[mecab], making exactly the letters m, e, c, a, b thatlanguage's word characters.
MecabParserread MeCab's character-type column with the test inverted,so every word came out a non-word and every
。came out a word: twelve wordsparsed to three. This is a live bug on
develop, not one the migrationintroduces — it hits any language created from the Japanese preset, which
names
mecaband carries a real regex. MeCab now always stays on the built-inJapaneseTextParser: one tokenizer, nothing left to drift.Already on develop before this work
Config islands behind one emitter and one reader (#301), the annotated display
view rendered client-side (#302),
Globalscut from 505 to 353 lines (#300),800 lines of unreachable server-side rendering deleted (#299), cached difficulty
previews plus the
npm run serveworker fix that stopped Cypress timing out(#303), and PHP-floor/docs corrections (#298).
Verification
phpcs / ESLint / tsc clean, assets rebuilt.
parsed before and after, across all three legacy flag combinations, gives
identical tokens, sentence split and phonetic reading — 0 unexpected
changes. It is idempotent (applied twice), writes exactly the langdefs value,
and the app applied it for real during the E2E run.
end to end rather than reasoned about.
Closes #289.