diff --git a/CHANGELOG.md b/CHANGELOG.md index b129226..0fdc51f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,20 @@ and this module adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0. ### Added +- `stubs/xoops.stub`: the XOOPS 2.7.3 error-screen ownership API — + `xoops_recordErrorScreenOwner()`, `xoops_getRecordedErrorScreenOwner()` and + `xoops_releaseErrorScreenOwner()`. Every error-screen provider calls these from its + install/update/uninstall hooks, and they were being hand-copied into each module's local + stub. Declared with the caveat attached: a `core27` profile describes the newest 2.7 API, + so a module whose `min_xoops` is below 2.7.3 must keep its `function_exists()` guards. +- `docs/hosting-a-library-that-grabs-global-handlers.md`: how to stop a Composer package + taking PHP's error handler, the session or the output buffer out from under XOOPS — + refuse the side effect at the library's injection seam rather than undoing it afterwards, + which breaks the library's own register/unregister pairing. +- README: an *Extending the stub* section, because declaring a class in the shared stub + converts one `class.notFound` into a `method.notFound` per undeclared method and quietly + invalidates matching baseline entries in every consuming module. + ### Changed ### Fixed diff --git a/README.md b/README.md index fa18443..27513f1 100644 --- a/README.md +++ b/README.md @@ -27,7 +27,8 @@ maintainers for the matching variant.) | `.github/ISSUE_TEMPLATE/`, `PULL_REQUEST_TEMPLATE.md`, `CODE_OF_CONDUCT.md`, `CONTRIBUTING.md`, `SECURITY.md`, `FUNDING.yml` | Community-health files. If your repo lives in an org that provides these centrally, you may delete the local copies to inherit the org-wide versions. | | `composer.json` | Module identity + the standard `require-dev` and `scripts`. **Rename** `name`/namespace for your module. | | `.php-cs-fixer.dist.php` | Coding-style rules (PSR-12 + XOOPS adjustments). The **only** style engine used. | -| `phpstan.neon.dist`, `phpstan-baseline.neon`, `phpstan-bootstrap.php`, `stubs/xoops.stub` | Static analysis config. The stub teaches PHPStan the core XOOPS classes (XoopsObject, handlers, Criteria, `Xmf\Module\Helper`) so analysis is meaningful without a full XOOPS runtime. | +| `phpstan.neon.dist`, `phpstan-baseline.neon`, `phpstan-bootstrap.php`, `stubs/xoops.stub` | Static analysis config. The stub teaches PHPStan the core XOOPS classes (XoopsObject, handlers, Criteria, `Xmf\Module\Helper`, the 2.7.3+ error-screen ownership functions) so analysis is meaningful without a full XOOPS runtime. See [Extending the stub](#extending-the-stub) before adding a class to it. | +| `docs/` | Guidance that outlives any one module. Currently: [Hosting a library that grabs global handlers](docs/hosting-a-library-that-grabs-global-handlers.md) — how to keep a Composer package from taking PHP's error handler, the session or the output buffer out from under XOOPS. | | `rector.php` | Automated refactoring, pinned to the target PHP level (only scans directories that exist). Ships the **`xoops/rector-xoops`** rule set (`XoopsSetList::XOOPS`) so XOOPS-specific modernisation (DB `query`/`exec` split, removed PHP funcs, MyTextSanitizer + Smarty API renames) runs alongside the generic PHP sets. | | `phpunit.xml.dist`, `tests/` | Test config + a guarded bootstrap. Put unit tests in `tests/Unit/`, integration tests in `tests/Integration/`. | | `.gitattributes` | `export-ignore` rules — keeps dev/CI files **out** of the release ZIP. | @@ -140,6 +141,29 @@ To enable it for a module: 3. Add a **`SONAR_TOKEN`** secret under *Settings → Secrets and variables → Actions*. 4. *(Optional)* install the SonarCloud GitHub App for pull-request decoration. +## Extending the stub + +`stubs/xoops.stub` declares core symbols so PHPStan stops reporting `class.notFound` / +`function.notFound` for APIs that only exist inside a booted XOOPS. Two things to know +before you add to it. + +**Declaring a class turns one error into several.** While a class is unknown, every call on +it is a single `class.notFound`. Once the class exists, PHPStan starts checking the calls — +so a class with five methods you did not declare produces five `method.notFound` where +there was one error before, and any `phpstan-baseline.neon` entry matching the old message +silently stops applying. Add the methods your modules actually call in the same change, and +re-run `composer analyse` on a consumer module before you push the baseline. + +**A `core27` stub describes the newest 2.7 API, not the site's.** It cannot express "since +2.7.3". If your module's `min_xoops` is below the release that added a function, keep the +`function_exists()` guard around every call — the stub silences the analyser, it does not +create the function at runtime. The error-screen ownership functions in the stub are the +current example and say so where they are declared. + +**Prefer the shared stub to a local one.** Modules that hand-copy declarations drift the +moment a signature changes. If you need a symbol, add it here and re-sync, rather than +patching your module's copy. + ## Release archives Tag a release (e.g. `git tag 1.0.0 && git push --tags`). `release.yml` runs diff --git a/docs/hosting-a-library-that-grabs-global-handlers.md b/docs/hosting-a-library-that-grabs-global-handlers.md new file mode 100644 index 0000000..c1493ac --- /dev/null +++ b/docs/hosting-a-library-that-grabs-global-handlers.md @@ -0,0 +1,101 @@ +# Hosting a library that grabs global handlers + +Some libraries take things that belong to the whole process: PHP's error and exception +handlers, shutdown functions, the session, the output buffer, `ini` settings, autoloaders, +headers. They do it because they were written for an application that owns the request. +Inside XOOPS your module does not own the request — it is one of many things loading during +a boot, and XoopsLogger, DebugBar and the error screen all have claims on the same globals. + +So you will sometimes want a library's features **without** one of the side effects it +installs. This page is about how to do that without breaking the library. + +## The rule + +> **Refuse the side effect at the library's own injection seam. Do not let it happen and +> then undo it.** + +Most libraries that grab a global offer a way to say no: a constructor argument, a config +flag, a driver or facade object, a factory you can subclass. Passing your own object through +that seam means the side effect never happens. Undoing it afterwards means it happened, and +you are now relying on the library not noticing. + +## Why "undo it afterwards" fails + +It usually works, right up until the library tidies up after itself. + +Global hooks are almost always installed and removed as a **matched pair**: `register()` and +`unregister()`, `enable()` and `disable()`, and often a destructor that calls the second one +whether or not you asked. PHP's own handler functions make this concrete — +`set_error_handler()` **pushes** a frame and `restore_error_handler()` **pops** one; they are +not setter and getter. + +So if the library pushes two frames and you quietly pop one of them back, the library's +teardown still pops two. One of those pops takes a frame that was never the library's, and +the request ends with the error handler one level below where it started — usually somewhere +that silently swallows notices. Nothing throws. Nothing logs. You find it months later. + +The worked example this rule came from: a Whoops provider needed the exception and shutdown +handlers but not the error handler. `restore_error_handler()` immediately after +`$whoops->register()` looked like the obvious fix, passed its tests, and was wrong — +`Run::unregister()`, called by `Run::__destruct()`, restores **both** handlers. The fix that +holds is to pass `Run::__construct()` a `Whoops\Util\SystemFacade` subclass whose +`setErrorHandler()` and `restoreErrorHandler()` do nothing, so Whoops is never given the +error handler and its own pairing stays balanced by construction. + +## Does it apply here? + +| Use the seam when | Do something else when | +|---|---| +| The library exposes a facade, driver, factory or constructor argument for its global hooks | It offers only a global `enable()` with no injection point | +| Its lifecycle is paired — register/unregister, enable/disable, or a destructor that cleans up | Nothing is ever torn down, so there is no pairing to break | +| You want a documented **subset** of what it installs by default | You actually want everything it installs | +| The subset is part of your module's contract with the site | Refusing would break the feature you are shipping | + +### When there is genuinely no seam + +Undo-after is not the fallback. It is the thing this page tells you not to do, and having no +alternative does not make it safe — it makes the integration **unsupported until you have +done the work to know it is safe**. Concretely, before you ship a `restore_*`: + +- read the library's setup path, its explicit teardown, **and** its destructor, and know + which of them touch the global you are taking back; +- know whether the library's teardown assumes it still owns what you took — that is the + failure that motivated this page, and it is invisible until something destructs; +- **write a test that asserts the process-wide state is what it was**, exercised across the + library's full lifetime including teardown. Not a comment. A comment records your belief; + a test notices when the belief stops being true, which is usually at the next upgrade. + +If you cannot do those three, do not host the library that way. Ask upstream for a seam — +that is how `SystemFacade` came to exist in Whoops — or keep the side effect and design +around it. + +## Before you ship one + +1. **Prefer an official option.** A documented config flag beats a subclass. +2. **Use a public seam.** Never reflection, never a vendor edit, never `@` on the symptom. +3. **Copy the parent signature verbatim.** Adding a type to an inherited untyped parameter + is a fatal at class-declaration time, on every request. Refactoring and static-analysis + tools propose exactly this — write a comment saying no, because you will be asked again. + (Adding a *return* type where the parent declares none is fine and covariant-safe.) +4. **Check the library does not use what you refused.** If it stores the return value of the + method you neutered, returning `null` may not be harmless. +5. **Test the teardown, not just the setup.** The bug above is invisible if you only assert + state after `register()`. Assert it after `unregister()` too — and after the object goes + out of scope, if it has a destructor. +6. **Re-check on upgrade.** You are depending on a shape the library did not promise to + keep. Pin the dependency or re-run the teardown test when you bump it. + +## Libraries this comes up with + +`SystemFacade` is Whoops' door and nothing else's. Tracy, Monolog, Symfony's `ErrorHandler` +and Sentry each install global hooks through a different one, and some of them through +several. The reflex to carry is not an API name: + +> **Look for the injection point before reaching for `restore_*`.** + +--- + +*Origin: XOOPS 2.7.3's error-screen provider seam, where the first attempt at this was +wrong in the way described above. The seam-specific decision — why xWhoops refuses the error +handler in particular — lives in the error-screen provider ADR that ships with that feature. +The rule is general and lives here.* diff --git a/phpstan-bootstrap.php b/phpstan-bootstrap.php index 0f3b7ab..ea7e575 100644 --- a/phpstan-bootstrap.php +++ b/phpstan-bootstrap.php @@ -25,4 +25,22 @@ define('_CHARSET', 'utf-8'); } +// The module's own language constants (_MI_*, _MA_*, _AM_*). The files are pure +// define() lists, so loading them lets analysis resolve every constant to a real +// string -- and catch a mistyped constant name -- instead of baselining each one +// as unknown. Explicit allowlist, never a glob: only named language-definition +// files are ever executed here, so a stray PHP file in language/english (or the +// anti-indexing index.php stub) cannot run during analysis. EDIT the list to +// match your module's language files (admin.php, main.php, modinfo.php, +// blocks.php, ...). The is_file() guard keeps the template copy-paste safe when +// an entry does not exist; a file you forget to LIST is loud, as PHPStan then +// reports every one of its constants as unknown. +foreach (['admin.php', 'main.php', 'modinfo.php'] as $xoopsLanguageFile) { + $xoopsLanguagePath = __DIR__ . '/language/english/' . $xoopsLanguageFile; + if (is_file($xoopsLanguagePath)) { + require_once $xoopsLanguagePath; + } +} +unset($xoopsLanguageFile, $xoopsLanguagePath); + // Profile target: XoopsCore27 / PHP 8.2+ diff --git a/sonar-project.properties b/sonar-project.properties index 7da6485..0172e6b 100644 --- a/sonar-project.properties +++ b/sonar-project.properties @@ -33,7 +33,11 @@ sonar.issue.ignore.multicriteria=x1,x2a,x2b,x3,x4,x5 # S115: constant names must match ^[A-Z]..., but every XOOPS _MI_/_MA_/_AM_ constant # begins with an underscore -- the rule can never pass for XOOPS language constants. -# Scoped to language/, the mandated home of those constants; all other PHP keeps S115. +# language/** is also source-excluded above (pure define() lists, nothing to scan), so +# this ignore is currently dormant; it is kept deliberately as defense-in-depth so that +# lifting the exclusion -- e.g. to SAST-scan language files -- cannot flood the report +# with S115. Scoped to language/, the mandated home of those constants; all other PHP +# keeps S115. sonar.issue.ignore.multicriteria.x1.ruleKey=php:S115 sonar.issue.ignore.multicriteria.x1.resourceKey=language/**/*.php diff --git a/stubs/xoops.stub b/stubs/xoops.stub index c728d76..1b279ae 100644 --- a/stubs/xoops.stub +++ b/stubs/xoops.stub @@ -177,6 +177,28 @@ namespace { { public function __construct() {} } + + // ------------------------------------------------- error-screen ownership (2.7.3+) + + // The provider API published by htdocs/include/debugconfig.php. A module that wants to + // own the error screen calls these from its install / update / uninstall hooks. + // + // SINCE 2.7.3, and this stub does not say otherwise. A `core27` profile describes the + // LATEST 2.7 API, not what a given site is running -- so a module whose min_xoops is + // below 2.7.3 must keep its function_exists() guard around every call. Declaring a + // symbol here makes analysis stop complaining; it does not make the function exist. + // + // See docs/hosting-a-library-that-grabs-global-handlers.md for the companion rule on + // what a provider must not do once it has the seat. + + /** Record this module as the error-screen owner. $force takes an occupied seat. */ + function xoops_recordErrorScreenOwner(string $dirname, bool $force = false): bool {} + + /** The recorded owner's dirname, or '' when the seat is free. */ + function xoops_getRecordedErrorScreenOwner(): string {} + + /** Release the seat, but only if $dirname currently holds it. */ + function xoops_releaseErrorScreenOwner(string $dirname): bool {} } namespace Xmf\Module {