Skip to content

Bind the context parse to the revision and clarify provider docs - #1031

Closed
malberts wants to merge 3 commits into
provide-content-to-page-property-providersfrom
pr-982-review-fixes
Closed

Bind the context parse to the revision and clarify provider docs#1031
malberts wants to merge 3 commits into
provide-content-to-page-property-providersfrom
pr-982-review-fixes

Conversation

@malberts

@malberts malberts commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Follows-up to #982, delivering review fixes onto that PR's
branch (this diff contains only the fixes).

Bind the context parse to the stored revision. The parse that fills categories and parserProperties
ran without a revision id, i.e. as a preview-like parse: revision-context magic words ({{REVISIONID}},
{{REVISIONUSER}}, ...) and parser hooks that read the revision while recording page properties resolved
against nothing, so the recorded values could diverge from what core stored for the same revision. That
mattered little while the parse only fed categories, but the PR publishes the parse products to providers
as the recommended derivation path. The fix passes $revision->getId() through ContentParseParams. The
test pins the behavior with a {{DEFAULTSORT:Rev{{REVISIONID}}}} page: it records Rev without the fix and
Rev<id> with it, and a second stored revision ensures resolving the page's latest revision instead of the
provided one also fails. Verified end to end on a dev stack, where the graph property carries the actual
revision id.

Pin the remaining documented contracts with tests. The content-unavailable sentinels (content,
contentModel empty and parserProperties, categories empty when the main-slot content is hidden) had no
covering test, so type-clean drift such as switching the builder to getContentOrThrow would pass the whole
suite while making undeletion of a page with rev-deleted history fatal in production; the new test suppresses
a real revision and asserts what a provider receives. The contentModel test only exercised wikitext, the
one value a hardcode would also produce, so a JSON page now pins that the model comes from the actual
content. Each test was verified against a production mutation that only it catches.

Docs accuracy in extending.md. The sentence introducing the context now names the content and parse
products (previously it promised content derivation while enumerating only the pre-existing fields), and the
section states when providers actually run: when a revision is stored for a page carrying Subject data
(including undeletions) and during RebuildGraphDatabases runs, with pages whose last Subject was deleted
still qualifying via their subject slot.

Review-fix delivery directed by @malberts over two rounds: he triaged the initial review's findings and
scoped the first delivery to them, then requested a deeper re-review that produced the additional tests and
docs corrections; the history is squashed to one commit and @malberts gave the diff a quick look before requesting review. Findings
come from multi-agent reviews of #982 (code review, security review, test review, adversarial verification
of candidate blockers, with MediaWiki 1.43 core-contract claims checked against core source), and a third,
independent re-review of #982 reproduced the findings these fixes address. All code changes were developed
test-first or mutation-verified, with the full PHPUnit, phpcs, and phpstan gate run locally and the parse
binding verified end to end on a running dev stack.
Context: the NeoWiki codebase, the PR #982 and issue #979 discussions, and MediaWiki 1.43 core source.
Written by Claude Code, Fable 5 (initial fixes at high effort, later additions at max)

JeroenDeDauw and others added 2 commits July 3, 2026 18:29
Fixes #979

PagePropertiesBuilder already loaded the revision's main slot content and
parsed it to extract categories, but threw everything else away. Providers
wanting to derive Page Properties from the content had to re-fetch and
re-parse it themselves - and since the context carries no revision ID, they
could only look up the latest revision of the page, which is redundant work
and subtly racy.

The context now additionally exposes:

* `content` - the serialized main slot content, e.g. the wikitext
* `contentModel` - e.g. "wikitext"
* `parserProperties` - the MediaWiki page properties recorded while parsing
  the content (e.g. "defaultsort", or values set by parser hooks via
  ParserOutput::setPageProperty)

Design notes:

* The context is a Domain value object holding only scalars and arrays.
  Exposing RevisionRecord, Content, or ParserOutput directly would put
  MediaWiki core classes into the Domain layer and make provider tests
  heavyweight. The relevant data is flattened instead, following the
  approach already used for `categories`.
* `parserProperties` covers the template-expansion-safe path: extensions
  whose parser hooks already record values during the parse can read them
  back in their provider instead of re-analyzing raw markup. The parse
  NeoWiki already performs for categories is reused, so no additional parse
  happens.
* When the content is unavailable (e.g. suppressed), `content` and
  `contentModel` are empty strings and `parserProperties` is empty,
  matching the existing behavior of `categories`.

Note: content-derived properties refresh only when a new revision is
stored, so values coming from transcluded templates can go stale - the same
existing limitation as categories.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follow-up polish on the extending.md guidance for the new context fields:
recommend deriving Page Properties from the robust parse products
(categories, parserProperties) and frame raw content/contentModel as the
escape hatch for custom, non-wikitext content models rather than the default.
Also carries the "parserProperties is a parse input, not the NeoWiki Page
Properties you return" disambiguation into the docs, where provider authors
actually read it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@malberts
malberts marked this pull request as ready for review July 15, 2026 13:48
The parse that fills categories and parserProperties ran without a
revision id, i.e. as a preview-like parse: revision-context magic words
({{REVISIONID}}, {{REVISIONUSER}}, ...) and parser hooks that read the
revision while recording page properties resolved against nothing, so
the recorded values could diverge from what core stored for the same
revision. That mattered little while the parse only fed categories, but
the parse products are now published to providers as the recommended
derivation path. The fix passes the revision id through
ContentParseParams. The new test pins the behavior with a
{{DEFAULTSORT:Rev{{REVISIONID}}}} page: it records Rev without the fix
and Rev<id> with it, and a second stored revision ensures resolving the
page's latest revision instead of the provided one also fails.

The content-unavailable sentinels (content and contentModel empty,
parserProperties and categories empty when the main-slot content is
hidden) had no covering test, so type-clean drift such as switching the
builder to getContentOrThrow would pass the whole suite while making
undeletion of a page with rev-deleted history fatal in production; the
new test suppresses a real revision and asserts what a provider
receives. The contentModel test only exercised wikitext, the one value
a hardcode would also produce, so a JSON page now pins that the model
comes from the actual content.

In extending.md, the sentence introducing the context now names the
content and parse products, and the section states when providers
actually run: when a revision is stored for a page carrying Subject
data (including undeletions) and during RebuildGraphDatabases runs.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@malberts
malberts force-pushed the pr-982-review-fixes branch from 114b10f to 2116ae6 Compare July 15, 2026 13:50
@malberts
malberts requested a review from JeroenDeDauw July 15, 2026 13:57
@JeroenDeDauw
JeroenDeDauw force-pushed the provide-content-to-page-property-providers branch from ec599b9 to 489a30f Compare August 20, 2026 23:01
@JeroenDeDauw
JeroenDeDauw deleted the branch provide-content-to-page-property-providers August 20, 2026 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants