Repository navigation
chore(phpstan): load module language constants via explicit allowlist - #13
Merged
Merged
Conversation
Teach the bootstrap template to load the module's own language files (_MI_*/_MA_*/_AM_* defines) so analysis resolves every constant to a real string -- and catches a mistyped name -- instead of baselining each one as unknown. First proven in xtracy, where it shrank the baseline from 32 to 20 entries. The loading is an explicit allowlist, never a glob: only named language-definition files can execute during analysis, so a stray PHP file in language/english (or the anti-indexing index.php stub) never runs. Consuming modules edit the list to match their files; is_file() keeps the template copy-paste safe when an entry is absent, while a file missing from the list stays loud through unknown-constant reports.
language/** is excluded from sonar.sources, so the S115 ignore scoped to language/**/*.php never currently fires. That is deliberate: the scoped ignore stays as defense-in-depth so that lifting the exclusion (e.g. to SAST-scan language files) cannot flood the report with S115 naming findings. Record that intent where the ignore is declared so the pairing no longer reads as a contradiction.
Contributor
Reviewer's GuideAdds explicit loading of module language constants for PHPStan via an allowlisted set of language files, extends documentation around the shared XOOPS stub and global handler-hosting, documents new error-screen ownership functions in the stub, and clarifies the dormant Sonar S115 suppression for language files. Sequence diagram for PHPStan bootstrap language-constant loadingsequenceDiagram
participant PhpStan
participant PhpstanBootstrap as phpstan-bootstrap.php
participant LangAdmin as language_english_admin.php
participant LangMain as language_english_main.php
participant LangModinfo as language_english_modinfo.php
PhpStan->>PhpstanBootstrap: require phpstan-bootstrap.php
PhpstanBootstrap->>PhpstanBootstrap: foreach ['admin.php','main.php','modinfo.php']
loop for each xoopsLanguageFile
PhpstanBootstrap->>PhpstanBootstrap: is_file(__DIR__.'/language/english/'+xoopsLanguageFile)
alt [file exists]
PhpstanBootstrap->>LangAdmin: require_once admin.php
PhpstanBootstrap->>LangMain: require_once main.php
PhpstanBootstrap->>LangModinfo: require_once modinfo.php
else [file missing]
PhpstanBootstrap-->>PhpstanBootstrap: skip file (constants stay unknown)
end
end
PhpstanBootstrap-->>PhpStan: module language constants _MI_*, _MA_*, _AM_* defined for analysis
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThe PR adds XOOPS 2.7.3 error-screen ownership stubs, allowlisted PHPStan language loading, SonarPHP documentation, README guidance, changelog entries, and a guide for integrating libraries that install global handlers. ChangesXOOPS analysis support
Global handler integration guidance
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
phpstan-bootstrap.phpthe language path is hardcoded tolanguage/english/; consider deriving the language subdirectory from a single constant/config value so modules using a different default locale (or supporting multiple locales in analysis) don’t need to edit the bootstrap logic itself.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `phpstan-bootstrap.php` the language path is hardcoded to `language/english/`; consider deriving the language subdirectory from a single constant/config value so modules using a different default locale (or supporting multiple locales in analysis) don’t need to edit the bootstrap logic itself.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Greptile SummaryThe PR extends the static-analysis template to load explicitly allowlisted module language definitions and adds XOOPS 2.7.3 error-screen ownership declarations, with accompanying documentation refinements.
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking template-default omission for modules that define block language constants. The guarded explicit loading is safe for absent files, but including blocks.php in the default allowlist would avoid unnecessary consumer customization and unknown-constant analysis noise. Files Needing Attention: phpstan-bootstrap.php
|
| Filename | Overview |
|---|---|
| phpstan-bootstrap.php | Adds guarded language-file loading; the allowlist omits the conventional blocks.php file despite naming it in the customization guidance. |
| stubs/xoops.stub | Adds documented XOOPS 2.7.3 error-screen ownership API declarations and clearly preserves the runtime version-guard requirement. |
| docs/hosting-a-library-that-grabs-global-handlers.md | Adds focused guidance on refusing global side effects through library injection seams rather than unbalancing lifecycle cleanup. |
| README.md | Documents shared-stub extension risks, version compatibility, and the new integration guide. |
| sonar-project.properties | Clarifies why the language naming suppression remains configured while language sources are excluded. |
Reviews (1): Last reviewed commit: "chore(sonar): document why the dormant S..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // 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) { |
There was a problem hiding this comment.
The guidance names blocks.php as a conventional language-definition file, but the default allowlist omits it. Modules with scanned block code must therefore customize the list or retain avoidable unknown-constant errors and baseline entries.
Suggested change
| foreach (['admin.php', 'main.php', 'modinfo.php'] as $xoopsLanguageFile) { | |
| foreach (['admin.php', 'blocks.php', 'main.php', 'modinfo.php'] as $xoopsLanguageFile) { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@stubs/xoops.stub`:
- Around line 194-201: Update the parameter name in the ownership-related stub
declarations xoops_recordErrorScreenOwner and xoops_releaseErrorScreenOwner from
$dirname to $token, matching the XOOPS 2.7.3 runtime declarations while
preserving their types, defaults, and return types.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ecfe32d-0f0d-40bd-bbff-03b17b883561
📒 Files selected for processing (6)
CHANGELOG.mdREADME.mddocs/hosting-a-library-that-grabs-global-handlers.mdphpstan-bootstrap.phpsonar-project.propertiesstubs/xoops.stub
Comment on lines
+194
to
+201
| /** 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 {} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
runtime_files="$(fd -t f '^debugconfig\.php$' . || true)"
if [[ -z "$runtime_files" ]]; then
echo "No debugconfig.php found."
exit 0
fi
while IFS= read -r file; do
rg -n -A4 -B2 'function xoops_(record|release)ErrorScreenOwner' "$file"
done <<< "$runtime_files"
rg -nP '\bxoops_(record|release)ErrorScreenOwner\s*\([^)]*\b(dirname|token)\s*:' \
--glob '*.php' . || trueRepository: XoopsModules27x/module-devops
Length of output: 193
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo '--- stub declarations and nearby documentation ---'
sed -n '175,205p' stubs/xoops.stub
echo '--- local ownership API references ---'
rg -n -C2 'xoops_(record|release)ErrorScreenOwner|dirname:|token:' . \
--glob '*.php' --glob '*.stub' --glob '*.inc' --glob '*.md' || true
echo '--- XOOPS 2.7.3 runtime declaration ---'
url='https://raw.githubusercontent.com/XOOPS/XoopsCore27/master/htdocs/include/debugconfig.php'
curl -fsSL "$url" | rg -n -A5 -B3 \
'function xoops_(record|release)ErrorScreenOwner' || trueRepository: XoopsModules27x/module-devops
Length of output: 3715
Match the runtime parameter names.
XOOPS 2.7.3 declares $token for both ownership functions. PHP 8 named arguments require the stub and runtime to use the same parameter names.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@stubs/xoops.stub` around lines 194 - 201, Update the parameter name in the
ownership-related stub declarations xoops_recordErrorScreenOwner and
xoops_releaseErrorScreenOwner from $dirname to $token, matching the XOOPS 2.7.3
runtime declarations while preserving their types, defaults, and return types.
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.
Summary
Two template refinements proven downstream in xtracy and xwhoops.
1. Language-constant loading in
phpstan-bootstrap.php. The bootstrap now loads the module's own language files (_MI_*/_MA_*/_AM_*defines), so analysis resolves every constant to a real string — and catches a mistyped constant name — instead of baselining each one as unknown. First proven in xtracy, where it shrank the PHPStan baseline from 32 to 20 entries.2. S115 suppression comment in
sonar-project.properties.language/**is source-excluded, so the S115 ignore scoped tolanguage/**/*.phpnever currently fires; the comment now records that the ignore is deliberately kept dormant as defense-in-depth (lifting the exclusion later cannot flood the report with S115 naming findings), so the pairing no longer reads as a contradiction. Applied with identical wording in xtracy (XoopsModules27x/xtracy#2) and xwhoops (XoopsModules27x/xwhoops#5).Approach
The language loading is an explicit allowlist, never a glob: only named language-definition files can execute during analysis, so a stray PHP file in
language/english(or the anti-indexingindex.phpstub) never runs. Consuming modules edit the list to match their files. Unlike the xtracy original, entries are wrapped inis_file()— the template must stay copy-paste safe for modules missing an entry (and for this repo, which has nolanguage/directory), while a file missing from the list stays loud through unknown-constant reports.Testing
Full local gate green in this repo (
composer qa: php-cs-fixer 0 findings, PHPStan no errors, Rector clean, PHPUnit passing) — proving the guarded block is a no-op where no language files exist.Summary by Sourcery
Document shared XOOPS stub usage and extend static-analysis bootstrap to load module language constants, while clarifying SonarQube configuration and adding guidance for handling libraries that grab global handlers.
New Features:
Enhancements:
CI:
Documentation:
Summary by CodeRabbit
New Features
Documentation
Chores