Repository navigation
Fix #1314: rename reused :sid placeholder in admin.srvadmins.php - #1328
Merged
Merged
Conversation
The admin-list SELECT in `pages/admin.srvadmins.php` mentioned the
named placeholder `:sid` twice (one in the outer
`WHERE server_id = :sid`, one in the inner subquery's
`WHERE server_id = :sid`) but called `bind(':sid', …)` only ONCE.
Under emulated prepares (PDO's pre-#1124 default), client-side
substitution rewrote every `:sid` occurrence to the literal value
before the SQL hit MariaDB, so the duplicate-name pattern Just
Worked. After #1124 / #1167 flipped `PDO::ATTR_EMULATE_PREPARES`
to `false` (so `LIMIT '0','30'` would stop tripping MariaDB strict
mode), the MySQL driver started expanding each `:name` occurrence
into its own positional `?` slot in the prepared statement —
`bind()`-ing one occurrence leaves the others unbound and
`execute()` raises `SQLSTATE[HY093] Invalid parameter number`.
Result: every admin with `ADMIN_LIST_SERVERS` who clicked
**Server Management → Admins** after upgrading to v2.0 hit a
page-load-blocking PHP fatal with no UI workaround.
The fix renames the inner placeholder to `:sid_inner` so each
position is bound separately. While here, also:
- Add the missing `global $userbank;` declaration. The page used
`$userbank` without declaring it global; this worked in
production because `core/header.php` runs first inside `build()`
and imports it into the function scope, but it was a latent bug
that broke the new test harness and the corresponding
`Variable $userbank might not be defined` baseline entry was
sitting in `phpstan-baseline.neon`. Drop the baseline entry now
that the page declares the global itself, matching every other
admin page.
- Lift `(int) $_GET['id']` into a local `$sid` so the value isn't
re-cast across the SELECT bind + the `checkMultiplePlayers` call,
and coalesce on `null` so the read survives PHP 9's stricter
null-into-scalar rules.
Regression coverage in `web/tests/integration/SrvAdminsPdoParamTest.php`
(two methods):
- `testReusedNamedPlaceholderUnderNativePreparesIsRejected` — issues
a tiny `SELECT 1 ... WHERE aid = :sid OR aid = :sid` against
`Sbpp\Db\Database` with one `bind()` and asserts it throws
`HY093`. Pins the contract; also a regression guard if anyone
re-flips `EMULATE_PREPARES` back to `true`.
- `testAdminSrvadminsPageRendersWithoutPdoException` — process-
isolated `require` of the page handler with `?id=0` asserts no
`PDOException` escapes. Pre-fix this test fails at
`Database.php:101` with the exact stack from the issue trace.
Docs updated in the same PR per AGENTS.md "Keep the docs in sync":
- `AGENTS.md` adds the `:name`-binding rule under the "Database"
convention, the matching "Anti-patterns" entry, and a
"Where to find what" row pointing at the new test.
- `ARCHITECTURE.md` documents the two practical consequences of
`EMULATE_PREPARES => false` for callers (binary-protocol numerics
AND each `:name` occurrence expanding to its own slot) under the
Database subsystem walkthrough.
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.
Fixes #1314.
Summary
The admin-list SELECT in
pages/admin.srvadmins.phpmentioned the named placeholder:sidtwice (the outerWHERE server_id = :sidand the inner subquery'sWHERE server_id = :sid) but calledbind(':sid', …)only ONCE. Under emulated prepares (PDO's pre-#1124 default) client-side substitution rewrote every:sidoccurrence to the literal value before the SQL hit MariaDB, so the duplicate-name pattern Just Worked. After #1124 / #1167 flippedPDO::ATTR_EMULATE_PREPAREStofalse(soLIMIT '0','30'would stop tripping MariaDB strict mode), the MySQL driver started expanding each:nameoccurrence into its own positional?slot —bind()-ing one occurrence leaves the others unbound andexecute()raisesSQLSTATE[HY093] Invalid parameter number. Result: every admin withADMIN_LIST_SERVERSwho clicked Server Management → Admins after upgrading to v2.0 hit a page-load-blocking PHP fatal with no UI workaround.What changed
web/pages/admin.srvadmins.php— rename the inner subquery's placeholder to:sid_innerand bind both, lift(int) $_GET['id']into a local$sid(now coalesced onnullfor PHP 9 forward-compat), and reuse it in thecheckMultiplePlayers($sid, …)call. While here, also add the missingglobal $userbank;declaration the page was relying oncore/header.phpto import as a side effect — clears the correspondingVariable $userbank might not be definedbaseline entry in the same diff.web/tests/integration/SrvAdminsPdoParamTest.php— new file. Two methods:testReusedNamedPlaceholderUnderNativePreparesIsRejected— issues a tinySELECT 1 ... WHERE aid = :sid OR aid = :sidagainstSbpp\Db\Databasewith onebind()and asserts it throwsHY093. Pins the contract; also a regression guard if anyone re-flipsEMULATE_PREPARESback totrue.testAdminSrvadminsPageRendersWithoutPdoException— process-isolatedrequireof the page handler with?id=0asserts noPDOExceptionescapes. Pre-fix this test fails atDatabase.php:101with the exact stack from the issue trace.AGENTS.md— adds the:name-binding rule under the "Database" convention, the matching "Anti-patterns" entry, and a "Where to find what" row pointing at the new test.ARCHITECTURE.md— documents the two practical consequences ofEMULATE_PREPARES => falsefor callers (binary-protocol numerics AND each:nameoccurrence expanding to its own slot) under the Database subsystem walkthrough.web/phpstan-baseline.neon— drop theVariable $userbank might not be definedentry forpages/admin.srvadmins.phpnow that the page declares the global itself.A regex scan of
web/pages/,web/api/handlers/,web/includes/,web/install/,web/updater/, andweb/tests/for queries that reuse the same:namemore than once found this:sidinadmin.srvadmins.phpto be the only true offender — every other "reuse" hit was the:prefix_<table>literal thatDatabase::setPrefix()rewrites BEFOREprepare()(and thus is harmless).Test plan
./sbpp.sh phpstan— passes (228 files, 0 errors). The cleared baseline entry stays consistent../sbpp.sh test— passes (405 tests, 1768 assertions, 0 errors / 0 failures; the one PHPUnit deprecation is a pre-existingGroupsTestdoc-comment metadata note unrelated to this PR)../sbpp.sh test --filter=SrvAdminsPdoParam— both methods green.:sid_inner→:sid+ drop thebind(':sid_inner', …)); re-ran the new test class —testAdminSrvadminsPageRendersWithoutPdoExceptionfails at exactlypages/admin.srvadmins.php:52withPDOException: SQLSTATE[HY093]: Invalid parameter number, matching the issue's stack. Restored the fix; both methods green again../sbpp.sh ts-check/./sbpp.sh composer api-contract/./sbpp.sh e2e— not run; this PR touches no JS, no API handlers, and no user-facing UI. The integration test covers the page render end-to-end.