You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
PHP 8.5 adds native array_first() and array_last() functions that do not match the signature provided by Winter's helper functions. If you are currently using either function in your code with two or three arguments (filter & default) you will need to switch to \Winter\Storm\Support\Arr::first() or \Winter\Storm\Support\Arr:last() instead.
The -s short flag for the --silent option on mix:compile, mix:create, mix:install, mix:watch, npm:install, npm:run, npm:update, npm:version, vite:compile, vite:create, vite:install, vite:watch has been removed as Symfony v7.2 added the --silent option to all commands by default which conflicted with our definition. You must use the full option --silent going forward.
static $defaultName in Console commands to support lazy loading is no longer used in Symfony\Console 7.2, use the AsCommand class attribute instead. We will need to update all first party plugins & modules to make use of that.
All other Winter plugins have been tested and confirmed compatible (some with minor tweaks to their PHPUnit configs to remove deprecation warnings.
New Minimum Requirements:
PHP 8.2+
Composer 2.2+
curl 7.34.0+
SQLite 3.26.0+
PHPUnit 11.0 (when using the testing functionality)
Backwards Compatibility Fixes:
Laravel 11 Modifying Columns in Migrations: This change has been reverted in Winter's core framework (Storm) so that you do not have to modify any of your existing migrations.
symfony/symfony#53632 made it so that the --silent option is now provided & managed by the symfony core itself which would have been all well and good, except for the fact that since symfony/symfony#24425 symfony has been persisting the selected verbosity layer in the environment. This means that if you have a test that calls a command with the --silent flag set, and then it's followed by a test that relies on the --silent flag not being set, that second test (and any future test that expects the --silent flag not to be set) will also fail.
This leads to all sorts of headaches around tests failing depending on the order they are run in, whether or not you're running a single test case or multiple, or even whether the environment you're running the tests in actually supports putenv().
Massive credit to @jaxwilko for finding the issue and coming up with the solution.
Raise PHP/Laravel requirements, modernize CI and PHPUnit configs, remove many CLI --silent options, add an SQLite minimum-version check in UpdateManager::update, update test bootstrapping and many tests for newer PHPUnit/Laravel facades/attributes, and apply assorted import/signature and minor fixture/.gitignore changes.
Changes
Cohort / File(s)
Summary of changes
CI workflows .github/workflows/manifest.yml, .github/workflows/tests.yml
Replace env/template indirection with explicit PHP versions/extensions and GITHUB_TOKEN; pin actions (actions/checkout@v4); adjust PHP matrix to 8.2–8.4; remove several cache steps and legacy steps; update workflow targets and settings.
Replace array_first with Arr::first, adjust imports, call parent::tearDown() after instance cleanup, and refactor WidgetMaker test to use an anonymous class with Controller property/constructor.
Add SQLite runtime guard: when using sqlite driver, verify server version ≥ 3.35 and throw an exception if older before proceeding with updates/migrations.
Update WinterTest signature and skip/target handling; delegate createApplication to parent; bind Kernel to suppress shell verbosity; add new assertion helper and remove deprecated compatibility shims.
System tests modernizations modules/system/tests/... (many files; see diff)
Switch tests to Winter/Laravel facades (DB/Log/File/Schema), convert dataProviders/depends to PHPUnit attributes (make providers static), add parent::tearDown() calls, reposition node_modules skip checks, and update migration test assertions to SchemaBuilder-based checks.
Fixtures & minor files modules/system/tests/fixtures/.../Comments.php, .gitignore
Reorder Comments constructor parameters to require Users $users first and make CodeBase nullable second; add .phpunit.cache and *.code-workspace to .gitignore.
Sequence Diagram(s)
sequenceDiagram
autonumber
actor Admin as Admin/CLI
participant UM as UpdateManager
participant DB as Database Connection
Admin->>UM: update()
UM->>DB: getDriverName()
alt driver == sqlite
UM->>DB: getServerVersion()
UM->>UM: compare >= 3.35
alt version < 3.35
UM-->>Admin: throw Exception (SQLite too old)
else version OK
UM->>UM: continue update/migrations
end
else other driver
UM->>UM: continue update/migrations
end
UM-->>Admin: result or exception
Loading
sequenceDiagram
autonumber
actor Dev as Developer/CLI
participant WT as WinterTest Command
Dev->>WT: winter:test --configuration=... --module=... --plugin=...
WT->>WT: build types map (modules, plugins) and counts
WT->>WT: skip processing for empty targets
loop each non-empty type
WT->>WT: run tests for specified items
end
WT-->>Dev: aggregated results
Loading
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Potential review focus areas:
PHPUnit config/schema migration and broad test attribute/provider conversions.
UpdateManager SQLite version check (correct exception class/message, unit tests).
Removal of --silent options where code still references the option (ensure no unexpected empty args or output changes).
Composer constraint bumps across root and modules (dependency resolution and compatibility).
CreateMigrationTest changes (SchemaBuilder assertions and type/index expectations).
Poem
A rabbit nudges the repo bright,
Bumps PHP, tunes CI overnight.
Silent flags hop far away,
Tests don attributes to play.
SQLite checked — the meadow’s right. 🥕
Pre-merge checks and finishing touches
✅ Passed checks (3 passed)
Check name
Status
Explanation
Description Check
✅ Passed
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
✅ Passed
The title 'Support Laravel 12' directly and clearly describes the main objective of this pull request, which is to upgrade the codebase to support Laravel 12.
Docstring Coverage
✅ Passed
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
📝 Generate docstrings
🧪 Generate unit tests (beta)
Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch wip/1.3
📜 Recent review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 63f2a3e and 58057d0.
📒 Files selected for processing (1)
modules/system/console/WinterTest.php (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
modules/system/console/WinterTest.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
GitHub Check: windows-latest / PHP 8.3
GitHub Check: windows-latest / PHP 8.4
GitHub Check: ubuntu-latest / PHP 8.3
GitHub Check: windows-latest / PHP 8.2
GitHub Check: ubuntu-latest / PHP 8.4
GitHub Check: ubuntu-latest / PHP 8.2
GitHub Check: windows-latest / JavaScript
Comment @coderabbitai help to get the list of available commands and usage tips.
6-6: Align schema with PHPUnit 11 (and across module configs).
Repo now targets PHPUnit 11; using the 10.5 XSD may be suboptimal. Please verify and update to the 11.x schema URL (and keep module phpunit.xml files consistent).
Would you confirm the exact PHPUnit 11 minor used and align the XSD accordingly?
21-33: Exclude test directories from coverage to avoid skewed metrics.
Current includes all of modules/, which will also count test files. Exclude modules/**/tests to keep coverage focused on source.
36-36: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
6-6: LGTM! Proper facade namespace for Laravel 12.
The change correctly updates the File reference to use the Winter Storm facade, and the static method usage in tearDown() is appropriate for facade calls.
The updated imports now use fully qualified namespaces (Winter\Storm\Exception\SystemException, Winter\Storm\Support\Str) instead of short aliases, making dependencies explicit and improving code maintainability. All imported classes are properly used throughout the file:
The import statement is correctly added and necessary for the AuthManager class usage throughout the test file.
44-48: LGTM! Proper teardown sequence.
The order is correct: forgetting the singleton instance before calling parent::tearDown() ensures test-specific state is cleaned up before the parent's application context teardown, preventing potential issues with the instance accessing a destroyed context.
11-15: Constructor change verified as safe and correct—no instantiation sites affected.
The Comments component's constructor correctly accepts the Users dependency and properly calls parent::__construct($cmsObject, $properties), which matches the ComponentBase signature. Verification of the codebase found no direct instantiations or container resolutions of this component, confirming the change introduces no breaking changes to actual code usage. The component is registered in the test plugin but appears unused in current tests, making this a safe refactoring.
modules/system/classes/UpdateManager.php (1)
138-143: Fix exception type and remove unconventional semicolon; verify SQLite version requirement.
ApplicationException is already imported and consistently used throughout this file for similar errors. Replace the plain Exception at line 141, remove the semicolon after the closing brace at line 143, and add the detected version to the error message for better debugging:
$connection = Schema::getConnection();
if ($connection->getDriverName() === 'sqlite') {
- if (version_compare($connection->getServerVersion(), '3.35', '<')) {- throw new Exception("SQLite version minimum requirement not met (>= 3.35)");+ $version = $connection->getServerVersion();+ if (version_compare($version, '3.35', '<')) {+ throw new ApplicationException(+ "SQLite version {$version} does not meet the minimum requirement (>= 3.35)"+ );
}
- };+ }
However, verify that 3.35 is the correct minimum version. The PR description or related documentation should confirm whether the requirement is 3.26.0 or 3.35—adjust the version number in the code and error message accordingly.
modules/system/console/WinterTest.php (3)
12-12: LGTM!
The Config facade import is correctly added and used later in the file (line 250).
37-42: LGTM!
The command signature updates are syntactically correct. The = and =* syntax properly defines options that require values when provided and accept multiple values, respectively.
107-109: LGTM!
The empty target guard is a good defensive measure that prevents processing empty strings and avoids potential downstream issues.
Adding parent::tearDown() ensures the base class cleanup logic executes after removing test artifacts, following PHPUnit best practices. This change is consistent with similar improvements across other test files in this PR.
modules/cms/composer.json (1)
26-28: LGTM! Version constraints updated correctly.
The PHP and Laravel framework constraints have been appropriately updated to align with the PR's minimum requirements (PHP 8.2+ and Laravel 12).
modules/backend/composer.json (1)
26-28: LGTM! Version constraints updated correctly.
The PHP and Laravel framework constraints have been appropriately updated to align with the PR's minimum requirements (PHP 8.2+ and Laravel 12).
The change from array_first() to Arr::first() prevents conflicts with PHP 8.5's native array_first() function, which has a different signature than Winter's helper. Using the explicit class method ensures forward compatibility.
Adding parent::tearDown() ensures the base class cleanup logic executes after removing test artifacts, following PHPUnit best practices. This change is consistent with similar improvements across other test files in this PR.
Adding parent::tearDown() ensures the base class cleanup logic executes after all test-specific cleanup operations, following PHPUnit best practices. This change is consistent with similar improvements across other test files in this PR.
The rehash_on_login option has been added to support Laravel 11's automatic password rehashing feature. Setting it to false by default is appropriate, making this feature opt-in to avoid unexpected behavior changes during the framework upgrade.
The removal of legacy PHPUnit attributes (backupGlobals, backupStaticAttributes, convertErrorsToExceptions, etc.) is appropriate for PHPUnit 10/11 compatibility. The essential bootstrap and colors attributes are retained.
.github/workflows/manifest.yml (2)
18-18: Verify the token change.
The environment variable changed from potentially using phpVersion, extensions, and key to using GITHUB_TOKEN (mapped from COMPOSER_GITHUB_TOKEN). Ensure that COMPOSER_GITHUB_TOKEN secret exists and has appropriate permissions for the manifest update workflow.
26-27: LGTM! Explicit version configuration improves clarity.
Hardcoding PHP version 8.4 and the extension list makes the workflow more explicit and easier to maintain compared to environment variables. The extension list is comprehensive for Winter CMS requirements.
Moving the imports earlier in the file improves code organization.
56-56: Essential: parent::tearDown() call added.
Adding the parent::tearDown() call is critical for PHPUnit 10+ compatibility. This ensures the parent class's cleanup logic executes properly, preventing potential test pollution or resource leaks.
Explicitly declaring the $fixturePath property improves type safety and aligns with PHP 8.2+ best practices.
27-29: LGTM! Proper initialization order.
Moving the node_modules existence check to occur after fixturePath is initialized prevents potential issues with accessing an uninitialized property. This matches the pattern used in similar test classes like MixInstallTest.
12-17: LGTM! Property and constructor added for trait testing.
Adding the $controller property and constructor properly initializes the dependency required by the WidgetMaker trait. This improves the test fixture setup.
40-42: Required: Anonymous class replaces deprecated getObjectForTrait.
PHPUnit 10 removed the getObjectForTrait() method. Using an anonymous class with the trait is the correct modern approach for testing traits.
composer.json (5)
32-32: LGTM! PHP 8.2 required for Laravel 12.
Bumping the PHP requirement to ^8.2 is necessary for Laravel 12 compatibility and aligns with the PR objectives.
33-36: LGTM! Winter dependencies updated to 1.3 branch.
Switching Winter ecosystem dependencies from dev-develop to dev-wip/1.3 aligns with the PR's goal of supporting Laravel 12 in the 1.3 release branch.
37-37: LGTM! Laravel 12 framework adopted.
Upgrading from Laravel ^9.1 to ^12.0 is the core objective of this PR.
41-41: LGTM! PHPUnit 11 required.
Upgrading PHPUnit from ^9.5.8 to ^11.0 is necessary for PHP 8.2+ support and includes breaking changes addressed throughout the test suite.
46-53: Verify forked PHPUnit Arraysubset-Asserts dependency
File composer.json (lines 46–53)
• Confirm the add-phpunit-11-support branch in pieterocp/phpunit-arraysubset-asserts has recent commits and ongoing maintenance
• If it’s stale, switch to an official PHPUnit 11-compatible package or merge upstream fixes to avoid relying on an unmaintained fork
config/app.php (1)
5-29: Documentation updates look good.
The updated comments improve clarity and align with Laravel 12 conventions.
.github/workflows/tests.yml (3)
10-12: LGTM: Concurrency control properly configured.
The concurrency configuration will cancel in-progress test runs when new commits are pushed, which improves CI efficiency and aligns with the previous review suggestion.
27-27: LGTM: Checkout action updated to latest version.
Using actions/checkout@v4 is appropriate and ensures compatibility with current GitHub Actions infrastructure.
Also applies to: 77-77
69-69: LGTM: PHP version matrix updated for Laravel 12.
The matrix now includes PHP 8.2, 8.3, and 8.4, which aligns with the Laravel 12 and PHP 8.2+ requirements mentioned in the PR objectives.
modules/system/phpunit.xml (1)
2-9: LGTM: PHPUnit configuration updated for PHPUnit 10.5+.
The configuration changes are correct for PHPUnit 10/11 compatibility:
Added XML schema namespace and location
Replaced deprecated backupStaticAttributes with backupStaticProperties
Added cacheDirectory for improved test performance
Removed deprecated convertErrorsToExceptions, convertNoticesToExceptions, and convertWarningsToExceptions flags
These changes align with PHPUnit 10/11 requirements mentioned in the PR objectives.
Adding the import for PHPUnit\Framework\Attributes\DataProvider is necessary for using the attribute-based data provider syntax in PHPUnit 10+.
24-38: LGTM: Data providers converted to static methods.
Converting invalidPathsProvider and validPathsProvider to static methods is required when using the #[DataProvider] attribute in PHPUnit 10+. The data itself remains unchanged and correct.
Also applies to: 40-63
65-65: LGTM: Data provider annotations migrated to attributes.
Replacing docblock @dataProvider annotations with #[DataProvider] attributes is the correct approach for PHPUnit 10+ and aligns with the broader PHPUnit modernization across the repository.
40-63: LGTM: Simplified by delegating to parent implementation.
The change to call parent::createApplication() is a good refactoring that reduces code duplication. The parent TestCase::createApplication() handles the common bootstrap logic (app initialization, cache driver, locale, encryption key, Kernel binding), while this method continues to handle plugin-specific setup (backend.auth singleton, database configuration, plugin path).
This aligns with the broader test infrastructure modernization in the Laravel 12 upgrade.
Using explicit facade imports (Illuminate\Support\Facades\Log and Winter\Storm\Support\Facades\DB) rather than relying on global aliases is a good practice that improves code clarity and IDE support.
24-24: Verify the DB facade import and casing. Ensure that Winter\Storm\Support\Facades\DB exists and is correctly imported, and that no Db:: references remain in tests.
modules/cms/phpunit.xml (1)
2-9: LGTM: PHPUnit configuration updated for PHPUnit 10.5+.
The configuration changes mirror those in modules/system/phpunit.xml and are correct for PHPUnit 10/11 compatibility. The changes maintain consistency across module test configurations.
modules/cms/tests/classes/CmsObjectTest.php (2)
8-8: LGTM: Depends attribute imported.
Adding the import for PHPUnit\Framework\Attributes\Depends is necessary for using the attribute-based test dependency syntax in PHPUnit 10+.
240-240: LGTM: Test dependencies migrated to attributes.
Replacing docblock @depends annotations with #[Depends] attributes is the correct approach for PHPUnit 10+ and maintains the same test execution order and dependency relationships.
Also applies to: 267-267, 288-288
modules/cms/tests/classes/ControllerTest.php (2)
6-12: Imports modernization looks good.
Using facades and the shared TestCase aligns with the new bootstrap.
83-83: Good switch to Cache facade.
Consistent with the rest of the test suite. No issues.
modules/system/tests/bootstrap/TestCase.php (1)
87-109: Assertion shims are correct for PHPUnit 10/11.
Good compatibility wrappers to replace deprecated methods.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🔴 Critical
🧩 Analysis chain
Verify the default debug value in production.
Casting to boolean is correct, but the default value is true. In production environments, this should typically default to false to avoid exposing sensitive information. Consider if the default should be false instead of true for security best practices.
🌐 Web query:
What is the recommended default value for APP_DEBUG in Laravel 12 production environments?
💡 Result:
Set APP_DEBUG to false in production — e.g. APP_DEBUG=false. [1][2]
Default APP_DEBUG to false in production
In config/app.php (line 49), change to:
'debug' => (bool) env('APP_DEBUG', false),
to prevent exposing sensitive information.
🤖 Prompt for AI Agents
In config/app.php around line 49, the APP_DEBUG default is currently true which
can leak sensitive info in production; change the env default to false by
updating the 'debug' line to use env('APP_DEBUG', false) so the application
defaults to non-debug mode unless explicitly enabled via environment.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
Kernel override likely ineffective after initial resolve; ensure the new instance is used.
You resolve/boot the Kernel on Line 22, then bind a replacement. Because the Kernel singleton is already instantiated, later resolves return the original instance, so call() won’t be overridden.
Apply one of these fixes right after bootstrapping:
- $app->bind(Kernel::class, function (Application $app) {- return new class($app, $app->make(Dispatcher::class)) extends \Winter\Storm\Foundation\Console\Kernel {+ // Replace the existing Kernel instance so Artisan uses our override.+ $app->forgetInstance(Kernel::class);+ $app->forgetInstance('Illuminate\Contracts\Console\Kernel'); // alias safety+ $app->instance(Kernel::class, new class($app, $app->make(Dispatcher::class)) extends \Winter\Storm\Foundation\Console\Kernel {
public function call($command, array $parameters = [], $outputBuffer = null)
{
$result = parent::call($command, $parameters, $outputBuffer);
$shellVerbosity = 0;
if (\function_exists('putenv')) {
@putenv('SHELL_VERBOSITY=' . $shellVerbosity);
}
$_ENV['SHELL_VERBOSITY'] = $_SERVER['SHELL_VERBOSITY'] = $shellVerbosity;
return $result;
}
- };- });+ });
Optionally also set SHELL_VERBOSITY=0 once in createApplication to cover non-Artisan paths.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In modules/system/tests/bootstrap/TestCase.php around lines 30 to 52, the Kernel
override is bound after the Kernel has already been resolved so the running
Kernel instance won’t use the new call() implementation; to fix this either (a)
replace the existing resolved instance immediately after bootstrapping using
$app->forgetInstance(Kernel::class) and then bind the anonymous class, or (b)
directly register the new Kernel instance via $app->instance(Kernel::class, new
class($app, $app->make(Dispatcher::class)) extends
\Winter\Storm\Foundation\Console\Kernel { ... }), and additionally set
SHELL_VERBOSITY=0 once in createApplication (or immediately after bootstrapping)
to cover non-Artisan execution paths.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🔴 Critical
Fix reversed array_get arguments; index membership never validated
Args are flipped, so $indexColumns is always null and the check is skipped. Correct it to actually validate index columns.
- if ($indexColumns = array_get($indexName, $tableIndexes)) {- $this->assertTrue(in_array($name, $indexColumns));+ if ($indexColumns = array_get($tableIndexes, $indexName)) {+ $this->assertTrue(in_array($name, $indexColumns, true));
}
📝 Committable suggestion
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
if ($indexColumns = array_get($indexName, $tableIndexes)) {
In modules/system/tests/console/CreateMigrationTest.php around lines 80-81, the
call to array_get has its arguments reversed so $indexColumns is always null and
the membership assertion is skipped; swap the arguments to read
array_get($tableIndexes, $indexName) (optionally provide a default) so
$indexColumns actually contains the index columns, then run the existing
in_array assertion against that result.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
All tests passing. Requires wintercms/storm#207. Replaces #1094
Remaining Tasks:
Breaking changes:
array_first()andarray_last()functions that do not match the signature provided by Winter's helper functions. If you are currently using either function in your code with two or three arguments (filter & default) you will need to switch to\Winter\Storm\Support\Arr::first()or\Winter\Storm\Support\Arr:last()instead.-sshort flag for the--silentoption onmix:compile,mix:create,mix:install,mix:watch,npm:install,npm:run,npm:update,npm:version,vite:compile,vite:create,vite:install,vite:watchhas been removed as Symfony v7.2 added the--silentoption to all commands by default which conflicted with our definition. You must use the full option--silentgoing forward.static $defaultNamein Console commands to support lazy loading is no longer used in Symfony\Console 7.2, use theAsCommandclass attribute instead. We will need to update all first party plugins & modules to make use of that.Currently incompatible plugins:
All other Winter plugins have been tested and confirmed compatible (some with minor tweaks to their PHPUnit configs to remove deprecation warnings.
New Minimum Requirements:
Backwards Compatibility Fixes:
Upgrade Guides:
Summary by CodeRabbit
New Features
Bug Fixes
CLI Changes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.