DRAFT: Raise the modules' PHPStan level from 0 to 1 - #1513
Closed
austinderrick wants to merge 7 commits into
Closed
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.
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.
…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.
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.
|
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 |
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.
Level 1 adds undefined-variable, unknown-method and argument-count analysis, which surfaced 180 errors. Every one is accounted for: Real defects fixed: - CmsException took the wrong branch for unknown error codes: the switch has no default, so $result stayed undefined and null !== false passed the is-CMS-exception check. It now starts false. - MediaLibrary passed undefined $type and $key into item construction for directory contents that are neither files nor folders; those entries are now skipped. - Config::package() and the FileManifest constructor were invoked with a vestigial extra argument PHP silently discards. - Uninitialized variables read on edge paths: $success in AutoDatasource, $result in Backend\Controller, $branch in UpdateManager, $resizer in SystemController, and $pluginId in PluginManager's catch blocks. - An unused closure import in BundleManager. Magic contracts documented: - The settings models carry @mixin and @method annotations for the SettingsModel behavior's API, and the attachOne relations, Halcyon builder forwards and the applyKey scope are annotated where used. - Model attributes resolve through magic accessors, declared via universalObjectCratesClasses for the two model base classes; typing them per model is the next ratchet, not a level-1 gate. - A stub corrects the Event facade's listen() arity (storm accepts a priority). - View partials inside model directories are excluded, matching phpcs. Two narrowly scoped, commented ignores remain where Winter's extendable dispatch defeats annotation: the SettingsModel get($key, $default) arity collision with the query builder, and parent::resetDefault() reaching the behavior through __call.
austinderrick
force-pushed
the
feat/modules-phpstan-level-1
branch
from
August 14, 2026 03:46
3934c6b to
dd97c22
Compare
austinderrick
marked this pull request as draft
August 14, 2026 03:46
austinderrick
changed the title
austinderrick
changed the title
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.
Stacked on #1512 — review only the last commit until that merges.
Summary
Raises the modules' PHPStan level from 0 to 1, which adds undefined-variable, unknown-method and argument-count analysis. The first run reported 180 errors; every one is accounted for below, and the analysis finishes clean with no baseline.
Real defects fixed
CmsExceptiontook the wrong branch for unknown error codes. The switch over the code has no default case, so$resultstayed undefined, andnull !== falsepassed the is-CMS-exception check that should have failed. It now starts as false.MediaLibrarypassed undefined$typeand$keyinto library item construction for directory contents that are neither files nor folders. Those entries are now skipped.Config::package()and theFileManifestconstructor were each invoked with a vestigial extra argument that PHP silently discards.$successinAutoDatasource,$resultinBackend\Controller,$branchinUpdateManager::requestChangelog(),$resizerinSystemController's catch path, and$pluginIdinPluginManager's catch blocks.BundleManager.Magic contracts documented
BrandSetting,EditorSetting,Preference,MailSetting,MailBrandSetting,LogSetting,MaintenanceSetting) carry@mixinand@methodannotations for theSettingsModelbehavior's API. TheattachOnerelations onImportModelandThemeImport, the Halcyon builder forwards onPageandCmsCompoundObject, andParameter'sapplyKeyscope are annotated where used.universalObjectCratesClassesfor the two model base classes. Typing attributes per model is the natural next ratchet rather than a level-1 gate.Eventfacade'slisten()arity: Storm's dispatcher accepts a listener priority that the resolved Laravel signature does not.Two scoped ignores, with reasons in the config
Winter's extendable dispatch defeats annotation in exactly two places, and both carry comments in
phpstan.neon.distrather than baseline entries: theSettingsModelbehavior'sget($key, $default)collides with the query builder'sget($columns)arity when larastan resolves model statics, andparent::resetDefault()reaches the behavior through__call, which analysis cannot follow through a parent reference.