Conversation
1b020f9 to
a66855a
Compare
e6b625d to
0b33897
Compare
jaapio
left a comment
There was a problem hiding this comment.
I think this is a great addition to what we already do with this project. However I would love to see a bit smaller PR in steps. I think the template on documents makes a lot of sense also in a separate feature.
The compiler pass needs some extra attention. It contains a lot of logic, which could be split. This would allow tools like phpDocumentor to extend the index before rendering.
0b33897 to
bf1c649
Compare
Extract the duplicated trim+filter-empty logic (lines and segments) into trimAndFilterEmpty(), per review feedback on PR #1358. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PP4LkejR5PSubbhNmF4RkT
05370a5 to
2aa3031
Compare
Add the generic ':template:' field-list metadata: TemplateNode, TemplateFieldListItemRule to parse it, TemplateMetadataNodeRenderer to render it as nothing (it's a marker, not visible content), and DocumentNode::getTemplate() to expose the selected template name. Split out of #1358 per review feedback -- the genindex work on that branch is the first consumer (`:template: genindex`), but the mechanism itself is generic and reviewable on its own. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PP4LkejR5PSubbhNmF4RkT
e2a33b5 to
072e9da
Compare
Extract the duplicated trim+filter-empty logic (lines and segments) into trimAndFilterEmpty(), per review feedback on PR #1358. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PP4LkejR5PSubbhNmF4RkT
2aa3031 to
7cef3bc
Compare
Matches the ```rest convention already used by the other directive docblocks in this package (ErrorDirective, YoutubeDirective, etc.), per review feedback on PR #1358. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PP4LkejR5PSubbhNmF4RkT
GenIndexNodeRenderer, GenIndexRowNodeRenderer, and GenIndexTermNodeRenderer were all the same fixed node->template mapping, exactly what TemplateNodeRenderer (already used for UmlNode in guides-graphs) exists for. Register the three GenIndex node types through it instead of three bespoke classes, per review feedback on PR #1358. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PP4LkejR5PSubbhNmF4RkT
Drop isLink()/isSee()/isSeeAlso() (isSee() was unused) and switch the row template on node.kind.name directly, per review feedback on PR #1358. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PP4LkejR5PSubbhNmF4RkT
IndexCollectorPass collected entries, expanded Sphinx pair/triple/ module conventions, resolved see/seealso targets, filtered by scope, and built the node tree -- all as private methods of one 600-line class. Split into four single-purpose classes under a new Compiler/Passes/IndexCollector/ namespace: - IndexEntryCollector: walks documents and expands `.. index::` entries into a term map - GenIndexSeeResolver: points see/seealso rows at their target anchor - GenIndexTermMapFilter: scopes a term map to a `:scope:` path prefix - GenIndexNodeBuilder: turns a term map into the sorted node tree IndexCollectorPass is now a thin orchestrator over these four. Per review feedback on PR #1358 ("it feels like this class is doing too much"). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PP4LkejR5PSubbhNmF4RkT
Merging main pulled in the phpstan/phpstan 2 upgrade (#1366), which tightened several checks and turned up 12 errors on this branch's index-generation code: - GenIndexDirective/IndexDirective::process() declared a nullable return type they never actually return - IndexDirective's typo-detection loop carried a dead null check; IndexEntryType::cases() is never empty, so the loop always sets $closestType - GenIndexSeeResolver::resolveSeeRows() mutated $termMap in place via deep offset writes, which lost the 'term' key from PHPStan's tracked shape for 'subterms' entries; rebuilding each entry as a whole array literal keeps the shape precise - IndexEntryCollector::collectFromDocument() tagged flattened nodes with a string type list ('index'/'section'/'content') and matched on the string, so PHPStan couldn't narrow $node to IndexNode to call getEntries(); switched to an instanceof check instead Verified via Docker (php:8.4-cli, matching CI): phpstan clean, unit/ functional/integration suites green, phpcs clean, deptrac 0 violations. Fixes the failing "Static analysis / Static Code Analysis (8.2)" check on #1358. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PP4LkejR5PSubbhNmF4RkT
Merging main pulled in the phpstan/phpstan 2 upgrade (#1366), which tightened several checks and turned up 12 errors on this branch's index-generation code: - GenIndexDirective/IndexDirective::process() declared a nullable return type they never actually return - IndexDirective's typo-detection loop carried a dead null check; IndexEntryType::cases() is never empty, so the loop always sets $closestType - GenIndexSeeResolver::resolveSeeRows() mutated $termMap in place via deep offset writes, which lost the 'term' key from PHPStan's tracked shape for 'subterms' entries; rebuilding each entry as a whole array literal keeps the shape precise - IndexEntryCollector::collectFromDocument() tagged flattened nodes with a string type list ('index'/'section'/'content') and matched on the string, so PHPStan couldn't narrow $node to IndexNode to call getEntries(); switched to an instanceof check instead Verified via Docker (php:8.4-cli, matching CI): phpstan clean, unit/ functional/integration suites green, phpcs clean, deptrac 0 violations. Fixes the failing "Static analysis / Static Code Analysis (8.2)" check on #1358. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PP4LkejR5PSubbhNmF4RkT
bec4b87 to
ad2b219
Compare
|
@jaapio I applied all requested changes, please resume review |
|
@jaapio Replaced the phpstan-type array shapes in the IndexCollector classes with real value classes (GenIndexRowData, GenIndexTermData, GenIndexTermMap) per your Slack feedback and the article you linked. Ready for another look. |
Sphinx-authored documentation -- including TYPO3 Core's own Changelog files -- relies on `.. index::` entries so readers can jump straight to every page a term is discussed on, the way a printed book's index does. This library parsed those entries but threw them away, so porting such docs here meant silently losing that navigation aid. Entries are now collected project-wide and made available as a genindex page, so documentation migrated from Sphinx keeps working the way its authors intended. Each term is also stamped onto the section it belongs to as a data attribute, independent of any genindex page. That's for tooling that crawls the rendered HTML rather than the reST source -- a custom search engine such as TYPO3's Elasticsearch integration can pick up the same terms an author already tagged with `index`, instead of reimplementing that logic itself. Closes #921 Assisted-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y48g2S9Q2uUVLcsTEAeyGX Signed-off-by: linawolf
Matches the ```rest convention already used by the other directive docblocks in this package (ErrorDirective, YoutubeDirective, etc.), per review feedback on PR #1358. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PP4LkejR5PSubbhNmF4RkT
GenIndexNodeRenderer, GenIndexRowNodeRenderer, and GenIndexTermNodeRenderer were all the same fixed node->template mapping, exactly what TemplateNodeRenderer (already used for UmlNode in guides-graphs) exists for. Register the three GenIndex node types through it instead of three bespoke classes, per review feedback on PR #1358. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PP4LkejR5PSubbhNmF4RkT
Drop isLink()/isSee()/isSeeAlso() (isSee() was unused) and switch the row template on node.kind.name directly, per review feedback on PR #1358. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PP4LkejR5PSubbhNmF4RkT
IndexCollectorPass collected entries, expanded Sphinx pair/triple/ module conventions, resolved see/seealso targets, filtered by scope, and built the node tree -- all as private methods of one 600-line class. Split into four single-purpose classes under a new Compiler/Passes/IndexCollector/ namespace: - IndexEntryCollector: walks documents and expands `.. index::` entries into a term map - GenIndexSeeResolver: points see/seealso rows at their target anchor - GenIndexTermMapFilter: scopes a term map to a `:scope:` path prefix - GenIndexNodeBuilder: turns a term map into the sorted node tree IndexCollectorPass is now a thin orchestrator over these four. Per review feedback on PR #1358 ("it feels like this class is doing too much"). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PP4LkejR5PSubbhNmF4RkT
Merging main pulled in the phpstan/phpstan 2 upgrade (#1366), which tightened several checks and turned up 12 errors on this branch's index-generation code: - GenIndexDirective/IndexDirective::process() declared a nullable return type they never actually return - IndexDirective's typo-detection loop carried a dead null check; IndexEntryType::cases() is never empty, so the loop always sets $closestType - GenIndexSeeResolver::resolveSeeRows() mutated $termMap in place via deep offset writes, which lost the 'term' key from PHPStan's tracked shape for 'subterms' entries; rebuilding each entry as a whole array literal keeps the shape precise - IndexEntryCollector::collectFromDocument() tagged flattened nodes with a string type list ('index'/'section'/'content') and matched on the string, so PHPStan couldn't narrow $node to IndexNode to call getEntries(); switched to an instanceof check instead Verified via Docker (php:8.4-cli, matching CI): phpstan clean, unit/ functional/integration suites green, phpcs clean, deptrac 0 violations. Fixes the failing "Static analysis / Static Code Analysis (8.2)" check on #1358. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PP4LkejR5PSubbhNmF4RkT
A #[phpstan-type] array shape only documents structure for static analysis, not runtime -- see https://www.garfieldtech.com/blog/array-type-hint. Per jaapio's feedback, replaces the GenIndexRowData/GenIndexTermData/ GenIndexTermMap phpstan-type aliases with real classes in the IndexCollector namespace: GenIndexRowData as a plain immutable DTO (matching ReferenceData/InterlinkData), GenIndexTermData as a small mutable collector object, GenIndexTermMap as an IteratorAggregate collection. Behavior is unchanged. Verified via Docker: phpstan/phpcs/deptrac clean, 949 tests green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PP4LkejR5PSubbhNmF4RkT Signed-off-by: lina.wolf
Rebasing onto main pulled in the compile-time createNode() dispatch IndexDirective and GenIndexDirective now use (main gained the #[Directive]/createNode() model, and an empty IndexDirective stub under this same name, after this PR's branch diverged). The tests called the old process(BlockContext, Directive) directly; switched to createNode(DirectiveNode). Signed-off-by: linawolf Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PP4LkejR5PSubbhNmF4RkT
714c6e5 to
973c11d
Compare
A changelog entry is the one page whose front matter says least about it:
its version is "main", because that is the only branch the Changelog is
deployed as. The release, the kind of change, the issue and the entry's
tags are all in the entry already -- in the path, in the anchor and in
the ".. index::" directive -- just nowhere a Markdown reader can see
them.
typo3-version: "14.0"
typo3-major: 14
type: "deprecation"
issue: 106393
forge: "https://forge.typo3.org/issues/106393"
tags: ["TCA", "FullyScanned", "ext:core"]
The tags are what made this worth doing: they are the scanner status and
the extension a change touches, and a consumer had to read the
reStructuredText source for them alone. Getting them needed a directive
of our own, because the library parses ".. index::" and returns null.
phpDocumentor/guides#1358 does that properly; ours goes when it lands.
Core Changelog only. Measured against it: every field on all 3402
entries, bar the tags of the one entry whose source has no index
directive.
One field is renamed for every manual, not just this one. "modified" is
the moment the manual was built -- the same value on all of its pages --
so it claimed that every entry had changed on every run, while a
changelog entry hardly ever changes after publication. It is "rendered"
now.
Signed-off-by: lina.wolf
Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019EuSAzF5qw81NzPusWy2hC
…1421) A changelog entry is the one page whose front matter says least about it: its version is "main", because that is the only branch the Changelog is deployed as. The release, the kind of change, the issue and the entry's tags are all in the entry already -- in the path, in the anchor and in the ".. index::" directive -- just nowhere a Markdown reader can see them. typo3-version: "14.0" typo3-major: 14 type: "deprecation" issue: 106393 forge: "https://forge.typo3.org/issues/106393" tags: ["TCA", "FullyScanned", "ext:core"] The tags are what made this worth doing: they are the scanner status and the extension a change touches, and a consumer had to read the reStructuredText source for them alone. Getting them needed a directive of our own, because the library parses ".. index::" and returns null. phpDocumentor/guides#1358 does that properly; ours goes when it lands. Core Changelog only. Measured against it: every field on all 3402 entries, bar the tags of the one entry whose source has no index directive. Signed-off-by: lina.wolf Co-authored-by: lina.wolf <>
…#1421)
A changelog entry is the one page whose front matter says least about
it:
its version is "main", because that is the only branch the Changelog is
deployed as. The release, the kind of change, the issue and the entry's
tags are all in the entry already -- in the path, in the anchor and in
the ".. index::" directive -- just nowhere a Markdown reader can see
them.
typo3-version: "14.0"
typo3-major: 14
type: "deprecation"
issue: 106393
forge: "https://forge.typo3.org/issues/106393"
tags: ["TCA", "FullyScanned", "ext:core"]
The tags are what made this worth doing: they are the scanner status and
the extension a change touches, and a consumer had to read the
reStructuredText source for them alone. Getting them needed a directive
of our own, because the library parses ".. index::" and returns null.
phpDocumentor/guides#1358 does that properly; ours goes when it lands.
Core Changelog only. Measured against it: every field on all 3402
entries, bar the tags of the one entry whose source has no index
directive.
Signed-off-by: lina.wolf
Co-authored-by: lina.wolf <>
…#1421)
A changelog entry is the one page whose front matter says least about
it:
its version is "main", because that is the only branch the Changelog is
deployed as. The release, the kind of change, the issue and the entry's
tags are all in the entry already -- in the path, in the anchor and in
the ".. index::" directive -- just nowhere a Markdown reader can see
them.
typo3-version: "14.0"
typo3-major: 14
type: "deprecation"
issue: 106393
forge: "https://forge.typo3.org/issues/106393"
tags: ["TCA", "FullyScanned", "ext:core"]
The tags are what made this worth doing: they are the scanner status and
the extension a change touches, and a consumer had to read the
reStructuredText source for them alone. Getting them needed a directive
of our own, because the library parses ".. index::" and returns null.
phpDocumentor/guides#1358 does that properly; ours goes when it lands.
Core Changelog only. Measured against it: every field on all 3402
entries, bar the tags of the one entry whose source has no index
directive.
Signed-off-by: lina.wolf
Co-authored-by: lina.wolf <>
jaapio
left a comment
There was a problem hiding this comment.
I have the feeling that the index terms should not be part of the section node. It's more like some kind of metadata that we want to add to the nodes? What if we would add this as a more generic node element?
If I see the rendering as a data element, and combine that with options I'm researching with https://pagefind.app/docs/metadata/ this could be very useful,
Maybe it makes sense to add some metadata property to the abstractNode, That will make it more generic.
public function addMetaData(DataNode $node)
The rendering should check the metadata for nodes it can handle. We could use a custom twig filter/function for this to render the data attributes on the nodes.
<h1 data-index="{{ renderIndexData(node) }}">
This new system will help us to add attributes to the html elements in a more generic way, allowing people to do even more, like what I try to do with the pagefind, to make static search possible.
| * document have been collected; at parse time it's an empty placeholder. | ||
| */ | ||
| #[Attributes\Directive(name: 'genindex')] | ||
| final class GenIndexDirective extends BaseDirective |
There was a problem hiding this comment.
the options scope and no-letter-index seem to be missing on this directive.
| * @link https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-index | ||
| */ | ||
| #[Attributes\Directive(name: 'index', rawContent: true)] | ||
| final class IndexDirective extends BaseDirective |
There was a problem hiding this comment.
The :name: option described in the linked sphinx docs seem to be missing?
Sphinx-authored documentation -- including TYPO3 Core's own Changelog
files -- relies on
.. index::entries so readers can jump straight toevery page a term is discussed on, the way a printed book's index does.
This library parsed those entries but threw them away, so porting such
docs here meant silently losing that navigation aid.
Entries are now collected project-wide and made available as a genindex
page, so documentation migrated from Sphinx keeps working the way its
authors intended.
Each term is also stamped onto the section it belongs to as a data
attribute, independent of any genindex page. That's for tooling that
crawls the rendered HTML rather than the reST source -- a custom search
engine such as TYPO3's Elasticsearch integration can pick up the same
terms an author already tagged with
index, instead of reimplementingthat logic itself.
Closes #921
Assisted-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01Y48g2S9Q2uUVLcsTEAeyGX
Signed-off-by: linawolf