Skip to content

Commit cd65fca

Browse files
authored
chore(cleanup): drop dead PHP fields rendering unreferenced JS calls (#1404) (#1407)
* chore(cleanup): drop dead v1.x PHP fields + ReferenceError-throwing script blobs (#1404) Five page handlers built server-side data fields (or `<script>` blobs) containing dead JS function calls that survived the v2.0 `sourcebans.js` deletion (#1123 D1). Two cleanup shapes: Shape 1 — inert dead fields (no template consumed them): * `page.banlist.php:858-862` — drop `$data['friend_ban_link']` / `$data['unban_link']` / `$data['delete_link']` (v1.x `CreateLinkR`-built `BanFriendsProcess` / `UnbanBan` / `RemoveBan` `<a onclick=…>` blobs). Modern unban / delete affordances ride `.row-actions` with `data-action="bans-unban"` / `data-action="bans-delete"` in `page_bans.tpl`. * `page.commslist.php:679-688` — drop `$data['unban_link']` (UnGag / UnMute switch arms) + `$data['delete_link']` (RemoveBlock blob). Same story, mirror affordances in `page_comms.tpl`. * `page.home.php:73` — drop `$info['popup']` (`ShowBox(...)` blob the dashboard never echoed). The dashboard reads `short_name` / `search_link` / `bid` / `sname` / `blocked_human` instead; the no-nickname placeholder string that fed the popup goes with it. Shape 2 — live dead `<script>` blobs that raised ReferenceErrors: * `admin.admins.php:132-140` — drop the `$serverscript` blob that emitted `LoadServerHost('SID', 'id', 'saSID');` per server (every Add Admin page load raised `ReferenceError: LoadServerHost is not defined` once per configured server with no visible effect — the `sa<SID>` span already renders bare `IP:port` from `$server_list`). Paired drops: - `AdminAdminsAddView.php`: remove the orphan `server_script` readonly property (SmartyTemplateRule would fail otherwise). - `page_admin_admins_add.tpl:224`: drop the `{$server_script nofilter}` echo. * `admin.groups.php:166-168` — drop the per-row `<script>LoadServerHostPlayersList('<sids>', 'id', 'servers_<gid>')</script>` echo (every page load raised `ReferenceError: LoadServerHostPlayersList is not defined` once per server group). The `:prefix_servers_groups` lookup that fed the SID list goes too (nothing else reads it). Paired drops: - `page_admin_groups_list.tpl:341-343`: drop the literal "Servers populate via the legacy `LoadServerHostPlayersList` hook." placeholder copy (admin-facing forever pre-fix) + the sibling `<div id="servers_{gid}">` hydration slot. The two hydration features (Add Admin's Server-access multi-select; Server-Groups card's per-group server list) are filed as separate follow-up feature tickets per the issue body's recommendation — mixing cleanup with new feature wiring would inflate the PR review surface. Regression guard: `web/tests/integration/DeadJsCallSitesTest.php` (three methods, 12 assertions). The per-file forbidden-substring list is scanned against the **comment-stripped** source of each page handler — `php_strip_whitespace()` drops the explanatory `// #1404 — ...` markers so they don't false-fire the gate. Sibling cross-checks pin the template halves (`{$server_script}` echo, hydration slot, placeholder copy) and the View DTO (`AdminAdminsAddView::server_script`). A future "convenient" reintroduction (a fork pasting back the v1.x row-action shape) is caught at PR time rather than landing as another silent `ReferenceError` flood. Ties the existing AGENTS.md "Anti-patterns" entries (legacy 1.4.11 JS handler names, sourcebans.js bulk file) to the code. No user-visible behavior change. Sister #1402 / #1403 cover the JS-handler wiring rewrites (RemoveAdmin trash-icon, the `ShowBox(...)` toast emission); those scopes deliberately stay out of this PR. * chore(cleanup): drop defensive $info reset + cite #1404 in AGENTS.md (#1404 review) Adversarial-review feedback follow-ups on the #1404 cleanup. No behaviour change — pure deletion + docs. - web/pages/admin.admins.php: drop the per-iteration `$info = []` reset inside the `foreach ($servers as $server)` loop. Pre-review self-report flagged this as a "latent state-leak fix"; the reviewer verified under `E_ALL` that the loop assigns every key (`sid` / `ip` / `port`) on every iteration AND `array_push($server_list, $info)` is value-copy for PHP arrays, so prior-iteration values are fully overwritten before the next push. The reset added zero observable behaviour change and muddied what should be a pure-deletion PR. - AGENTS.md: amend the `web/scripts/sourcebans.js` anti-pattern entry (lines 2136-2156 after the edit) to cite both #1404 surfaces (the trailing per-row `<script>LoadServerHost('SID', …)</script>` echo on the Add Admin form AND the per-group `<script>LoadServerHostPlayersList('<sids>', …)</script>` echo on the Server Groups list), name the regression guard (`DeadJsCallSitesTest`), and forward-reference the two hydration follow-ups (#1405 + #1406). Add a sister row to the "Where to find what" regression-guard cluster (next to `Php82DeprecationsTest` and `SrvAdminsPdoParamTest`) so an archaeologist searching for "what test stops dead JS call sites from coming back" finds it on the first index lookup. Mandated by the "Keep the docs in sync" table's "Remove a legacy pattern" row. No source code changed beyond the one-line `$info` reset removal; every quality gate stays green (PHPStan 240 files / 0 errors, PHPUnit 680 tests / 2710 assertions, ts-check clean, API contract no-diff, Playwright E2E at CI=1 = 267 passed / 279 skipped / 0 failed).
1 parent f97b4eb commit cd65fca

10 files changed

Lines changed: 387 additions & 52 deletions

File tree

‎AGENTS.md‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2138,7 +2138,22 @@ contacting every contributor individually.
21382138
→ removed at v2.0.0 (#1123 D1). Page-tail helpers are inlined as
21392139
self-contained vanilla JS per page (see `web/pages/admin.edit.ban.php`
21402140
/ `admin.edit.comms.php` for canonical examples); toasts go through
2141-
`window.SBPP.showToast` from the theme JS.
2141+
`window.SBPP.showToast` from the theme JS. The trailing per-row
2142+
`<script>LoadServerHost('SID', …)</script>` echo on the Add Admin
2143+
form (`admin.admins.php`) and the per-group
2144+
`<script>LoadServerHostPlayersList('<sids>', …)</script>` echo on
2145+
the Server Groups list (`admin.groups.php`) survived the v2.0
2146+
cutover and raised one `ReferenceError` per server / per server
2147+
group on every page load with no visible effect (the static `<span
2148+
id="saSID">IP:port</span>` fallback / the literal "Servers populate
2149+
via the legacy LoadServerHostPlayersList hook." placeholder masked
2150+
the symptom). Both went at #1404 along with the orphan
2151+
`AdminAdminsAddView::server_script` ctor param and the
2152+
`<div id="servers_{gid}">` hydration slot; `DeadJsCallSitesTest` is
2153+
the per-file forbidden-substring gate so a fork pasting back the
2154+
v1.x shape gets caught at PR time. Hydration follow-ups tracked at
2155+
#1405 (Add Admin per-server access list) / #1406 (Server Groups
2156+
per-group server cards).
21422157
- `onclick="if (typeof <Helper> === 'function') <Helper>(...)"`
21432158
legacy-helper presence guards in templates (the v1.x sourcebans.js
21442159
defensiveness pattern that survived the #1123 D1 deletion of the
@@ -2825,6 +2840,7 @@ contacting every contributor individually.
28252840
| Action -> permission lock | `web/tests/api/PermissionMatrixTest.php` |
28262841
| Trap PHP 8.1 null-into-scalar deprecations at runtime (the bits PHPStan can't see) | `web/tests/integration/Php82DeprecationsTest.php` (#1273) — process-isolated render harness with a stub Smarty + `set_error_handler` that promotes `E_DEPRECATED` / `E_USER_DEPRECATED` to `\ErrorException`. Mirrors the LostPasswordChromeTest stub-Smarty pattern; each test method runs in a separate process because the page handlers declare top-level helpers (`setPostKey()` etc.) that PHP can't redeclare in one process. Add a marquee route here whenever a new high-traffic page handler ships, especially if it reads nullable `:prefix_*` columns or `$_POST` / `$_GET` lookups. |
28272842
| Pin the "every `:name` PDO placeholder needs as many `bind()` calls as occurrences" contract under native prepares | `web/tests/integration/SrvAdminsPdoParamTest.php` (#1314) — 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`; this is the contract pin (also a regression guard if anyone re-flips `EMULATE_PREPARES` back to `true`). `testAdminSrvadminsPageRendersWithoutPdoException` is the page-level regression guard for the actual #1314 fatal — process-isolated `require` of `pages/admin.srvadmins.php` with `?id=0` asserting no `PDOException` escapes. Mirrors the Php82DeprecationsTest stub-Smarty + process-isolation shape. |
2843+
| Pin "dead v1.x JS call sites + their server-side feeders stay deleted" (the `<script>LoadServerHost(...)</script>` / `$data['unban_link']` / `$info['popup']` shapes the v2.0.0 `sourcebans.js` cutover left without a JS landing site) | `web/tests/integration/DeadJsCallSitesTest.php` (#1404) — three methods, 12 assertions. Per-file forbidden-substring map (`page.banlist.php` / `page.commslist.php` / `page.home.php` / `admin.admins.php` / `admin.groups.php`) scanned against the **comment-stripped** source of each handler via `php_strip_whitespace()`, so the cleanup's own `// #1404 — ...` explanatory comments (and pre-existing historical-context comments that name the dead helpers in passing) don't false-fire the gate. The Smarty half (`{$server_script nofilter}`, the `<div id="servers_{$group.gid}">` hydration slot, the "Servers populate via the legacy LoadServerHostPlayersList hook." placeholder copy) is pinned by `testDeadTemplateSidesStayDropped` (also comment-stripped via `{* *}`-regex). The View-DTO side (`Sbpp\View\AdminAdminsAddView::server_script` orphan property) is pinned independently by `testAdminAdminsAddViewDoesNotCarryServerScriptProperty` so the failure points at the View directly instead of cascading through SmartyTemplateRule. Pure file scanning — extends `PHPUnit\Framework\TestCase` (no DB / session / Smarty bring-up). Sister #1402 (rewire dead JS handlers) and #1403 (ShowBox→`window.SBPP.showToast` rewrite) extend the per-file forbidden-substring map as their PRs land. |
28282844
| Add an E2E spec | `web/tests/e2e/specs/<smoke|flows|a11y|responsive>/...` + `web/tests/e2e/pages/...` |
28292845
| Add a route to the screenshot gallery | `web/tests/e2e/specs/_screenshots.spec.ts` (`ROUTES` array) |
28302846
| Tweak mobile (<=768px) chrome layout | `web/themes/default/css/theme.css` — see the `#1207` `@media (max-width: 768px)` blocks for the canonical shapes (icon-only topbar search, full-width drawer + scroll lock). Sub-paged admin routes (servers / mods / groups / comms / settings / admins / bans) use the `<details open>` accordion in the `#1259` `@media (min-width: 1024px)` block (sidebar inline at `<1024px`, sticky 14rem rail at `>=1024px`); see "Sub-paged admin routes" in Conventions. |

‎web/includes/View/AdminAdminsAddView.php‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ public function __construct(
3232
public readonly array $server_list,
3333
public readonly array $server_admin_group_list,
3434
public readonly array $server_group_list,
35-
public readonly string $server_script,
3635
) {
3736
}
3837
}

‎web/pages/admin.admins.php‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -128,16 +128,23 @@
128128
$servers = $GLOBALS['PDO']->query("SELECT * FROM `:prefix_servers`")->resultset();
129129
$server_admin_group_list = $GLOBALS['PDO']->query("SELECT * FROM `:prefix_srvgroups`")->resultset();
130130
$server_group_list = $GLOBALS['PDO']->query("SELECT * FROM `:prefix_groups` WHERE type != 3")->resultset();
131-
$server_list = [];
132-
$serverscript = "<script type=\"text/javascript\">";
131+
// #1404 — the pre-fix loop also built a `$serverscript` `<script>`
132+
// blob that emitted `LoadServerHost('sid', 'id', 'saSID');` per
133+
// server. The `LoadServerHost` helper was deleted with
134+
// `sourcebans.js` at #1123 D1, so every page load raised
135+
// `ReferenceError: LoadServerHost is not defined` once per
136+
// configured server with no visible effect (the `sa<SID>` <span>
137+
// already renders bare `IP:port` text from `$server_list`).
138+
// Hydration to per-server hostnames is the next step — tracked as
139+
// a follow-up ticket; see AGENTS.md "Anti-patterns" for the
140+
// matching `LoadServerHost` entry.
141+
$server_list = [];
133142
foreach ($servers as $server) {
134-
$serverscript .= "LoadServerHost('" . $server['sid'] . "', 'id', 'sa" . $server['sid'] . "');";
135143
$info['sid'] = $server['sid'];
136144
$info['ip'] = $server['ip'];
137145
$info['port'] = $server['port'];
138146
array_push($server_list, $info);
139147
}
140-
$serverscript .= "</script>";
141148

142149
\Sbpp\View\Renderer::render($theme, new \Sbpp\View\AdminAdminsAddView(
143150
// See AdminAdminsListView for why we don't splat Perms::for().
@@ -146,7 +153,6 @@
146153
server_list: $server_list,
147154
server_admin_group_list: $server_admin_group_list,
148155
server_group_list: $server_group_list,
149-
server_script: $serverscript,
150156
));
151157
echo '</div></div><!-- /.admin-sidebar-content + /.admin-sidebar-shell — opened by new AdminTabs(...) above -->';
152158
return;

‎web/pages/admin.groups.php‎

Lines changed: 14 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -137,12 +137,20 @@
137137
// ------------------------------------------------------------------
138138
// Server groups (`:prefix_groups` WHERE type = 3).
139139
//
140-
// `LoadServerHostPlayersList(...)` inline scripts are still emitted
141-
// for any third-party theme that forked the pre-v2.0.0 default and
142-
// renders an accordion-revealed server list per row. The shipped
143-
// template omits the accordion entirely (the marquee surface is the
144-
// master-detail flag grid above, not these server-of-servers
145-
// groupings) and ignores the script tags.
140+
// #1404 — the pre-fix loop also echoed a per-row
141+
// `<script>LoadServerHostPlayersList('<sids>', 'id', 'servers_<gid>')</script>`
142+
// blob meant to async-hydrate the per-group server list into the
143+
// matching `<div id="servers_{gid}">` slot. The helper was deleted
144+
// with `sourcebans.js` at #1123 D1, so every page load raised
145+
// `ReferenceError: LoadServerHostPlayersList is not defined` once
146+
// per server group AND left the slot showing the literal "Servers
147+
// populate via the legacy LoadServerHostPlayersList hook." copy
148+
// admin-facing forever. The echo + the slot + the placeholder copy
149+
// all went together; the `:prefix_servers_groups` lookup that fed
150+
// the SID list went too (nothing else read it). Hydration to per-
151+
// group server cards is the next step — tracked as a follow-up
152+
// ticket; see AGENTS.md "Anti-patterns" for the matching
153+
// `LoadServerHostPlayersList` entry.
146154
// ------------------------------------------------------------------
147155
$server_group_rows = $GLOBALS['PDO']->query("SELECT * FROM `:prefix_groups` WHERE type = '3'")->resultset();
148156
$server_list = [];
@@ -155,18 +163,6 @@
155163
$cnt = $GLOBALS['PDO']->single();
156164
$row['server_count'] = (int) $cnt['cnt'];
157165

158-
$GLOBALS['PDO']->query("SELECT server_id FROM `:prefix_servers_groups` WHERE group_id = :gid");
159-
$GLOBALS['PDO']->bind(':gid', $row['gid']);
160-
$servers_in_group = $GLOBALS['PDO']->resultset();
161-
162-
$server_arr = "";
163-
foreach ($servers_in_group as $server) {
164-
$server_arr .= $server['server_id'] . ";";
165-
}
166-
echo "<script>";
167-
echo "LoadServerHostPlayersList('" . $server_arr . "', 'id', 'servers_" . $row['gid'] . "');";
168-
echo "</script>";
169-
170166
$server_list[] = $row;
171167
$server_counts[] = $row['server_count'];
172168
}

‎web/pages/page.banlist.php‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -855,12 +855,15 @@ function setPostKey()
855855
// automatically (see the smarter-default block in admin.bans.php),
856856
// but we make it explicit + drop the dead fragment.
857857
$data['groups_link'] = CreateLinkR('<i class="fas fa-users fa-lg"></i> Show Groups', "index.php?p=admin&c=bans&section=group-ban&fid=" . urlencode($data['communityid']));
858-
$data['friend_ban_link'] = CreateLinkR('<i class="fas fa-trash fa-lg"></i> Ban Friends', '#', '', '_self', false, "BanFriendsProcess('" . $data['communityid'] . "','" . $data['player'] . "');return false;");
858+
// #1404 — `friend_ban_link`, `unban_link`, and `delete_link` were
859+
// v1.x `CreateLinkR`-built `BanFriendsProcess` / `UnbanBan` /
860+
// `RemoveBan` `<a onclick=…>` blobs; the helpers were deleted with
861+
// `sourcebans.js` at #1123 D1 and no v2.0+ template ever consumed
862+
// the fields. Modern unban / delete affordances live on the
863+
// `.row-actions` cell in `page_bans.tpl` (`data-action="bans-unban"`
864+
// / `data-action="bans-delete"`). Pinned by `DeadJsCallSitesTest`.
859865
$data['edit_link'] = CreateLinkR('<i class="fas fa-edit fa-lg"></i> Edit Details', "index.php?p=admin&c=bans&o=edit" . $pagelink . "&id=" . $row['ban_id'] . "&key=" . $_SESSION['banlist_postkey']);
860866

861-
$data['unban_link'] = CreateLinkR('<i class="fas fa-undo fa-lg"></i> Unban', "#", "", "_self", false, "UnbanBan('" . $row['ban_id'] . "', '" . $_SESSION['banlist_postkey'] . "', '" . $pagelink . "', '" . $data['player'] . "', 1, false);return false;");
862-
$data['delete_link'] = CreateLinkR('<i class="fas fa-trash fa-lg"></i> Delete Ban', "#", "", "_self", false, "RemoveBan('" . $row['ban_id'] . "', '" . $_SESSION['banlist_postkey'] . "', '" . $pagelink . "', '" . $data['player'] . "', 0, false);return false;");
863-
864867

865868
$data['server_id'] = $row['ban_server'];
866869

‎web/pages/page.commslist.php‎

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -674,18 +674,14 @@ function setPostKey()
674674

675675
$data['edit_link'] = CreateLinkR('<i class="fas fa-edit fa-lg"></i> Edit Details', "index.php?p=admin&c=comms&o=edit" . $pagelink . "&id=" . $row['ban_id'] . "&key=" . $_SESSION['banlist_postkey']);
676676

677-
switch ($data['type']) {
678-
case 2:
679-
$data['unban_link'] = CreateLinkR('<i class="fas fa-undo fa-lg"></i> UnGag', "#", "", "_self", false, "UnGag('" . $row['ban_id'] . "', '" . $_SESSION['banlist_postkey'] . "', '" . $pagelink . "', '" . $data['player'] . "', 1);return false;");
680-
break;
681-
case 1:
682-
$data['unban_link'] = CreateLinkR('<i class="fas fa-undo fa-lg"></i> UnMute', "#", "", "_self", false, "UnMute('" . $row['ban_id'] . "', '" . $_SESSION['banlist_postkey'] . "', '" . $pagelink . "', '" . $data['player'] . "', 1);return false;");
683-
break;
684-
default:
685-
break;
686-
}
687-
688-
$data['delete_link'] = CreateLinkR('<i class="fas fa-trash fa-lg"></i> Delete Block', "#", "", "_self", false, "RemoveBlock('" . $row['ban_id'] . "', '" . $_SESSION['banlist_postkey'] . "', '" . $pagelink . "', '" . $data['player'] . "', 0);return false;");
677+
// #1404 — the `unban_link` (UnGag/UnMute switch arms) and
678+
// `delete_link` fields were `CreateLinkR`-built `UnGag` / `UnMute`
679+
// / `RemoveBlock` `<a onclick=…>` blobs; the helpers were deleted
680+
// with `sourcebans.js` at #1123 D1 and no v2.0+ template ever
681+
// consumed the fields. Modern unmute / delete affordances live on
682+
// the `.row-actions` cell in `page_comms.tpl`
683+
// (`data-action="comms-unblock"` / `data-action="comms-delete"`).
684+
// Pinned by `DeadJsCallSitesTest`.
689685

690686
$data['server_id'] = $row['ban_server'];
691687

‎web/pages/page.home.php‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -66,11 +66,16 @@ function SbppServerQryHelpers(): string
6666
}
6767
$info['link_url'] = "window.location = '" . $info['search_link'] . "';";
6868

69-
// To print a name in the popup instead an empty string
70-
if (empty($cleaned_name)) {
71-
$cleaned_name = "<i>No nickname present</i>";
72-
}
73-
$info['popup'] = "ShowBox('Blocked player: " . $cleaned_name . "', '" . $cleaned_name . " tried to enter<br />' + document.getElementById('" . $info['server'] . "').title + '<br />at " . $info['date'] . "<br /><div align=middle><a href=" . $info['search_link'] . ">Click here for ban details.</a></div>', 'red', '', true);";
69+
// #1404 — `$info['popup']` carried a `ShowBox(...)` `<script>`
70+
// blob that v1.x stitched into each row's onclick. `ShowBox`
71+
// was deleted with `sourcebans.js` at #1123 D1 and no v2.0+
72+
// template ever consumed `{$player.popup}` (the dashboard
73+
// reads `short_name` / `search_link` / `bid` / `sname` /
74+
// `blocked_human` instead). The no-nickname placeholder
75+
// (`<i>No nickname present</i>`) that fed the popup string
76+
// went with it; the dashboard's `{if $p.short_name}…{else}no
77+
// nickname{/if}` arm renders the empty-name fallback. Pinned
78+
// by `DeadJsCallSitesTest`.
7479

7580
$GLOBALS['server_qry'] .= "__sbppLoadServerHostProperty(" . (int) $row['sid'] . ", 'block_" . (int) $row['sid'] . "_$blcount', 'title');";
7681

0 commit comments

Comments
 (0)