Repository navigation
chore(php): namespace legacy includes under Sbpp\ + class_alias shims (#1290) - #1298
Merged
Merged
Conversation
…#1290) Phase B of issue #1290. Move every class under `web/includes/` to a `Sbpp\…` namespace matching its directory and emit a `class_alias(\Sbpp\…\X::class, 'X')` from each file so the legacy global names (`Database`, `CUserManager`, `Log`, `Api`, …) keep resolving for procedural call sites that haven't been migrated yet. What moved (file path + class name change): - includes/Database.php → includes/Db/Database.php (Sbpp\Db\Database) - includes/CUserManager.php → includes/Auth/UserManager.php (Sbpp\Auth\UserManager — drops the hungarian "C") - includes/AdminTabs.php → includes/View/AdminTabs.php (Sbpp\View\AdminTabs) - includes/Api.php → includes/Api/Api.php (Sbpp\Api\Api) - includes/ApiError.php → includes/Api/ApiError.php (Sbpp\Api\ApiError) - includes/auth/Auth.php → includes/Auth/Auth.php (Sbpp\Auth\Auth) - includes/auth/Host.php → includes/Auth/Host.php (Sbpp\Auth\Host) - includes/auth/JWT.php → includes/Auth/JWT.php (Sbpp\Auth\JWT) - includes/auth/openid.php → includes/Auth/openid.php (LightOpenID — third-party, stays in global ns) - includes/auth/handler/NormalAuthHandler.php → includes/Auth/Handler/NormalAuthHandler.php (Sbpp\Auth\Handler\NormalAuthHandler) - includes/auth/handler/SteamAuthHandler.php → includes/Auth/Handler/SteamAuthHandler.php (Sbpp\Auth\Handler\SteamAuthHandler) - includes/security/CSRF.php → includes/Security/CSRF.php (Sbpp\Security\CSRF) - includes/security/Crypto.php → includes/Security/Crypto.php (Sbpp\Security\Crypto) Namespaced in place (no rename): - includes/Log.php → Sbpp\Log - includes/Config.php → Sbpp\Config The `class_alias` shims live below each class declaration so the runtime aliases register the moment the file is required. The explicit `require_once` chain at the top of `web/init.php`, `web/tests/bootstrap.php`, and `web/phpstan-bootstrap.php` stays in place for that reason — `class_alias()` is a runtime call the PSR-4 autoloader cannot trigger on a global-name lookup. The three lists were rewritten to load the same 14 namespaced files in the same order so any class loaded in one but not the other (a latent regression) would surface immediately. New code consumes the namespaced names directly via `use Sbpp\Db\Database;` etc. Adjacent fixes pulled along: - The PHPStan service registrations that read `Sbpp\PhpStan\…` were fixed to `Sbpp\PHPStan\…` (the directory is `web/includes/PHPStan/`, PSR-4 case-sensitive). `phpstan.neon` and `phpstan-dba-bootstrap.php` match. - `phpstan.neon`'s `excludePaths` was updated for the `auth/openid.php` → `Auth/openid.php` rename, and `Sbpp\Db\Database::query#0` was added alongside the legacy `Database::query#0` to the SbppSyntaxErrorInQueryMethodRule watchlist. - `web/install/template/page.{2..5}.php`, `web/install/includes/converter.inc.php`, `web/pages/admin.settings.php`, `web/includes/View/AdminThemesView.php`, `web/includes/Mail/{Mail,Mailer}.php`, `web/includes/page-builder.php` picked up `use` imports / FQCN updates so the analyser sees the new namespaces from every entry point. - `web/themes/default/{core/admin_tabs.tpl, css/theme.css, page_admin_settings_*.tpl}` doc-comments updated to reference the new `web/includes/View/AdminTabs.php` path. - `web/phpstan-baseline.neon` shrank by 18 entries because the namespaced reflection resolves several previously-unsigned types (Mail/Mailer hand-offs, AdminTabs constructor types, etc.). Documentation: - `AGENTS.md` adds a "Stack at a glance" line, a "Namespacing" Conventions subsection (with a per-class table), and an Anti-pattern row pair: top-level `class Foo {}` in `web/includes/`, AND removing the eager `require_once` chain "now that PSR-4 exists". Existing references to `web/includes/auth/…` / `web/includes/security/…` / `Database` / `CUserManager` were retargeted to the namespaced shapes, with the legacy alias called out. - `ARCHITECTURE.md`'s "Web panel → Directory layout" tree was rewritten with the new paths + namespaced FQCNs, the bootstrap step now explains why the explicit require chain stays, and the "Auth" subsystem section was re-pointed at `Sbpp\Auth\*`. Verified: phpstan + phpunit + ts-check + composer api-contract all clean. A class-resolution smoke test confirms all 14 legacy global names resolve to their `Sbpp\…` FQCN counterparts (alias_resolves=yes across the board).
rumblefrog
pushed a commit
that referenced
this pull request
May 8, 2026
Phase K from #1290 — now unblocked by #1289 landing PHP 8.5 as the floor and #1298 landing the namespace move. Each sub-phase is small and surgical; the goal is the canonical sites the issue body calls out, not a "sprinkle 8.5 features everywhere" sweep. K.1 — `#[\NoDiscard]` (PHP 8.5) ----------------------------- Annotate methods whose return value is the meaningful signal so the PHPStan `method.resultDiscarded` rule fails the build on a discarded call site: - `Sbpp\Api\Api::redirect()` — the returned envelope IS the redirect. A bare `Api::redirect(...);` call (no `return`) silently no-ops the navigation while looking like it worked. - `Sbpp\Security\CSRF::validate()` — running the check and ignoring the bool is the textbook bug shape this attribute exists to catch. Callers either branch on the bool or use the higher-level `rejectIfInvalid()` helper. `Sbpp\Db\Database::execute()` was surveyed and intentionally deferred: PHPStan flags 9 sites the static gate can see, but the runtime gate (PHPUnit) catches another ≈40 discards in updater migrations / page handlers / API handlers that go through `$GLOBALS['PDO']` / `$this->dbs` (invisible to PHPStan). Adopting it requires the paired sweep, which belongs in its own PR — tracked as a follow-up in #1294. K.2 — Property hooks (PHP 8.4) ----------------------------- Surveyed; no current candidate. The codebase's getter methods (`UserManager::GetAid()`, `GetProperty()`, etc.) are simple delegators where a property hook would add measurable read overhead (~4x slower on tight loops) without paying for itself. Reach for hooks when there's actual compute inside the getter (lazy DB lookup, derived value caching, value validation on set). For plain stored data, `public readonly` is the right shape — engine-enforced single-write, faster than the equivalent hook. K.3 — Asymmetric visibility (PHP 8.4) ------------------------------------- Surveyed; no current candidate. `public private(set) readonly X $foo;` is indistinguishable from plain `public readonly X $foo;` (the engine enforces single-write in both shapes), so reach for `private(set)` only when there's a concrete multi-write internal flow. The codebase's read-mostly properties are all single-write, so plain `public readonly` is the right shape. K.4 — Pipe operator `|>` (PHP 8.5) --------------------------------- Adopted at the canonical site called out in the issue body: `web/pages/page.home.php`'s dashboard intro renders through `($raw ?? '') |> strval(...) |> IntroRenderer::renderIntroText(...)`. Reads left-to-right ("take the raw setting → coerce to string → render Markdown") vs the inside-out `IntroRenderer::renderIntroText((string)(Config::get(...) ?? ''))` shape. AGENTS.md "PHP 8.5 idioms" calls out the precedence pitfall (`|>` binds tighter than `??` — parenthesize the coalesce LHS). Documentation ------------- - AGENTS.md gains a "PHP 8.5 idioms (post-#1289 floor bump)" Conventions subsection covering all four features (two adopted, two declined-with-rationale + the precedence note). - AGENTS.md gains an Anti-pattern row for discarded return values from `Api::redirect()` / `CSRF::validate()`, pointing at the `method.resultDiscarded` PHPStan rule + the `#[\NoDiscard]` attribute. Verified: phpstan + phpunit + ts-check + composer api-contract all clean. No phpstan-baseline.neon delta. No wire-format change.
This was referenced May 8, 2026
shenhaihuixiang
pushed a commit
to shenhaihuixiang/sourcebans-pp
that referenced
this pull request
May 10, 2026
Phase K from sbpp#1290 — now unblocked by sbpp#1289 landing PHP 8.5 as the floor and sbpp#1298 landing the namespace move. Each sub-phase is small and surgical; the goal is the canonical sites the issue body calls out, not a "sprinkle 8.5 features everywhere" sweep. K.1 — `#[\NoDiscard]` (PHP 8.5) ----------------------------- Annotate methods whose return value is the meaningful signal so the PHPStan `method.resultDiscarded` rule fails the build on a discarded call site: - `Sbpp\Api\Api::redirect()` — the returned envelope IS the redirect. A bare `Api::redirect(...);` call (no `return`) silently no-ops the navigation while looking like it worked. - `Sbpp\Security\CSRF::validate()` — running the check and ignoring the bool is the textbook bug shape this attribute exists to catch. Callers either branch on the bool or use the higher-level `rejectIfInvalid()` helper. `Sbpp\Db\Database::execute()` was surveyed and intentionally deferred: PHPStan flags 9 sites the static gate can see, but the runtime gate (PHPUnit) catches another ≈40 discards in updater migrations / page handlers / API handlers that go through `$GLOBALS['PDO']` / `$this->dbs` (invisible to PHPStan). Adopting it requires the paired sweep, which belongs in its own PR — tracked as a follow-up in sbpp#1294. K.2 — Property hooks (PHP 8.4) ----------------------------- Surveyed; no current candidate. The codebase's getter methods (`UserManager::GetAid()`, `GetProperty()`, etc.) are simple delegators where a property hook would add measurable read overhead (~4x slower on tight loops) without paying for itself. Reach for hooks when there's actual compute inside the getter (lazy DB lookup, derived value caching, value validation on set). For plain stored data, `public readonly` is the right shape — engine-enforced single-write, faster than the equivalent hook. K.3 — Asymmetric visibility (PHP 8.4) ------------------------------------- Surveyed; no current candidate. `public private(set) readonly X $foo;` is indistinguishable from plain `public readonly X $foo;` (the engine enforces single-write in both shapes), so reach for `private(set)` only when there's a concrete multi-write internal flow. The codebase's read-mostly properties are all single-write, so plain `public readonly` is the right shape. K.4 — Pipe operator `|>` (PHP 8.5) --------------------------------- Adopted at the canonical site called out in the issue body: `web/pages/page.home.php`'s dashboard intro renders through `($raw ?? '') |> strval(...) |> IntroRenderer::renderIntroText(...)`. Reads left-to-right ("take the raw setting → coerce to string → render Markdown") vs the inside-out `IntroRenderer::renderIntroText((string)(Config::get(...) ?? ''))` shape. AGENTS.md "PHP 8.5 idioms" calls out the precedence pitfall (`|>` binds tighter than `??` — parenthesize the coalesce LHS). Documentation ------------- - AGENTS.md gains a "PHP 8.5 idioms (post-sbpp#1289 floor bump)" Conventions subsection covering all four features (two adopted, two declined-with-rationale + the precedence note). - AGENTS.md gains an Anti-pattern row for discarded return values from `Api::redirect()` / `CSRF::validate()`, pointing at the `method.resultDiscarded` PHPStan rule + the `#[\NoDiscard]` attribute. Verified: phpstan + phpunit + ts-check + composer api-contract all clean. No phpstan-baseline.neon delta. No wire-format change. Co-authored-by: cursor-agent <cursor-agent@cursor.local>
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
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
Phase B of #1290. Move every class under
web/includes/to aSbpp\…namespace matching its directory and emit aclass_alias(\Sbpp\…\X::class, 'X')from each file so the legacy global names (Database,CUserManager,Log,Api, …) keep resolving for procedural call sites that haven't been migrated yet.What moved
git mv)includes/Database.php→includes/Db/Database.phpSbpp\Db\Databaseincludes/CUserManager.php→includes/Auth/UserManager.phpSbpp\Auth\UserManager(drops the hungarian "C")includes/AdminTabs.php→includes/View/AdminTabs.phpSbpp\View\AdminTabsincludes/Api.php→includes/Api/Api.phpSbpp\Api\Apiincludes/ApiError.php→includes/Api/ApiError.phpSbpp\Api\ApiErrorincludes/auth/Auth.php→includes/Auth/Auth.phpSbpp\Auth\Authincludes/auth/Host.php→includes/Auth/Host.phpSbpp\Auth\Hostincludes/auth/JWT.php→includes/Auth/JWT.phpSbpp\Auth\JWTincludes/auth/handler/NormalAuthHandler.php→includes/Auth/Handler/NormalAuthHandler.phpSbpp\Auth\Handler\NormalAuthHandlerincludes/auth/handler/SteamAuthHandler.php→includes/Auth/Handler/SteamAuthHandler.phpSbpp\Auth\Handler\SteamAuthHandlerincludes/security/CSRF.php→includes/Security/CSRF.phpSbpp\Security\CSRFincludes/security/Crypto.php→includes/Security/Crypto.phpSbpp\Security\Cryptoincludes/auth/openid.php→includes/Auth/openid.phpLightOpenID(third-party — stays in global ns, also excluded from PHPStan)Namespaced in place (no rename):
includes/Log.phpSbpp\Logincludes/Config.phpSbpp\ConfigHow the legacy aliases stay alive
Each namespaced file emits a
class_alias(\Sbpp\…\X::class, 'X')below the class declaration. The runtime aliases register the moment the file is required, and the explicitrequire_oncechain at the top ofweb/init.php,web/tests/bootstrap.php, andweb/phpstan-bootstrap.phpstays in place specifically so thoseclass_aliascalls fire eagerly —class_alias()is a runtime call the PSR-4 autoloader cannot trigger on a global-name lookup.The three lists were rewritten to load the same 14 namespaced files in the same order. Asymmetry would be a latent regression: a class loaded by
phpstan-bootstrap.phpbut notinit.phpwould pass static analysis and die at runtime in any code path the autoloader hadn't already triggered.New code consumes the namespaced names directly via
use Sbpp\Db\Database;etc. A follow-up PR will burn theclass_aliasshims as call sites inweb/pages/*.php/web/api/handlers/*.php/web/updater/data/*.phpadopt the namespaced names.Adjacent fixes pulled along
Sbpp\PhpStan\…→Sbpp\PHPStan\…casing inphpstan.neonandphpstan-dba-bootstrap.php(the directory isweb/includes/PHPStan/, PSR-4 is case-sensitive).phpstan.neonexcludePathsupdated for theauth/openid.php→Auth/openid.phprename; addedSbpp\Db\Database::query#0alongsideDatabase::query#0to theSbppSyntaxErrorInQueryMethodRulewatchlist.web/install/template/page.{2..5}.php,web/install/includes/converter.inc.php,web/pages/admin.settings.php,web/includes/View/AdminThemesView.php,web/includes/Mail/{Mail,Mailer}.php,web/includes/page-builder.phppicked upuseimports / FQCN updates so the analyser sees the new namespaces from every entry point.web/themes/default/{core/admin_tabs.tpl, css/theme.css, page_admin_settings_*.tpl}doc-comments updated to reference the newweb/includes/View/AdminTabs.phppath.web/phpstan-baseline.neonshrank by 18 entries — namespaced reflection resolves several previously-unsigned types (Mail/Mailer hand-offs, AdminTabs constructor types, etc.).Documentation
AGENTS.md:class Foo {}inweb/includes/, AND removing the eagerrequire_oncechain "now that PSR-4 exists" (the latter explains exactly why dropping the chain breaks legacy global-name lookups).web/includes/auth/…/web/includes/security/…/ bareDatabase/CUserManagerretargeted to the namespaced shapes, with the legacy alias called out.ARCHITECTURE.md:require_oncechain stays.Sbpp\Auth\*.Test plan
./sbpp.sh phpstan— clean (0 errors over 225 files)../sbpp.sh test— 393 tests, 1692 assertions, all pass (the 1 PHPUnit deprecation is pre-existing inGroupsTest's docblock metadata, not introduced here)../sbpp.sh ts-check— clean../sbpp.sh composer api-contract— regenerates byte-identical (no diff)./,/index.php?p=banlist,/api.php) returns expected status codes.Database,CUserManager,Auth,JWT,CSRF,Crypto,Api,ApiError,AdminTabs,Host,NormalAuthHandler,SteamAuthHandler,Log,Config) resolve and their alias targets match theSbpp\…FQCNs.Stacked on top of #1295 (PR1 mechanical sweep) + #1296 (PR2 native types +
final class) + #1297 (PR3 backed enums), all of which are already onmain. Issue #1290.