Add PHPStan static analysis for the modules - #1512
Open
austinderrick wants to merge 9 commits into
Open
Conversation
Analyses the classes and models of the three modules. Winter's modules are
not composer-autoloaded and extend runtime class aliases, so a small
bootstrap registers the alias map from modules/system/aliases.php and the
module directories are scanned for symbol discovery.
The first run surfaced real defects, fixed here:
- ThemeExport and ThemeImport built their not-intended-to-be-saved exception
message with a broken sprintf placeholder ("The % model"), so the model
class name never appeared in the message.
- FilterScope declared every configurable property except $default, which
was created dynamically on assignment (deprecated since PHP 8.2).
- Eleven docblocks promised a return value on paths that return nothing, or
the wrong type entirely: FormTabs::getIcon() and getPaneCssClass(),
WidgetBase::render(), CmsObject::save(), ComponentManager's
registerComponents() and makeComponent(), Router's findByUrl() and
setParameters(), the four yaml-backed PluginBase register methods, and
UpdateManager's downloadPlugin() and downloadTheme(), which claim to
return self but return nothing (no caller chains on them).
CombineAssets::getDeepHashFromAssets() claimed void while returning the
hash string its only caller concatenates.
The baseline carries three deliberate entries: BundleManager's setup handler
calls (the closures are rebound to the console command with Closure::bind, so
the methods exist at runtime), the new.static warnings on non-final
constructors, and CodeParser::handleCorruptCache(), which is a real defect
with a fix already in flight in wintercms#1511.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
The workflow now installs with --no-scripts and resets the working tree the same way tests.yml does, since composer/installers replaces the checked-out modules with the packaged copies during install. The bootstrap gains an autoloader for the modules' class loader convention, so alias targets resolve without depending on how composer happened to install the module packages.
The lock file is not tracked, so the dependency costs one composer.json line, and storm already maintains the same setup. Larastan's rules immediately paid their way: two vestigial single-argument with() wrappers in UpdateManager and VersionManager, replaced with the direct (new ...)->render() call the syntax has supported since PHP 8.0. The baseline is unchanged.
austinderrick
force-pushed
the
feat/modules-phpstan
branch
from
August 14, 2026 01:05
730224b to
d7499cc
Compare
bennothommo
requested changes
Aug 14, 2026
bennothommo
left a comment
bennothommo
left a comment
Member
There was a problem hiding this comment.
OMG thank you @austinderrick - I've been wanting to do this for forever, but the last time I tried, it was a nightmare. I've suggested a couple of tweaks needed before we'd accept the changes, but great work so far!
…line - The bootstrap now registers Winter's own ClassLoader for the modules, mirroring modules/system/tests/bootstrap/app.php, instead of a hand-rolled approximation of its convention. - checkout@v7 and a 2G memory limit in the workflow job. - One more baseline entry resolved: CodeParser::handleCorruptCache() declared @return void while returning the repaired cache data on every path, which is what made its caller's use of the result look like a defect. The BundleManager entries were investigated rather than assumed: typing $this inside the handlers converts the errors to protected-method violations, because the closures' static scope stays BundleManager while the runtime rebinding through Closure::call() is what legitimizes the access, so those stay baselined.
austinderrick
force-pushed
the
feat/modules-phpstan
branch
from
August 14, 2026 03:11
c21a36e to
d37b346
Compare
Both remaining categories had proper answers after all: - @param-closure-this on registerSetupHandler() and registerScaffoldHandler() declares what Closure::call() does at runtime, resolving the fourteen undefined-method reports inside the handlers and giving handler authors a typed $this in the bargain. - @phpstan-consistent-constructor on the seven classes using new static() documents the constructor contract those factories already rely on, without the API change final constructors would be. PHPStan now reports zero errors with no ignores anywhere.
austinderrick
force-pushed
the
feat/modules-phpstan
branch
from
August 14, 2026 03:21
aa5ab3a to
1671c6a
Compare
Contributor
Author
|
@bennothommo : Updated! :) |
Halcyon's inherited update() propagates save()'s return value, and Model::save() answers bool, so swallowing the result here made every successful CmsObject update indistinguishable from a vetoed one: callers checking the documented bool always received null. Returning the parent result restores the contract the docblock now correctly states. throwHalcyonSaveException() gains a @return never annotation; every branch of it throws.
austinderrick
force-pushed
the
feat/modules-phpstan
branch
from
August 14, 2026 03:51
8c5f05f to
b8593e1
Compare
Member
|
Great stuff @austinderrick. Just one last change - per Winter Storm's workflow, the name of the workflow should be "Tests" so it's grouped with the testing, and job name should be "Code Analysis". It's more of a test rather than code quality in my opinion. |
Mirrors storm's code-analysis.yaml: the workflow is named Tests so the job groups with the test runs, the job is named Code Analysis, and the composer GitHub token is configured the way storm's workflow does. code-quality.yaml returns to its upstream state.
The branch this lands on first was missing from the push list, so merges to it would never have run the analysis outside of pull requests.
Contributor
Author
Makes sense, @bennothommo! Updated and pushed. 💯 |
LukeTowers
requested changes
Aug 15, 2026
austinderrick
force-pushed
the
feat/modules-phpstan
branch
from
August 15, 2026 19:14
45ac8b7 to
69981d3
Compare
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
3 participants
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
Adds PHPStan, via larastan as storm already uses, at level 0 over the classes and models of the three modules, wired into the Code Quality workflow. This brings the modules the same discipline storm has, starting at the lowest level with a small baseline so the diff stays reviewable.
Two things make the modules analysable that a stock PHPStan setup lacks: the modules are not composer-autoloaded, so the module directories are registered as scan directories, and module classes extend runtime class aliases (
Modeland friends), so a small bootstrap file registers the alias map frommodules/system/aliases.phpbefore analysis.What the first run found
Every error was verified against the code before being fixed or baselined. The fixes:
ThemeExportandThemeImportbuild their "not intended to be saved" exception message with a broken sprintf placeholder ("The % model"), so the model class name never appeared in the message.FilterScopedeclares every configurable property except$default, which was created dynamically on assignment. Dynamic property creation is deprecated since PHP 8.2.CmsObject::save()swallowed the parent's bool result while documenting@return bool, making a successful save indistinguishable from a vetoed one for any caller of the inheritedupdate(); it now returns the parent result.UpdateManager::downloadPlugin()anddownloadTheme()claim to returnselfbut return nothing (no caller chains on them, so this is a documentation fix rather than a behavior fix), andCombineAssets::getDeepHashFromAssets()claimedvoidwhile returning the hash string its only caller concatenates into the cache key.No baseline
Every error from the first run is resolved rather than ignored, so there is no baseline file. The two categories that initially looked unresolvable both had proper answers:
BundleManager's setup and scaffold handlers run bound to the invoking asset command throughClosure::call(), which PHPStan cannot see from the registration site.@param-closure-thisonregisterSetupHandler()andregisterScaffoldHandler()declares that contract on the API itself, which also gives third-party handler authors a typed$this.new static()warnings are answered with@phpstan-consistent-constructoron the seven classes concerned, which documents the contract those factories already rely on without touching the public API the wayfinalconstructors would.Running it
The workflow job does the same. From here the level can be raised one step at a time and the paths can widen beyond classes and models as the baseline burns down.