Repository navigation
Fix #1305: reject stale tier-1 version.json below v2.0 - #1321
Closed
rumblefrog wants to merge 1 commit into
Closed
rumblefrog wants to merge 1 commit into
rumblefrog wants to merge 1 commit into
Conversation
`Sbpp\Version::resolve()` previously trusted any `configs/version.json`
verbatim. The v1.x repo carried that file as a checked-in, hand-edited
literal (`{"version": "1.8.1", "git": "1434"}`) until #1070 deleted it
and made the release pipeline its sole writer. An operator who upgraded
v1.8 → v2.0 with a "skip if exists" overlay tool (FTP, `rsync` without
`--delete`, manual directory copy that treats `configs/` as user data)
keeps the stale file on disk; the resolver returned its contents
verbatim and the chrome footer read `SourceBans++ 1.8.1 | Git: 1434` on
a v2.0 install (with `data-version="1.8.1"` poisoning telemetry, bug
reports, and E2E specs that key off the attribute).
Add a `Version::MIN_TIER1_MAJOR = 2` floor and gate the tier-1 input
on it via a new `isAcceptableTier1Version()` helper. A version whose
major component is below the floor falls through to tier-2 (`git
describe`) or tier-3 (the `'dev'` sentinel) — the operator sees a
self-describing fallback instead of phantom v1.x copy.
Major-only comparison (vs. a strict `version_compare(..., '2.0.0',
'>=')`) is deliberate so pre-release tarballs like `2.0.0-rc.1` —
which sort BELOW `2.0.0` under semver's "pre-release < release" rule
— stay accepted. The constant carries a docblock that calls out the
"bump on every MAJOR release" maintenance burden, paired with a
`testFloorConstantIsTwo()` regression guard so the bump becomes a
deliberate test edit.
Pin every reject branch (stale v1.x with git available + with git
absent) and every accept branch (boundary `2.0.0`, pre-release
`2.0.0-rc.1`, future `3.0.0`, plus a malformed-version data provider
with six edge cases) in `web/tests/unit/VersionTest.php`. Document
the v2.0.0 file-ownership contract (and the recovery procedure for
already-upgraded installs) in `UPGRADING.md`; cross-link the floor
from `AGENTS.md` "Where to find what" and `ARCHITECTURE.md`'s
init.php lifecycle step 5.
Fixes #1305.
Member
Author
|
Not a bug, the user was testing straight from main. On normal path, the release will contain CI generated version.json. We don't need to support other paths of cloning from main. |
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
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.
Summary
Sbpp\Version::resolve()previously trusted anyconfigs/version.jsonverbatim. The v1.x repo carried that file as a checked-in, hand-edited
literal (
{"version": "1.8.1", "git": "1434"}) until #1070 deleted itand made the release pipeline its sole writer. An operator who
upgraded a v1.8 install to v2.0 with a "skip if exists" overlay tool
(FTP,
rsyncwithout--delete, manual directory copy that treatsconfigs/as user data alongsideconfigs/permissions/*.json) keepsthe stale file on disk; the resolver returned its contents verbatim
and the chrome footer read
SourceBans++ 1.8.1 | Git: 1434on a v2.0install (with
data-version=\"1.8.1\"poisoning telemetry, bugreports, and E2E specs that key off the attribute).
This PR ships the issue's suggested fix (2) — resolver-side floor
check — paired with the doc-only fix (3) for already-upgraded
in-the-wild installs. It does not ship the migration-side fix (1):
the migration could only delete the stale file, which would
regress correctly-overlaid installs (the new
version.jsonwouldbe wiped and the footer would read
dev); the resolver-side checkfails closed for the broken case without harming the working case.
Version::MIN_TIER1_MAJOR = 2and gate the tier-1 input onit via a new
isAcceptableTier1Version()helper. A versionwhose major component is below the floor falls through to tier-2
(
git describe) or tier-3 (the'dev'sentinel) — the operatorsees a self-describing fallback instead of phantom v1.x copy.
version_compare(..., '2.0.0', '>=')) is deliberate so pre-release tarballs like2.0.0-rc.1—which sort BELOW
2.0.0under semver's "pre-release < release"rule — stay accepted. The constant's docblock calls out the
"bump on every MAJOR release" maintenance burden, paired with a
testFloorConstantIsTwo()regression guard so the bump becomesa deliberate test edit.
absent) and every accept branch (boundary
2.0.0, pre-release2.0.0-rc.1, future3.0.0, plus a malformed-version dataprovider with six edge cases) in
web/tests/unit/VersionTest.php— bringing the suite from 6 to 18 tests, all passing.
procedure for already-upgraded installs) in
UPGRADING.md; cross-link the floor from
AGENTS.md"Where to find what" andARCHITECTURE.md's init.php lifecycle step 5.Fixes #1305.
Test plan
./sbpp.sh test tests/unit/VersionTest.php— 18 / 18 pass(6 pre-existing + 12 new across
testStaleV1JsonFallsThroughTo*,testTarballJsonAtFloorMajorIsAccepted,testTarballJsonPreReleaseWithinCurrentMajorIsAccepted,testTarballJsonFutureMajorIsAccepted, theprovideMalformedTier1Versionsdata provider, andtestFloorConstantIsTwo)../sbpp.sh test— 415 / 415 pass (the suite-wide PHPUnitdeprecation that remains is
GroupsTest::testEditRoundTripsHighBitFlagsAsPositiveIntegers's@dataProviderdoc-comment, pre-existing and out of scope)../sbpp.sh phpstan— clean (no errors at level 5 + dba).web/scripts/,web/themes/*/js/, orweb/api/handlers/touched, so
ts-check,composer api-contract, ande2earenot in the diff path. The chrome footer template
(
web/themes/default/core/footer.tpl) still mirrorsSB_VERSIONunchanged; the only behaviour change is what
SB_VERSIONresolvesto when
configs/version.jsoncarries a stale v1.x version.Reviewer notes
— see Summary for the regression-on-correctly-deployed-installs
reasoning. The resolver-side check + the doc note covers the
failure mode without harming the working case.
3when v3.0 ships;testFloorConstantIsTwo()is the paired regression guard soforgetting to bump it shows up as a failing test rather than a
silently re-opened bug. AGENTS.md's "Where to find what" row
carries the same note.