[BUGFIX] Render a code block without a language instead of aborting the run - #1353
Conversation
Rendering a code block whose language is not set aborts the whole run with
CodeExtension::highlight(): Argument phpDocumentor#3 ($language) must be of type
string, null given
`CodeNode::getLanguage()` is nullable and `body/code/highlighted-code.html.twig`
passes it straight into the filter, so the filter has to accept null. Its
signature already carries the intended fallback, `string $language = 'text'`,
but a default cannot apply to an argument that is passed explicitly.
Accept `string|null` and fall back to `text`.
Reproduced with a `literalinclude` without the `:language:` option, which is
the shortest way to reach a CodeNode with no language; `code-block` never
does, because it always sets either the given language or the configured
default.
Assisted-by: claude-code:claude-opus-5
Agent-Session: https://claude.ai/code/session_015QXXkquh2eQNBiTYA39Wss
Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
4ad93d6 to
8eb950e
Compare
|
Thanks a lot for your Bug report and suggested solution, I think it would also be helpful to turn your reproduction instructions into an integration test. I am also wondering if it really makes sense to create a Code Node without language in the first place. In TYPO3 we try to guess the language by file ending and default to text and a warning if it cannot be auto detected / resolves to an unsupported language. That would however be something for a follow up |
|
One more thing: |
The HTML template wrote class="language-{{ node.language }}" without a fallback, so a
CodeNode with a null language rendered class="language-" while the highlight filter
rendered its content as text. Align the class attribute with that fallback.
Only null is mapped, matching the filter: an empty-string language keeps rendering
class="language-" as before.
The integration test drives the reported reproduction, a literalinclude without
:language:, through the CLI with the Code extension enabled, which is where the
TypeError was raised.
Markdown fenced code blocks without an info string also carry a null language; their
expectation changes from language- to language-text.
Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
|
Both points are in e5edfbb. Class attribute. Integration test. Both halves were mutated against the new fixture: reverting the filter signature fails it with the original One expectation changed as a consequence: I also checked On guessing the language from the file extension: agreed, and out of scope here — happy to open an issue for it if you want it tracked. Marked as draft while the empty-string question above is open. |
|
Thanks a lot for your contribution, yes an issue about the auto discovery of the language would be good |
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
…w count hide Three findings from an upstream review round on phpDocumentor/guides#1353, each with a direct TYPO3 counterpart. proving-a-test: a test that stays green after the fix is reverted may be testing nothing, because the harness never loaded the changed code — $coreExtensionsToLoad and $testExtensionsToLoad decide that silently. Check the wiring before the assertion. Second addition: count with git grep, since grep -r in a post-run checkout counts var/, typo3temp/ and .Build/ as source. gerrit-review-patterns: a reviewer names the site they saw. Enumerate the other producers and renderers of the same value, and say in the reply which ones were checked and left alone. evals: one scenario per finding. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
…w count hide (#35) Three findings from an upstream review round on [phpDocumentor/guides#1353](phpDocumentor/guides#1353). Each one has a direct TYPO3 counterpart, which is what makes them worth writing down here. **`proving-a-test.md` — when the mutation does not fail.** The procedure so far treats a green test after a reverted fix as a weak assertion. There is a second explanation that produces the same symptom: the harness never loaded the code under test. In TYPO3 that is `$coreExtensionsToLoad` / `$testExtensionsToLoad` — an extension missing from those lists is simply absent, with no error to say so. In the guides monorepo the same shape cost a review round: the existing `literalinclude` integration fixture had never enabled the optional highlighting package, so it rendered a fallback template and could not reach the crash it appeared to cover. Enabling the package in the fixture's own config was what made the reverted fix fail — with the exact `TypeError` from the bug report. **`proving-a-test.md` — counting occurrences.** `grep -r` in a checkout where the suite has just run counts `var/`, `typo3temp/`, `.Build/` and fixture output directories as source. A count taken that way was wrong by four, went into a public review comment, and had to be corrected afterwards. `git grep` sees tracked files only. **`gerrit-review-patterns.md` — one named occurrence, a whole class behind it.** A reviewer names the site their eye landed on. In #1353 the same missing fallback sat in a second template and a second producer carried the same null value, which moved an existing expectation file; implementing the comment verbatim would have fixed a third of its own case. The new theme asks for the enumeration and for the reply to name the sites that were checked and deliberately left alone — including the one that turned out to need no change. **Evals**: one scenario per finding, all three graded by `content_contains`. Checks run locally: `validate-evals.sh` 69 passed / 0 failed, `validate-skill.sh` 0 errors (9 pre-existing README warnings, untouched).
Problem
Rendering a code block whose language is not set aborts the entire run:
CodeNode::getLanguage()is nullable, andpackages/guides/resources/template/html/body/code.html.twig→body/code/highlighted-code.html.twigpasses it straight into the filter as{{ node.value|highlight(node.language) }}. The filter's signature already carries the intended fallback —string $language = 'text'— but a default value cannot apply to an argument that is passed explicitly, so null reaches a non-nullable parameter and PHP throws.Reproduction
The shortest way to reach a
CodeNodewithout a language is aliteralincludewithout:language::vendor/bin/guides run input --output=outon that project fails with theTypeErrorabove and renders nothing. Measured onmainat90594491, so this is not a regression from anything currently in flight.The project needs
<extension class="phpDocumentor\Guides\Code"/>in itsguides.xml, the way this repository's ownguides.xmlhas it. Without the extension the container keeps the fallback templatepackages/guides/resources/template/html/body/code/highlighted-code.html.twig, a bare{{ node.value }}, and the filter is never called.code-blockis not affected:CodeBlockDirectivealways callssetLanguage(), either with the given language or withgetCodeBlockDefaultLanguage(), which returns astring.Fix
CodeExtension::highlight()acceptsstring|nulland falls back totext, which is what the existing default already expressed.code.html.twigwritesclass="language-{{ node.language ?? 'text' }}", so the class attribute names the language the content is actually highlighted with. Without this the block rendered astextbut was labelledclass="language-".Only null is mapped, matching the filter. An empty-string language still renders
class="language-", which is what a literal block (::) and acode-blockwithout language produce today; the highlighter maps''to text as well, so normalizing that too would be consistent, but it rewrites ten further expectations across nine files and is a separate change.Tests
packages/guides-code/tests/unit/Twig/CodeExtensionTest.phpasserts that a null language renders exactly like an explicittext, plus one test that a real language still highlights.The fixture is
<a> & "b"on purpose: a highlighter returns plain input unchanged for every language, so a plain fixture would pass no matter what the fallback is. With this one,textleaves the quoted part alone whilephpwraps it in a string token, and a second assertion states that the two differ, so the fixture itself is pinned as discriminating. Two mutations were checked against it: removing the null acceptance fails with theTypeErrorabove, and changing the fallback tophpfails the equality assertion.tests/Integration/tests/code/literalinclude-without-languageruns the reproduction above through the CLI. Itsguides.xmlenables theCodeextension, because otherwise the fallback templatepackages/guides/resources/template/html/body/code/highlighted-code.html.twigrenders and the filter is never reached. Both halves of the fix were mutated against it: reverting the filter signature fails with the originalTypeErrorraised inbody/code/highlighted-code.html.twig, reverting the template fallback fails onlanguage-vslanguage-text.tests/Integration/tests/markdown/code-mdchanges: a fenced code block without an info string carries a null language too, so its two expectations move fromlanguage-tolanguage-text.The TeX template renders
\lstset{language={{ node.language }}}without a fallback as well, but null and''produce the same\lstset{language=}there, which is the committed expectation for a language-less block intests/Functional/tests/code/code.tex. There is nothing null-specific to fix there, so it is left unchanged.Local run of
phpunit(unit, functional, integration),phpcs,phpstananddeptrac: green.Note
There is a second, complementary option: let
LiteralincludeDirectivefall back togetCodeBlockDefaultLanguage()the wayCodeBlockDirectivedoes — the producer-side approach #1302 took for markdown fenced code blocks. I did not do that here because it would leave the crash reachable from any other producer, and because the type mismatch really is at the template-to-filter boundary. Happy to add the directive-side default as well if you prefer both.Assisted by claude-code:claude-opus-5 — Session