[BUGFIX] Read project version from the DOM so "0.10" is not coerced to 0.1 - #1345
[BUGFIX] Read project version from the DOM so "0.10" is not coerced to 0.1#1345CybotTM wants to merge 4 commits into
Conversation
ffab307 to
fcc475c
Compare
fcc475c to
7d907c6
Compare
6b70577 to
6503239
Compare
6503239 to
c921c2c
Compare
|
Thanks - I can't really judge the impact of this and hope @jaapio can give some feedback. I remember having stabbed at this and not being able to resolve this. |
f346063 to
c748708
Compare
c748708 to
555a67e
Compare
## Problem For a package with both `0.1` and `0.10` (e.g. [netresearch/nr-vault](https://docs.typo3.org/p/netresearch/nr-vault/0.10/en-us/)) the version switcher is wrong in two ways: - **`0.10` is sorted last** instead of first — it should appear right after `main`. - **Opening the `0.10` page pre-selects `0.1`** in the dropdown. ## Cause `versions.js` sorted by `parseFloat(v)`, and `parseFloat("0.10") === 0.1`, so `0.10` ties with the `0.x` group and lands at the bottom. Pre-selection compared against the rendered `data-current-version` attribute, which is `"0.1"` on the `0.10` page because the version string `"0.10"` is numerically coerced to `"0.1"` server-side (a separate, deeper bug in the render pipeline). ## Fix - Sort each dotted version component **numerically** (`main` first, then highest version), so `0.10` > `0.9` > … > `0.1`. - Derive the active version from the **page URL** (the authoritative source, e.g. `…/0.10/en-us/…`) instead of the coercible `data-current-version` attribute. This makes pre-selection correct regardless of that server-side coercion. `resources/public/js/theme.min.js` is rebuilt (grunt uglify). ## Before / after The version dropdown on the `nr-vault` `0.10` page: **Before**  **After**  ## Tests New `tests/js/versions.test.js` (vitest/jsdom) asserts both the sort order (`main, 0.10, 0.9, … 0.1`) and that the `0.10` page pre-selects `0.10`. Both **pass with the fix and fail without it**. ## Related The wrong pre-selection has a second, deeper cause that this PR does **not** rely on: the `0.10` page is served with `data-current-version="0.1"` (and title / meta `0.1`) because `guides.xml`'s `version="0.10"` is numerically coerced to the float `0.1` while parsing (Symfony `XmlUtils` phpize). That is tracked in #1293 and fixed upstream in phpDocumentor/guides#1345. This PR makes the switcher correct regardless — it derives the active version from the page URL — so the two fixes are independent. --------- Signed-off-by: Sebastian Mendel <info@sebastianmendel.de> Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
## Problem For a package with both `0.1` and `0.10` (e.g. [netresearch/nr-vault](https://docs.typo3.org/p/netresearch/nr-vault/0.10/en-us/)) the version switcher is wrong in two ways: - **`0.10` is sorted last** instead of first — it should appear right after `main`. - **Opening the `0.10` page pre-selects `0.1`** in the dropdown. ## Cause `versions.js` sorted by `parseFloat(v)`, and `parseFloat("0.10") === 0.1`, so `0.10` ties with the `0.x` group and lands at the bottom. Pre-selection compared against the rendered `data-current-version` attribute, which is `"0.1"` on the `0.10` page because the version string `"0.10"` is numerically coerced to `"0.1"` server-side (a separate, deeper bug in the render pipeline). ## Fix - Sort each dotted version component **numerically** (`main` first, then highest version), so `0.10` > `0.9` > … > `0.1`. - Derive the active version from the **page URL** (the authoritative source, e.g. `…/0.10/en-us/…`) instead of the coercible `data-current-version` attribute. This makes pre-selection correct regardless of that server-side coercion. `resources/public/js/theme.min.js` is rebuilt (grunt uglify). ## Before / after The version dropdown on the `nr-vault` `0.10` page: **Before**  **After**  ## Tests New `tests/js/versions.test.js` (vitest/jsdom) asserts both the sort order (`main, 0.10, 0.9, … 0.1`) and that the `0.10` page pre-selects `0.10`. Both **pass with the fix and fail without it**. ## Related The wrong pre-selection has a second, deeper cause that this PR does **not** rely on: the `0.10` page is served with `data-current-version="0.1"` (and title / meta `0.1`) because `guides.xml`'s `version="0.10"` is numerically coerced to the float `0.1` while parsing (Symfony `XmlUtils` phpize). That is tracked in #1293 and fixed upstream in phpDocumentor/guides#1345. This PR makes the switcher correct regardless — it derives the active version from the page URL — so the two fixes are independent. --------- Signed-off-by: Sebastian Mendel <info@sebastianmendel.de> Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
702a579 to
55ea0c7
Compare
|
This PR fixes trailing-zero loss (
yield 'project version as float' => [
[['project' => ['version' => 3.0]]],
static function (ContainerBuilder $container): void {
self::assertSame('3.0', $container->getParameter('phpdoc.guides.project.version'));
},
]; |
55ea0c7 to
2cfd577
Compare
09b2053 to
1eabeee
Compare
…0.10" XmlUtils::convertDomElementToArray() runs phpize() on every attribute value. That is right for the <guides> attributes that want coercion - links-are-relative, max-menu-depth - and wrong for <project>, whose four attributes are all strings: version="0.10" arrives as the float 0.1 and version="1.0" as the int 1. The digit is gone before anything downstream can object. There was a workaround: write version="'3.0'" and let a beforeNormalization callback strip the quotes again. It runs after phpize has already discarded the digit, so it can restore a quoted 3.0 but never an unquoted 0.10. Read the attributes straight from the DOM instead and detach <project> before the conversion, so phpize never sees them. Files that adopted the quoted workaround must keep rendering 3.0 rather than the literal '3.0', so the single quotes are still stripped - here, where the quotes come from, and for version and release only. Two details the code cannot show on its own. The element is looked up among the direct children: the schema lets an <extension> carry arbitrary children, and a subtree search would take a <project> nested in one for the project configuration and remove it from that extension. And detaching <project> can leave a root with no children at all, for which convertDomElementToArray() returns null rather than an empty array; that is handled instead of asserted, so the outcome does not depend on `zend.assertions`. Three fixtures, guarding different things. version-from-guides-xml now uses the unquoted 0.10 it could not express before and version-from-guides-xml-nested-project pins the direct-child lookup; both fail against main. version-from-guides-xml-quoted cannot: the quoted form worked before, which is the whole point of it. It guards the other direction and fails as soon as the quote stripping is dropped. Reported downstream at TYPO3-Documentation/render-guides#1175 Assisted-by: claude-code:claude-opus-5 Agent-Session: https://claude.ai/code/session_014H1xwaAmrQRWUA3vx8bJcD Agent-Host: 0493f0 Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
… path The beforeNormalization callback on project.version and project.release did two unrelated jobs. One undid the quote workaround against phpize, which now belongs to XmlFileLoader and moved there with the previous commit. The other turned a non-string, non-int value into its literal via var_export, and that half serves every caller of this tree, not the XML one: ContainerFactory::loadExtensionConfig() feeds a raw PHP array straight in. A native float then reaches `(string) $config['project']['version']` unguarded, and `(string) 3.0` is "3" - the same lost trailing zero, reached without any XML at all. Keep that half, drop the quote stripping, and state both nodes once instead of twice. null now passes through rather than becoming the literal "NULL". The old callback turned it into that string, the isset() below then saw a configured version, and the page rendered "version NULL". An explicit null is how a caller says "not configured", so ProjectSettings keeps its default. Scope, stated rather than implied: title and copyright take the same bare (string) cast on the same path and are not guarded, matching what the callback covered. Only version and release carry the "always a string" contract and a real-world float shape. The other two writers of a version - ComposerSettingsLoader and VersionFieldListItemRule - already guard with is_string, so this tree was the only gap. GuidesExtensionTest gains the cases. The float one fails without this commit with '3' against '3.0', the null one with 'NULL' against ''. The assertion reads the ProjectSettings back off the SettingsManager definition's method call, because the version never becomes a container parameter. Assisted-by: claude-code:claude-opus-5 Agent-Session: https://claude.ai/code/session_014H1xwaAmrQRWUA3vx8bJcD Agent-Host: 0493f0 Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
… issue
A baseline entry pins a line number in third-party code, and for one shape of
issue no line number can ever match. The entry for
vendor/symfony/config/Loader/FileLoader.php is that case, which the next commit
would otherwise turn the (8.4, lowest) CI cell red on:
PHPUnit\Runner\Baseline\FileDoesNotHaveLineException:
File "vendor/symfony/config/Loader/FileLoader.php" does not have line 0
Declaring a subclass of that vendor class makes PHP 8.4 raise the implicit
nullable deprecations of the inherited signatures while linking. Those are
attributed to the parent file with no line, so PHPUnit reports the issue at line
0; the baseline looks the file up, tries to read line 0 to hash it, and throws.
The suites that load the same class today do not hit it, and what spares them was
not established - only that the suite is not the difference.
Reproduced in a php:8.4-cli container after `composer update --prefer-lowest`,
then narrowed by removing one baseline block at a time; removing this one is what
clears it. Moving the offending test to another suite does not - verified with
the same test under `functional`.
The deprecations this entry covered are now displayed, and
`failOnDeprecation="false"` keeps them from failing anything. The other 39 vendor
files in the baseline carry the same fragility; nothing observed justifies
touching them here.
Assisted-by: claude-code:claude-opus-5
Agent-Session: https://claude.ai/code/session_014H1xwaAmrQRWUA3vx8bJcD
Agent-Host: 0493f0
Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
guides-cli's unit suite covers Command and Logger only, so the config loader is reachable exclusively through fixtures that render a whole site. Those prove what a reader sees; they cannot cheaply reach the shapes an author is unlikely to write while the loader handles them explicitly. XmlFileLoaderTest pins eight: a version read as written, a trailing zero kept in release as well, the quotes stripped from version and release, the same quotes kept on every other attribute, an attribute-less <project>, a file whose only child is <project>, a <project> nested in an <extension> not being taken for the project, and a file without a <project> not gaining an empty one. The third and fourth are the pair worth stating: the stripping is backward compatibility for two attributes, not a general unquoting rule, so a title that really is quoted keeps its quotes. Nothing covered that. Each case was seen failing on a defect built for it - DOM reading removed, trim widened to every attribute, trim dropped, the direct-child search degraded to a subtree search, the project key withheld when empty, and the project key set unconditionally. Every case appears in at least one failure list. Two appeared in none until the assertion was split: `?? []` reported a missing project config and an empty one as the same state. Assisted-by: claude-code:claude-opus-5 Agent-Session: https://claude.ai/code/session_014H1xwaAmrQRWUA3vx8bJcD Agent-Host: 0493f0 Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
1eabeee to
2df114f
Compare
|
After some back-and-forth, I haven't managed to significantly reduce the scope of this PR. I’ve tried to structure the PR logically based on the commits, hopefully, that makes the review easier. |
|
Small note, I noticed that the referenced issue is pointing to something different. Which makes it harder to see if this was an issue. Given the fact code changes are made I assume there was an issue. But I could not find it. If you have the correct issue I can read up from the beginning. I will continue to review your changes step by step. |
|
Hi @jaapio , thanks for taking care of.
my fault, I mixed something while working on different issues. Correct: TYPO3-Documentation/render-guides#1293 you can see the issue here for example: https://docs.typo3.org/p/netresearch/nr-llm/0.10/en-us/
|
The stacked-PR material covers merge order, retargeting and lost approvals - everything that happens once the stack exists. Two things that decide whether it can exist at all were missing. A pull request's base must be a branch in the repository it targets. Contributing from a fork, the base branch lives in the fork, so `gh pr create --base` answers "Base ref must be a branch" and there is no stack to be had. One API call on .permissions.push settles it before the branch is built. And a split is only real if both halves stand on their own. A test and the CI change that lets it pass read like two things and are one; where a diagnosed CI failure has one half causing it and the other fixing it, the question is already answered and the split cannot be delivered. Both were paid for on the same day: a split proposed for phpDocumentor/guides#1345, built as a branch and a PR, then reversed - the halves were inseparable, and the follow-up could not have been stacked upstream anyway. Assisted-by: claude-code:claude-opus-5 Agent-Session: https://claude.ai/code/session_014H1xwaAmrQRWUA3vx8bJcD Agent-Host: 0493f0 Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>

Problem
A
<project version="0.10">inguides.xmlrenders as version0.1, andversion="1.0"as1. The digit is lost before anything downstream can object.Cause
XmlUtils::convertDomElementToArray()runsphpize()on every attribute value. That is right for the<guides>attributes that want coercion —links-are-relative,max-menu-depth— and wrong for<project>, whose four attributes are all strings.There was a workaround: write
version="'3.0'"and let abeforeNormalizationcallback strip the quotes again. It runs afterphpizehas already discarded the digit, so it can restore a quoted3.0but never an unquoted0.10.Fix
Read the
<project>attributes straight from the DOM and detach the element before the conversion, sophpizenever sees them. The version is correct at the source instead of being coerced and patched up afterwards.Two consequences that are easy to miss:
<extension>carry arbitrary children, and a subtree search would take a<project>nested in one for the project configuration and remove it from that extension.<project>can leave a root with no children, for whichconvertDomElementToArray()returnsnullrather than an empty array. That is handled rather than asserted, so the outcome does not depend onzend.assertions.The config tree keeps the other half of the removed callback. It preserved the literal of a non-string value, and that half serves every caller, not the XML one:
ContainerFactory::loadExtensionConfig()feeds a raw PHP array straight in, where a native float reaches a bare(string)cast and(string) 3.0is"3"— the same lost zero, reached without any XML.nullnow passes through instead of becoming the literal string"NULL".Scope, stated rather than implied:
titleandcopyrighttake the same cast on the same path and are not guarded, matching what the callback covered. Onlyversionandreleasecarry the "always a string" contract and a real-world float shape. The other two writers of a version —ComposerSettingsLoaderandVersionFieldListItemRule— already guard withis_string.Backward compatibility
Projects that adopted the quoted workaround may still have
<project version="'3.0'">. To keep those rendering3.0and not the literal'3.0', the single quotes are still stripped — now inXmlFileLoader, where the quotes come from, and forversionandreleaseonly. New files need nothing: writeversion="0.10".Before / after
Tests
Three integration fixtures, guarding different things.
version-from-guides-xmluses the unquoted0.10it could not express before, andversion-from-guides-xml-nested-projectpins the direct-child lookup; both fail againstmain.version-from-guides-xml-quotedcannot fail againstmain— the quoted form worked there, which is its whole point. It guards the other direction and fails as soon as the quote stripping is dropped.XmlFileLoaderTestis new:guides-cli's unit suite coveredCommandandLoggeronly, so the loader was reachable exclusively through fixtures that render a whole site. Eight cases, each seen failing on a defect built for it — DOM reading removed,trimwidened to every attribute,trimdropped, the direct-child search degraded to a subtree search, the project key withheld when empty, and the project key set unconditionally.The case with no coverage anywhere: the quote stripping is backward compatibility for two attributes, not a general unquoting rule — a
titlethat really is quoted keeps its quotes.GuidesExtensionTestgains the programmatic-path cases; the float one fails without the fix with'3'against'3.0', the null one with'NULL'against''.One baseline entry had to go
Adding a unit test for the loader turned the
(8.4, lowest)cell red on an error that is not an assertion:Declaring a subclass of that vendor class makes PHP 8.4 raise the implicit-nullable deprecations of the inherited signatures while linking. Those are attributed to the parent file with no line, so PHPUnit reports the issue at line 0; the baseline looks the file up, tries to read line 0 to hash it, and throws. A baseline entry pins a line number in third-party code, and for this shape of issue no line number can match.
Reproduced in a
php:8.4-clicontainer aftercomposer update --prefer-lowest, then narrowed by removing one baseline block at a time. Moving the test to another suite does not help — verified with the same test underfunctional. The deprecations the entry covered are now displayed, andfailOnDeprecation="false"keeps them from failing anything. The other 39 vendor files in the baseline carry the same fragility and are worth a separate look.Verified
Four commits, each green on its own (831 → 834 → 834 → 842 tests). On the locked set: PHPUnit 842, PHPStan level max, PHPCS and deptrac clean, and all 100
guides.xmlin the tree validate against the schema. In aphp:8.4-clicontainer with--prefer-lowest: unit 495, functional 117, integration 230.Context
Assisted by claude-code:claude-opus-5 — Session