Repository navigation
chore(cleanup): drop dead PHP fields rendering unreferenced JS calls (#1404) - #1407
Merged
Merged
Conversation
…cript 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.
…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).
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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
Closes #1404.
Five PHP page handlers built server-side data fields containing dead JS function calls that survived the v2.0
sourcebans.jsdeletion (#1123 D1). Three built dead fields that no template ever rendered (pure dead pollution). Two (admin.admins.php+admin.groups.php) actually emitted dead<script>blobs into the live DOM, throwingReferenceErrorper row on every page load (scaling with install size).Cleanup scope:
page.banlist.php:$data['friend_ban_link']/$data['unban_link']/$data['delete_link'](lines 858, 861, 862)page.commslist.php:$data['unban_link'](UnGag/UnMute switch arms) +$data['delete_link'](lines 679, 682, 688)page.home.php:$info['popup'](line 73) + the redundant<i>No nickname present</i>placeholder (page_dashboard.tpl:332already has the{if $p.short_name}{else}<span class="text-faint">no nickname</span>{/if}fallback)<script>blobs dropped:admin.admins.php:$serverscript(LoadServerHost('SID')per-server emission) + the{$server_script nofilter}echo inpage_admin_admins_add.tpl+ the orphanserver_scriptctor param onAdminAdminsAddViewadmin.groups.php: per-row<script>LoadServerHostPlayersList(...)</script>echo + the:prefix_servers_groupsSQL lookup that fed it (free perf win — one fewer DB roundtrip per server group per page load) + the placeholder copy + the<div id="servers_{gid}">slot inpage_admin_groups_list.tplRegression guard: new
web/tests/integration/DeadJsCallSitesTest.php(3 methods, 12 assertions) grep-asserts that none of the dead fields/blobs return. The helpers strip PHP + Smarty comments before grepping so the cleanup's own explanatory comments don't false-fire the gate.Follow-up issues filed for the proper hydration features that the dropped placeholders pointed at:
Sbpp\Servers\SourceQueryCache+server-tile-hydrate.js)Actions.GroupsServersList)AGENTS.md anti-pattern entries amended to cite #1404 as the precedent for the
LoadServerHost/LoadServerHostPlayersListremovals.Test plan
SmartyTemplateRulecross-check passes after theserver_scriptView property removal)DeadJsCallSitesTest)CI=1(267 passed / 279 skipped / 0 failed)Related
sourcebans.js