Support Laravel 12 #207
Support Laravel 12 #207
Conversation
Release v1.2.1
Co-authored-by: Luke Towers <git@luketowers.ca>
Co-authored-by: Wim Verhoogt <wim@verbant.nl> Co-authored-by: Ben Thomson <git@alfreido.com>
Can remove once laravel/framework#56392 is merged.
laravel/framework#49730 added the ContextServiceProvider
Laravel 10 made it so that raw expression objects cannot be cast directly into strings anymore. There will be other places that will need to be updated.
WalkthroughUpdates CI workflows and tooling, raises PHP/dependency constraints, expands PHPStan baseline, and introduces extensive database refactors: new PDO/DBAL drivers, connection trait, per-driver connection classes and schema grammars. Also adds model/trait adjustments, Foundation/API tweaks, many test-suite refactors and new grammar tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Conn as Winter Connection
participant Trait as HasConnection
participant Grammar as SchemaGrammar
participant DBAL as Doctrine Driver
participant PDO as PDO
App->>Conn: request query()/getDoctrineConnection()
Conn->>Trait: delegate query/logging/getDoctrineConnection
Trait->>Grammar: use grammar & post-processor for builder
alt Doctrine available
Trait->>DBAL: register/connect using PDO
DBAL->>PDO: use wrapped PDO instance
DBAL-->>Conn: return DBAL connection/adapters
else
Trait-->>Conn: using native schema operations
end
sequenceDiagram
autonumber
participant Dev as Migration/Blueprint
participant Conn as Connection
participant Gram as SchemaGrammar
participant DB as Database
Dev->>Conn: run Blueprint->change()
Conn->>Gram: compileChange(blueprint, command)
Gram->>Conn: query schema (columns, indexes, fks)
Gram->>Gram: build ALTER statements per-column
Gram-->>Conn: SQL statements
Conn->>DB: execute statements
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Areas to focus on:
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review this |
🧩 Analysis chain🏁 Script executed: Length of output: 38648 🏁 Script executed: Length of output: 7323 🏁 Script executed: Length of output: 9009 🏁 Script executed: Length of output: 6790 🏁 Script executed: Length of output: 2808 🏁 Script executed: Length of output: 7823 @LukeTowers I'll review this PR. Based on my analysis, here's what I found: OverviewThis is a massive undertaking upgrading from Laravel 9 to 12 with 2,538 additions and 854 deletions across ~100 files. The scope includes:
Critical Issues🔴 Code Analysis FailingThe Code Analysis CI check is currently failing while all unit tests pass. This needs to be addressed before merge. 🟡 PHPStan Baseline ExplosionThe PHPStan baseline grew from 136 to 513 issues (+377 new issues). This is concerning as it indicates:
Examples of new baseline issues:
🟡 Unresolved Migration System ConcernsFrom the comment chain, @mjauvin expressed:
Question: What specific migration system issues were you encountering? The PR shows the Blueprint resolver was updated, but are there remaining concerns about:
Architectural Changes✅ Database Layer RefactoringNew PDO Drivers (for Doctrine v3 compatibility):
Connection Refactoring:
Schema Grammar Updates:
✅ Positive Aspects
RecommendationsBefore Merge
Consider
Questions
The core architecture looks solid, but the PHPStan baseline growth and Code Analysis failure need attention. ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Network/Http.php (1)
553-553: IncorrectCURLOPT_SSL_VERIFYHOSTvalue inverifySSL()method.Line 553 sets
CURLOPT_SSL_VERIFYHOSTtotrue, which coerces to integer1. According to PHP's cURL documentation, this option should be set to2(not1ortrue) to properly verify that the certificate's Common Name or Subject Alternate Name matches the hostname.Using
1is deprecated and may not perform proper hostname verification.Apply this diff to fix the SSL host verification:
public function verifySSL() { $this->setOption(CURLOPT_SSL_VERIFYPEER, true); - $this->setOption(CURLOPT_SSL_VERIFYHOST, true); + $this->setOption(CURLOPT_SSL_VERIFYHOST, 2); return $this; }src/Database/MigrationServiceProvider.php (1)
12-14: Restore or replace dropped migration commands
src/Database/MigrationServiceProvider.phpclears all Laravel migration commands, and no*Migrate*Commandclasses or registrations were found in the codebase.- Implement and register equivalent migration commands or revert to Laravel’s defaults in
MigrationServiceProvider.- Document the new migration workflow for Winter CMS users and plugin developers.
src/Database/Connections/MySqlConnection.php (1)
70-79: Bug: floats bound as integers (loss of precision) and null/bool not handledSame issue as MariaDbConnection: float values get truncated; handle null and booleans explicitly.
public function bindValues($statement, $bindings) { foreach ($bindings as $key => $value) { $statement->bindValue( is_string($key) ? $key : $key + 1, $value, - is_int($value) || is_float($value) ? PDO::PARAM_INT : PDO::PARAM_STR + is_int($value) ? PDO::PARAM_INT + : (is_bool($value) ? PDO::PARAM_BOOL + : (is_null($value) ? PDO::PARAM_NULL + : PDO::PARAM_STR)) ); } }Consider extracting a shared bindValueType($value): int helper (or moving this into HasConnection) to avoid duplication.
♻️ Duplicate comments (1)
src/Database/Traits/Revisionable.php (1)
160-163: Guard access to $this->dates to avoid notices; use strict checksAccessing an undefined $dates can trigger notices on some models. Add property_exists/is_array and strict in_array.
Apply:
- if (in_array($attribute, $this->getDates()) || $this->isDateCastable($attribute) || in_array($attribute, $this->dates)) { + if ( + in_array($attribute, $this->getDates(), true) + || $this->isDateCastable($attribute) + || (property_exists($this, 'dates') && is_array($this->dates) && in_array($attribute, $this->dates, true)) + ) { return 'date'; }Longer-term, consider centralizing legacy $dates → $casts migration (or have getDates() include $dates) so callers don’t need to handle both.
🧹 Nitpick comments (48)
src/Foundation/Bootstrap/LoadConfiguration.php (1)
45-58: Replace the ignore flags withEnv::getUsing
// @phpstan-ignore-next-linetwice suppresses legitimate checks and can hide future issues. Instead, pull inIlluminate\Support\Env(already present in Laravel 12) and callEnv::get()so PHPStan understands the symbol:+use Illuminate\Support\Env; ... - // @phpstan-ignore-next-line - return env('APP_ENV', array_get($config, 'default', 'production')); + return Env::get('APP_ENV', array_get($config, 'default', 'production')); ... - // @phpstan-ignore-next-line - $environment = env('APP_ENV'); + $environment = Env::get('APP_ENV');This keeps behavior identical while eliminating the need for ignores.
src/Foundation/Http/Kernel.php (1)
35-42: LGTM: Property rename aligns with Laravel 11+ requirements.The property rename from
$routeMiddlewareto$middlewareAliasesis correct and necessary for Laravel 12 compatibility. Laravel deprecated$routeMiddlewarein v11 and replaced it with$middlewareAliases.The middleware configuration is appropriate:
- Commented middleware (auth, auth.basic, can, guest) are application-level concerns that Winter CMS applications should define themselves
- Retained middleware (bindings, throttle) are framework-level routing infrastructure
Optional: Clean up commented lines
Consider removing the commented middleware lines entirely rather than keeping them as comments, since they represent application-level middleware that shouldn't be defined at the framework level:
protected $middlewareAliases = [ - // 'auth' => \Illuminate\Auth\Middleware\Authenticate::class, - // 'auth.basic' => \Illuminate\Auth\Middleware\AuthenticateWithBasicAuth::class, 'bindings' => \Illuminate\Routing\Middleware\SubstituteBindings::class, - // 'can' => \Illuminate\Auth\Middleware\Authorize::class, - // 'guest' => \App\Http\Middleware\RedirectIfAuthenticated::class, 'throttle' => \Illuminate\Routing\Middleware\ThrottleRequests::class, ];However, keeping the comments may serve as documentation for Winter CMS users showing which middleware aliases are available in Laravel but intentionally omitted from the framework base.
tests/Foundation/ProviderRepositoryTest.php (1)
49-49: Fix typos in method names.Both method names contain the typo "Functionaliy" instead of "Functionality".
- public function testOriginalFunctionaliy(): void + public function testOriginalFunctionality(): void- public function testWinterFunctionaliy(): void + public function testWinterFunctionality(): voidAlso applies to: 67-67
src/Support/Facades/File.php (1)
47-47: Consider using the short form for consistency.Line 47 uses the fully qualified name
\Illuminate\Filesystem\FilesystemAdapter, while lines 57–58 use the short formFilesystemAdapter(now that the import is added). For consistency, consider updating line 47 to use the short form as well.Apply this diff to use the short form:
-* @method static bool isLocalDisk(\Illuminate\Filesystem\FilesystemAdapter $disk) +* @method static bool isLocalDisk(FilesystemAdapter $disk)tests/Parse/YamlTest.php (1)
10-223: Consider addressing the skipped tests.Multiple tests are currently skipped with TODO comments indicating a need to rethink YAML processing logic. While this technical debt predates the current PR, these skipped tests reduce coverage of the YAML parser functionality.
If you'd like, I can help draft a plan to address these TODOs or open an issue to track this improvement.
tests/Html/BlockBuilderTest.php (1)
7-7: Optional: Nullable type is defensively safe but not strictly necessary.The
?BlockBuilder $Block = nulldeclaration is defensive programming sincesetUp()always initializes$Blockbefore any test method runs. The nullable type and default value don't cause issues but add slight verbosity. You could simplify toprotected BlockBuilder $Block;if preferred, though the current approach is also acceptable.tests/Support/EventFakeTest.php (1)
8-8: Optional: Nullable type is unnecessary here.Since
setUp()always initializes$fakerbefore every test method (per PHPUnit's contract), the nullable type adds minimal defensive value. A non-nullable typed property without a default would suffice:- protected ?EventFake $faker = null; + protected EventFake $faker;That said, the explicit null initialization is harmless and may satisfy stricter static analysis tools or coding standards in the upgraded environment.
tests/Halcyon/ValidationTraitTest.php (2)
3-6: LGTM: Valid PHPUnit 11 modernization pattern.The
ValidationTraitHosthelper class correctly replaces the deprecatedgetMockForTrait()approach from older PHPUnit versions. This pattern is appropriate for testing traits in PHPUnit 11.Optionally, consider adding a docblock to clarify the helper's purpose:
+/** + * Helper class to test the Validation trait. + */ class ValidationTraitHost { use \Winter\Storm\Halcyon\Traits\Validation; }
12-12: Align trait testing with PHPUnit 11 test-double APIs
Replacenew ValidationTraitHost()with$this->createStub(ValidationTraitHost::class)or use the MockBuilder fluent API ($this->getMockBuilder(ValidationTraitHost::class)->disableOriginalConstructor()->getMock()) to follow PHPUnit 11 recommendations.src/Extension/ExtendableTrait.php (1)
550-563: Tighten param type to ReflectionClass and drop the phpstan ignoreUse a typed ReflectionClass parameter and an instanceof check to remove the need for the ignore and ensure $reflector is always a ReflectionClass.
- protected function extensionGetParentClass(?object $instance = null) + protected function extensionGetParentClass(?ReflectionClass $instance = null) { // Shortcut to prevent infinite loops if the class extends Extendable. if ($this instanceof Extendable) { return false; } // Find if any parent uses the Extendable trait - if (!is_null($instance)) { - $reflector = $instance; - } else { - // @phpstan-ignore-next-line - $reflector = new ReflectionClass($this); - } + if ($instance instanceof ReflectionClass) { + $reflector = $instance; + } else { + /** @var ReflectionClass<object> $reflector */ + $reflector = new ReflectionClass($this); + } $parent = $reflector->getParentClass();phpstan.neon (1)
9-10: Avoid broad analysis downgrades; scope ignoresGlobal ignores degrade signal:
- trait.unused: prefer path-specific baseline entries for known false-positives.
- treatPhpDocTypesAsCertain: false reduces type precision; consider enabling later per-path if needed.
Add reportUnmatchedIgnoredErrors to prevent stale ignores.
Proposed tweaks:
parameters: paths: - src level: 5 - ignoreErrors: - - identifier: trait.unused + ignoreErrors: + # Consider moving specific trait.unused cases to baseline with path+count excludePaths: # Exclude PHP Parser files - src/Parse/PHP/ArrayFile.php - src/Foundation/Console/KeyGenerateCommand.php - src/Scaffold/GeneratorCommand.php disableSchemaScan: true databaseMigrationsPath: - src/Auth/Migrations - src/Database/Migrations treatPhpDocTypesAsCertain: false + reportUnmatchedIgnoredErrors: truePlease confirm Code Analysis passes with these settings or if trait.unused must remain global for now.
Also applies to: 20-20
.github/workflows/tests.yml (1)
37-39: Guard Composer token step to avoid empty auth writes on forksOn pull_request from forks, secrets are unavailable. Guarding prevents writing an empty token to global auth.
- - name: Configure Composer GitHub token - run: composer config --global --auth github-oauth.github.com ${{ secrets.COMPOSER_GITHUB_TOKEN }} + - name: Configure Composer GitHub token + if: ${{ secrets.COMPOSER_GITHUB_TOKEN != '' }} + run: composer config --global --auth github-oauth.github.com ${{ secrets.COMPOSER_GITHUB_TOKEN }}phpstan-baseline.neon (1)
1-1039: Consolidate and incrementally reduce phpstan baseline ignoresThe current baseline masks many real mismatches (e.g. return.type, method.childParameterType, private Relation methods, property.notFound on ColumnDefinition/Fluent). Recommend:
- Group ignores by identifier category (Relations, Grammar properties, Mailer, etc.) and tackle each in turn.
- Introduce stubs or generics for dynamic Eloquent APIs to eliminate most method.notFound entries.
- Keep ignore counts minimal and scoped to paths; annotate each block with rationale.
- Open a follow-up issue to track and plan incremental baseline cleanup.
To verify baseline ignores locally:
#!/usr/bin/env bash # Locate phpstan PHPSTAN="./vendor/bin/phpstan" [ ! -x "$PHPSTAN" ] && PHPSTAN="$(which phpstan 2>/dev/null || echo phpstan)" $PHPSTAN analyse --no-progress --error-format=github || true rg -oP 'identifier:\s*\K\S+' phpstan-baseline.neon | sort | uniq -c | sort -nrtests/Database/ModelTest.php (1)
10-10: LGTM! Test base class standardized.The migration to the fully-qualified
\Winter\Storm\Tests\DbTestCasealigns with the broader test suite modernization effort.Consider removing the now-redundant import at Line 8 since you're using the fully-qualified class name:
-use Winter\Storm\Tests\DbTestCase;tests/Database/Traits/SimpleTreeTest.php (1)
9-9: LGTM! Test base class standardized.The fully-qualified base class usage is consistent with the test suite modernization.
The import at Line 7 can be removed now that you're using the fully-qualified class name:
-use Winter\Storm\Tests\DbTestCase;tests/Database/Traits/SortableTest.php (1)
7-7: LGTM! Test base class standardized.The switch to fully-qualified
\Winter\Storm\Tests\DbTestCaseis consistent with the test suite refactor.Consider removing the now-unused import at Line 5:
-use Winter\Storm\Tests\DbTestCase;tests/Database/Traits/NestedTreeTest.php (1)
9-9: LGTM! Test base class standardized.The fully-qualified base class aligns with the broader test infrastructure modernization.
The import at Line 7 is now redundant and can be removed:
-use Winter\Storm\Tests\DbTestCase;tests/Database/Relations/AttachManyTest.php (1)
8-8: Remove unused import.The import statement for
DbTestCaseis no longer needed since line 10 uses the fully-qualified class name in the extends clause.Apply this diff to remove the redundant import:
-use Winter\Storm\Tests\DbTestCase;tests/Database/Traits/DeferredBindingTest.php (1)
9-9: Remove unused import.The import statement for
DbTestCaseis no longer needed since line 11 uses the fully-qualified class name in the extends clause.Apply this diff to remove the redundant import:
-use Winter\Storm\Tests\DbTestCase;tests/Database/Traits/SluggableTest.php (1)
6-6: Remove unused import.The import statement for
DbTestCaseis no longer needed since line 8 uses the fully-qualified class name in the extends clause.Apply this diff to remove the redundant import:
-use Winter\Storm\Tests\DbTestCase;tests/Database/Relations/HasManyThroughTest.php (1)
10-10: Optional: Remove redundant import.Since the class now extends the fully-qualified
\Winter\Storm\Tests\DbTestCaseon line 12, the import on line 10 is no longer needed.Apply this diff to remove the redundant import:
-use Winter\Storm\Tests\DbTestCase;tests/Database/Relations/DuplicateRelationTest.php (1)
9-9: Optional: Remove redundant import.The import is no longer needed since line 11 uses the fully-qualified class name.
Apply this diff:
-use Winter\Storm\Tests\DbTestCase;tests/fixtures/events/EventTest.php (1)
5-13: Drop TestCase inheritance and dummy code in EventTest fixture
In tests/fixtures/events/EventTest.php removeextends TestCase, the custom__constructoverride and thetestDummy()method so that EventTest is a plain event payload class.tests/Database/UpdaterTest.php (1)
5-14: Call parent::setUp() when extending the Winter test baseSwitch to the FQ base class is fine, and nullable property is OK. Add parent::setUp() to ensure framework bootstrap runs.
public function setUp(): void { + parent::setUp(); include_once __DIR__.'/../fixtures/database/SampleClass.php'; $this->updater = new Updater(); }tests/Support/MailFakeTest.php (1)
4-14: Good MailManager injection; add Mockery teardownThe manager mock + MailFake swap is correct. Ensure Mockery is closed to prevent leaks; either use the PHPUnit integration trait or close in tearDown.
Option A (preferred): use the integration trait.
use Mockery as m; +use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; class MailFakeTest extends \Winter\Storm\Tests\TestCase { + use MockeryPHPUnitIntegration;Option B: close Mockery in tearDown (if you prefer not to use the trait).
protected function tearDown(): void { m::close(); parent::tearDown(); }Also applies to: 20-22
tests/Halcyon/HalcyonModelTest.php (1)
10-26: Add parent::setUp() with new base test classThe new base class likely requires bootstrap. Insert parent::setUp() at the start of setUp().
public function setUp(): void { + parent::setUp(); include_once __DIR__.'/../fixtures/halcyon/models/Page.php'; include_once __DIR__.'/../fixtures/halcyon/models/Menu.php'; include_once __DIR__.'/../fixtures/halcyon/models/Content.php';tests/Filesystem/FilesystemTest.php (1)
3-4: LGTM; consider adding return types to providersAttribute migration and static providers look good. Optionally add : array return types to provider methods for clarity.
Example:
- public static function provideSizesForSizeToBytes() + public static function provideSizesForSizeToBytes(): arrayAlso applies to: 6-7, 15-17, 24-35, 36-43, 44-58, 60-67, 68-80, 82-89, 90-100
src/Database/Schema/Grammars/Concerns/MySqlBasedGrammar.php (1)
11-23: Suppress PHPMD for intentionally unused $commandSignature must match Laravel’s interface; add suppression to avoid PHPMD noise.
Add to the docblock:
/** * Compile a change column command into a series of SQL statements. @@ * @return array|string * * @throws \RuntimeException + * @SuppressWarnings(PHPMD.UnusedFormalParameter) */tests/Database/Relations/HasOneThroughTest.php (1)
11-11: LGTM! Test base class standardization.The change to use the fully qualified
\Winter\Storm\Tests\DbTestCasebase class is correct and aligns with the PR-wide test suite refactoring. This makes the base class reference explicit and avoids potential namespace resolution issues.Optional refinement: The import statement on Line 9 (
use Winter\Storm\Tests\DbTestCase;) is now redundant since the fully qualified name is used in the extends clause. Consider removing it for consistency.tests/Database/Traits/NullableTest.php (1)
8-8: LGTM! Test base class correctly updated.The fully qualified base class reference is correct and consistent with the PR's test suite standardization.
Optional refinement: The import on Line 6 (
use Winter\Storm\Tests\DbTestCase;) is now redundant and could be removed.tests/Database/Relations/AttachOneTest.php (1)
12-12: LGTM! Consistent with test suite refactoring.The base class update to
\Winter\Storm\Tests\DbTestCaseis correct.Optional refinement: Line 10's import statement is now redundant.
tests/Database/Relations/BelongsToManyTest.php (1)
13-13: LGTM! Base class update is correct.The fully qualified
\Winter\Storm\Tests\DbTestCasereference aligns with the PR's test standardization.Optional refinement: The import statement on Line 11 is now redundant.
tests/Database/Concerns/HasRelationshipsTest.php (1)
24-24: LGTM! Test base class standardization complete.The update to use
\Winter\Storm\Tests\DbTestCaseis correct and finalizes the test suite refactoring pattern seen across all reviewed files.Optional refinement: Line 22's import statement is now redundant and could be cleaned up.
src/Database/PDO/SQLiteDriver.php (1)
12-18: Add: stringreturn type togetName()in all PDO driver classes
Doctrine DBAL is at ^3.0 and PHP 8+ supports covariant return types. Update eachsrc/Database/PDO/*Driver.php:- public function getName() + public function getName(): stringsrc/Database/PDO/SqlServerDriver.php (1)
18-23: Consider using theConnectsToDatabasetrait for consistency.Unlike the MySQL and PostgreSQL drivers which use the
ConnectsToDatabasetrait, this driver reimplements the connection logic. This creates code duplication and pattern inconsistency.If the wrapping with
SqlServerConnection(instead of plainConnection) is necessary, consider documenting why this driver requires different handling. Otherwise, apply this refactor:class SqlServerDriver extends AbstractSQLServerDriver { + use ConnectsToDatabase; + - /** - * Create a new database connection. - * - * @param mixed[] $params - * @param string|null $username - * @param string|null $password - * @param mixed[] $driverOptions - * @return \Winter\Storm\Database\PDO\SqlServerConnection - */ - public function connect(array $params, $username = null, $password = null, array $driverOptions = []) - { - return new SqlServerConnection( - new Connection($params['pdo']) - ); - } - /** * {@inheritdoc} */Note: You may need to verify whether
SqlServerConnectionprovides necessary server-specific behavior that justifies the custom wrapper.tests/Database/Schema/Grammars/PostgresSchemaGrammarTest.php (1)
6-10: Consistent class referenceYou import GrammarTestCase but extend via FQCN. Minor style nit: extend the imported name for consistency.
-use Winter\Storm\Tests\GrammarTestCase; +use Winter\Storm\Tests\GrammarTestCase; -class PostgresSchemaGrammarTest extends \Winter\Storm\Tests\GrammarTestCase +class PostgresSchemaGrammarTest extends GrammarTestCasesrc/Database/Connectors/ConnectionFactory.php (1)
17-21: Outdated/misleading comment about “uncatchable” exceptionThis comment references an override “below” that no longer exists here (only createConnection is overridden). Consider updating or removing to avoid confusion.
src/Database/Schema/Grammars/PostgresGrammar.php (1)
25-26: Unused parameter ($command) — silence PHPMD or use itTo satisfy PHPMD without altering the signature, explicitly unset the parameter.
public function compileChange(Blueprint $blueprint, Fluent $command) { + unset($command);Alternatively, add:
@SuppressWarnings(PHPMD.UnusedFormalParameter)to the method docblock. [Based on static analysis hints]tests/GrammarTestCase.php (2)
13-15: Add type declarations for properties.The properties lack type hints, reducing type safety and IDE support. Declare explicit types for
$connection,$grammarClass, and$builderClass.Apply this diff:
- protected $connection = null; - protected $grammarClass = null; - protected $builderClass = null; + protected ?\Mockery\MockInterface $connection = null; + protected ?string $grammarClass = null; + protected ?string $builderClass = null;
21-29: Simplify the mock setup pattern.The mock is created on lines 21-25, then immediately reassigned on lines 27-29. This can be consolidated into a single chain for better readability.
Apply this diff:
- $this->connection = m::mock(Connection::class) - ->shouldReceive('getTablePrefix')->andReturn('') - ->shouldReceive('getServerVersion')->andReturn('3.35') - ->shouldReceive('scalar')->andReturn('') - ->getMock(); - - $this->connection = $this->connection - ->shouldReceive('getSchemaGrammar')->andReturn($this->getSchemaGrammar()) + $this->connection = m::mock(Connection::class) + ->shouldReceive('getTablePrefix')->andReturn('') + ->shouldReceive('getServerVersion')->andReturn('3.35') + ->shouldReceive('scalar')->andReturn('') + ->shouldReceive('getSchemaGrammar')->andReturn($this->getSchemaGrammar()) ->getMock();src/Database/Schema/Grammars/SqlServerGrammar.php (2)
60-67: Only strip surrounding quotes; don’t remove inner quotes in defaultspreg_replace removes all single quotes, corrupting defaults like O'Connor. Strip only the outer pair.
Apply:
- public function getDefaultValue($value) + public function getDefaultValue($value) { - if (is_string($value)) { - $value = preg_replace('#\'#', '', $value); - } + if (is_string($value) && strlen($value) >= 2 && $value[0] === "'" && substr($value, -1) === "'") { + $value = substr($value, 1, -1); + } return parent::getDefaultValue($value); }
25-58: Optionally drop empty statements from the change batchDefensive cleanup to avoid executing empty statements if upstream returns an empty drop-default SQL.
- return $changes; + return array_values(array_filter($changes));src/Database/Connections/Connection.php (1)
5-8: Use a proper docblock and add deprecation detailsCurrent comment isn’t parsed as a docblock. Include since/version and replacement for clarity.
-/* - * @deprecated - */ +/** + * @deprecated 1.3 Use the concrete driver connections (e.g. MySqlConnection, MariaDbConnection, PostgresConnection, SqlServerConnection) + * + HasConnection trait instead. Planned removal in 2.0. + */ abstract class Connection extends BaseConnectionsrc/Database/PDO/SqlServerConnection.php (1)
121-131: Ensure quote() never returns false; throw on failurePDO::quote can return false; pass-through violates DBAL expectations. Throw instead, then apply the null-byte workaround.
- public function quote($value, $type = ParameterType::STRING) + public function quote($value, $type = ParameterType::STRING) { - $val = $this->connection->quote($value, $type); - - // Fix for a driver version terminating all values with null byte... - if (\is_string($val) && str_contains($val, "\0")) { - $val = \substr($val, 0, -1); - } - - return $val; + $val = $this->connection->quote($value, $type); + if ($val === false) { + throw new \RuntimeException('Failed to quote value for SQL Server.'); + } + // Fix for a driver version terminating all values with null byte... + if (str_contains($val, "\0")) { + $val = substr($val, 0, -1); + } + return $val; }src/Database/PDO/Connection.php (1)
1-5: Optional: enable strict types for safer typingAdd strict_types to catch type issues earlier.
-<?php +<?php +declare(strict_types=1);src/Database/Schema/Grammars/SQLiteGrammar.php (2)
151-158: Default value sanitization is too aggressiveRemoving all single quotes changes valid literals. Trim only wrapping quotes.
public function getDefaultValue($value) { - if (is_string($value)) { - $value = preg_replace('#\'#', '', $value); - } + if (is_string($value) && strlen($value) >= 2 && $value[0] === "'" && substr($value, -1) === "'") { + $value = substr($value, 1, -1); + } return parent::getDefaultValue($value); }
28-29: Silence PHPMD UnusedFormalParameter (or mark intent)$command and $column are required by the signature but unused here.
Options:
- Add suppression on the methods’ docblocks:
- @SuppressWarnings("PHPMD.UnusedFormalParameter")
- Or explicitly mark as unused:
public function compileChange(Blueprint $blueprint, Fluent $command) { + unset($command); $autoIncrementColumn = null;protected function typeVarChar(Fluent $column) { + unset($column); return 'varchar'; }Also applies to: 166-169
src/Database/Connections/HasConnection.php (1)
151-157: Connection construction: prefer DriverManager or drop redundant 'driver' paramYou already pass a Driver instance as arg 2; the 'driver' string in params is unnecessary and can confuse config resolution. Consider DriverManager::getConnection or omitting 'driver' from params.
- $this->doctrineConnection = new DoctrineConnection(array_filter([ - 'pdo' => $this->getPdo(), - 'dbname' => $this->getDatabaseName(), - 'driver' => $driver->getName(), - 'serverVersion' => $this->getConfig('server_version'), - ]), $driver); + $this->doctrineConnection = new DoctrineConnection(array_filter([ + 'pdo' => $this->getPdo(), + 'dbname' => $this->getDatabaseName(), + 'serverVersion' => $this->getConfig('server_version'), + ]), $driver);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (165)
.github/workflows/code-analysis.yaml(1 hunks).github/workflows/code-quality.yaml(0 hunks).github/workflows/tests.yml(3 hunks).gitignore(2 hunks)composer.json(3 hunks)phpstan-baseline.neon(1 hunks)phpstan.neon(1 hunks)phpunit.xml.dist(1 hunks)src/Auth/Manager.php(1 hunks)src/Auth/Models/Throttle.php(1 hunks)src/Auth/Models/User.php(5 hunks)src/Config/ConfigWriter.php(2 hunks)src/Config/Repository.php(1 hunks)src/Console/Traits/HandlesCleanup.php(1 hunks)src/Database/Attach/File.php(2 hunks)src/Database/Behaviors/Encryptable.php(1 hunks)src/Database/Builder.php(4 hunks)src/Database/Concerns/HasAttributes.php(1 hunks)src/Database/Connections/Connection.php(1 hunks)src/Database/Connections/HasConnection.php(1 hunks)src/Database/Connections/MariaDbConnection.php(1 hunks)src/Database/Connections/MySqlConnection.php(3 hunks)src/Database/Connections/PostgresConnection.php(3 hunks)src/Database/Connections/SQLiteConnection.php(3 hunks)src/Database/Connections/SqlServerConnection.php(4 hunks)src/Database/Connectors/ConnectionFactory.php(2 hunks)src/Database/Connectors/SQLiteConnector.php(0 hunks)src/Database/DatabaseServiceProvider.php(4 hunks)src/Database/MigrationServiceProvider.php(1 hunks)src/Database/Model.php(2 hunks)src/Database/PDO/Concerns/ConnectsToDatabase.php(1 hunks)src/Database/PDO/Connection.php(1 hunks)src/Database/PDO/MySqlDriver.php(1 hunks)src/Database/PDO/PostgresDriver.php(1 hunks)src/Database/PDO/SQLiteDriver.php(1 hunks)src/Database/PDO/SqlServerConnection.php(1 hunks)src/Database/PDO/SqlServerDriver.php(1 hunks)src/Database/Query/Grammars/MariaDbGrammar.php(1 hunks)src/Database/Query/Grammars/MySqlGrammar.php(1 hunks)src/Database/Query/Grammars/PostgresGrammar.php(1 hunks)src/Database/QueryBuilder.php(2 hunks)src/Database/Relations/Concerns/DeferOneOrMany.php(3 hunks)src/Database/Schema/Grammars/Concerns/MySqlBasedGrammar.php(1 hunks)src/Database/Schema/Grammars/MariaDbGrammar.php(1 hunks)src/Database/Schema/Grammars/MySqlGrammar.php(1 hunks)src/Database/Schema/Grammars/PostgresGrammar.php(1 hunks)src/Database/Schema/Grammars/SQLiteGrammar.php(1 hunks)src/Database/Schema/Grammars/SqlServerGrammar.php(1 hunks)src/Database/Traits/Revisionable.php(1 hunks)src/Extension/ExtendableTrait.php(1 hunks)src/Flash/FlashBag.php(2 hunks)src/Foundation/Application.php(4 hunks)src/Foundation/Bootstrap/LoadConfiguration.php(2 hunks)src/Foundation/Http/Kernel.php(1 hunks)src/Foundation/Http/Middleware/CheckForMaintenanceMode.php(1 hunks)src/Halcyon/Builder.php(1 hunks)src/Halcyon/Datasource/DbDatasource.php(2 hunks)src/Halcyon/MemoryCacheManager.php(1 hunks)src/Halcyon/Model.php(1 hunks)src/Halcyon/Processors/SectionParser.php(1 hunks)src/Html/FormBuilder.php(2 hunks)src/Http/Middleware/TrustHosts.php(1 hunks)src/Mail/Mailer.php(1 hunks)src/Network/Http.php(1 hunks)src/Support/Facades/DB.php(1 hunks)src/Support/Facades/File.php(1 hunks)src/Support/Facades/Flash.php(1 hunks)src/Support/Facades/Mail.php(2 hunks)src/Support/Testing/MocksClassLoader.php(1 hunks)src/Support/helpers-array.php(1 hunks)src/Translation/FileLoader.php(2 hunks)src/Translation/Translator.php(1 hunks)tests/Config/ConfigTest.php(1 hunks)tests/Config/ConfigWriterTest.php(1 hunks)tests/Database/Attach/ResizerTest.php(1 hunks)tests/Database/Behaviors/EncryptableBehaviorTest.php(1 hunks)tests/Database/Concerns/HasRelationshipsTest.php(1 hunks)tests/Database/DongleTest.php(1 hunks)tests/Database/Factories/FactoryTest.php(1 hunks)tests/Database/ModelTest.php(1 hunks)tests/Database/MorphPivotTest.php(1 hunks)tests/Database/QueryBuilderTest.php(4 hunks)tests/Database/Relations/AttachManyTest.php(1 hunks)tests/Database/Relations/AttachOneTest.php(1 hunks)tests/Database/Relations/BelongsToManyTest.php(1 hunks)tests/Database/Relations/BelongsToTest.php(1 hunks)tests/Database/Relations/DuplicateRelationTest.php(1 hunks)tests/Database/Relations/DynamicRelationTest.php(1 hunks)tests/Database/Relations/HasManyTest.php(1 hunks)tests/Database/Relations/HasManyThroughTest.php(1 hunks)tests/Database/Relations/HasOneTest.php(1 hunks)tests/Database/Relations/HasOneThroughTest.php(1 hunks)tests/Database/Relations/MorphManyTest.php(1 hunks)tests/Database/Relations/MorphOneTest.php(1 hunks)tests/Database/Schema/Grammars/MySqlSchemaGrammarTest.php(1 hunks)tests/Database/Schema/Grammars/PostgresSchemaGrammarTest.php(1 hunks)tests/Database/Schema/Grammars/SQLiteSchemaGrammarTest.php(1 hunks)tests/Database/Schema/Grammars/SqlServerSchemaGrammarTest.php(1 hunks)tests/Database/Traits/ArraySourceTest.php(1 hunks)tests/Database/Traits/DeferredBindingTest.php(1 hunks)tests/Database/Traits/EncryptableTest.php(1 hunks)tests/Database/Traits/HasSortableRelationsTest.php(1 hunks)tests/Database/Traits/NestedTreeTest.php(1 hunks)tests/Database/Traits/NullableTest.php(1 hunks)tests/Database/Traits/PurgeableTraitTest.php(1 hunks)tests/Database/Traits/RevisionableTest.php(1 hunks)tests/Database/Traits/SimpleTreeTest.php(1 hunks)tests/Database/Traits/SluggableTest.php(1 hunks)tests/Database/Traits/SoftDeleteTest.php(1 hunks)tests/Database/Traits/SortableTest.php(1 hunks)tests/Database/Traits/ValidationTest.php(2 hunks)tests/Database/UpdaterTest.php(1 hunks)tests/DbTestCase.php(1 hunks)tests/Events/DispatcherTest.php(1 hunks)tests/Extension/ExtendableTest.php(1 hunks)tests/Extension/ExtendableTraitTest.php(2 hunks)tests/Extension/ExtensionTest.php(1 hunks)tests/Filesystem/FilesystemTest.php(5 hunks)tests/Filesystem/PathResolverTest.php(1 hunks)tests/Foundation/ApplicationTest.php(1 hunks)tests/Foundation/Http/Middleware/CheckForTrustedHostTest.php(2 hunks)tests/Foundation/Http/Middleware/CheckForTrustedProxiesTest.php(2 hunks)tests/Foundation/ProviderRepositoryTest.php(1 hunks)tests/GrammarTestCase.php(1 hunks)tests/Halcyon/DatasourceResolverTest.php(1 hunks)tests/Halcyon/HalcyonModelTest.php(2 hunks)tests/Halcyon/MemoryRepositoryTest.php(1 hunks)tests/Halcyon/SectionParserTest.php(1 hunks)tests/Halcyon/ValidationTraitTest.php(2 hunks)tests/Html/BlockBuilderTest.php(1 hunks)tests/Html/FormBuilderTest.php(12 hunks)tests/Html/HtmlBuilderTest.php(1 hunks)tests/Html/HtmlHelperTest.php(1 hunks)tests/Mail/MailerTest.php(1 hunks)tests/Network/HttpTest.php(1 hunks)tests/Parse/ArrayFileTest.php(1 hunks)tests/Parse/BracketTest.php(1 hunks)tests/Parse/EnvFileTest.php(1 hunks)tests/Parse/IniTest.php(1 hunks)tests/Parse/MarkdownTest.php(4 hunks)tests/Parse/SyntaxFieldParserTest.php(1 hunks)tests/Parse/SyntaxParserTest.php(1 hunks)tests/Parse/YamlTest.php(1 hunks)tests/Router/RouterHelperTest.php(1 hunks)tests/Router/RouterTest.php(1 hunks)tests/Router/RoutingUrlGeneratorTest.php(2 hunks)tests/Router/UrlGeneratorTest.php(1 hunks)tests/Scaffold/ScaffoldBaseTest.php(1 hunks)tests/Support/ArrTest.php(1 hunks)tests/Support/ClassLoaderTest.php(1 hunks)tests/Support/CountableTest.php(1 hunks)tests/Support/EmitterTest.php(2 hunks)tests/Support/EventFakeTest.php(1 hunks)tests/Support/ExtensionAndEmitterSerializationTest.php(1 hunks)tests/Support/HelpersTest.php(1 hunks)tests/Support/MailFakeTest.php(1 hunks)tests/Support/StrTest.php(1 hunks)tests/Support/SvgTest.php(1 hunks)tests/Translation/FileLoaderTest.php(2 hunks)tests/Translation/TranslatorTest.php(4 hunks)tests/Validation/EmailValidationTest.php(1 hunks)tests/Validation/RuleObjectTest.php(1 hunks)tests/bootstrap.php(0 hunks)tests/fixtures/events/EventTest.php(1 hunks)tests/stubs/Facades.stub(0 hunks)
💤 Files with no reviewable changes (4)
- tests/bootstrap.php
- .github/workflows/code-quality.yaml
- tests/stubs/Facades.stub
- src/Database/Connectors/SQLiteConnector.php
🧰 Additional context used
🪛 PHPMD (2.15.0)
src/Database/PDO/SqlServerDriver.php
18-18: Avoid unused parameters such as '$username'. (undefined)
(UnusedFormalParameter)
18-18: Avoid unused parameters such as '$password'. (undefined)
(UnusedFormalParameter)
18-18: Avoid unused parameters such as '$driverOptions'. (undefined)
(UnusedFormalParameter)
src/Database/Schema/Grammars/PostgresGrammar.php
25-25: Avoid unused parameters such as '$command'. (undefined)
(UnusedFormalParameter)
src/Database/Schema/Grammars/Concerns/MySqlBasedGrammar.php
24-24: Avoid unused parameters such as '$command'. (undefined)
(UnusedFormalParameter)
src/Translation/FileLoader.php
22-22: Avoid unused parameters such as '$output'. (undefined)
(UnusedFormalParameter)
src/Console/Traits/HandlesCleanup.php
46-46: Avoid unused parameters such as '$previousExitCode'. (undefined)
(UnusedFormalParameter)
src/Database/PDO/Concerns/ConnectsToDatabase.php
22-22: Avoid unused parameters such as '$username'. (undefined)
(UnusedFormalParameter)
22-22: Avoid unused parameters such as '$password'. (undefined)
(UnusedFormalParameter)
22-22: Avoid unused parameters such as '$driverOptions'. (undefined)
(UnusedFormalParameter)
src/Database/Schema/Grammars/SQLiteGrammar.php
28-28: Avoid unused parameters such as '$command'. (undefined)
(UnusedFormalParameter)
166-166: Avoid unused parameters such as '$column'. (undefined)
(UnusedFormalParameter)
| "phpunit/phpunit": "^11.0", | ||
| "squizlabs/php_codesniffer": "^3.2", | ||
| "php-parallel-lint/php-parallel-lint": "^1.0", | ||
| "meyfa/phpunit-assert-gd": "^3.0.0", | ||
| "dms/phpunit-arraysubset-asserts": "^0.5.0", | ||
| "larastan/larastan": "^2.8.1", | ||
| "orchestra/testbench": "^7.47.0" | ||
| "meyfa/phpunit-assert-gd": "^4.2", | ||
| "mockery/mockery": "^1.6|^2.0", | ||
| "dms/phpunit-arraysubset-asserts": "dev-add-phpunit-11-support", | ||
| "orchestra/testbench": "^10.0", | ||
| "larastan/larastan": "^3.6.0" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify dev-branch PHPUnit dependency stability.
The upgrade to PHPUnit ^11.0 is appropriate, but there's a critical stability concern:
Line 60 uses dms/phpunit-arraysubset-asserts with dev-add-phpunit-11-support from a fork (lines 64-69). This dev branch is not a stable, tagged release and could break without notice.
Verify whether this fork has been merged upstream or if a stable release is available:
🌐 Web query:
Is there a stable release of dms/phpunit-arraysubset-asserts that supports PHPUnit 11?
💡 Result:
No — there is no stable release that declares support for PHPUnit 11. The latest stable release is v0.5.0 (Jun 2023) and its composer constraint allows up to PHPUnit ^10.0. [1][2]
Sources:
- Packagist: dms/phpunit-arraysubset-asserts (v0.5.0 requires phpunit ^4.8 … ^10.0). [1]
- GitHub repository releases (latest v0.5.0, Jun 7, 2023). [2]
Pin or replace the dev-branch dependency in composer.json. There’s no stable dms/phpunit-arraysubset-asserts release supporting PHPUnit 11, so lock the “dev-add-phpunit-11-support” branch to a specific commit or vendor a tagged fork to avoid unexpected breaks.
🤖 Prompt for AI Agents
In composer.json around lines 55 to 62, the dev-branch dependency
"dms/phpunit-arraysubset-asserts": "dev-add-phpunit-11-support" must be pinned
to avoid breakage; replace the floating dev branch with a specific commit
reference (e.g. "dev-add-phpunit-11-support#<commit-hash>") or switch to an
alternative stable fork/tag and update composer.lock, or vendor the patched code
into the repo and point composer to the local path; after changing the
requirement run composer update for that package and commit composer.json and
composer.lock.
| */ | ||
| public function attemptResetPassword($resetCode, $newPassword) | ||
| { | ||
| if ($this->checkResetPasswordCode($resetCode)) { |
There was a problem hiding this comment.
🧩 Analysis chain
Verify consistency with setPasswordAttribute().
The refactoring correctly updates checkPassword(), attemptResetPassword(), and getAuthPassword() to use the configurable password attribute name. However, setPasswordAttribute() (lines 318-329) remains hardcoded to the 'password' attribute:
public function setPasswordAttribute($value)
{
if ($this->exists && empty($value)) {
unset($this->attributes['password']); // Still hardcoded
}
else {
$this->attributes['password'] = $value; // Still hardcoded
$this->attributes['persist_code'] = null;
}
}If a subclass changes $authPasswordName to a different column (e.g., 'user_password'), the getter would read from 'user_password' but the setter would still write to 'password', creating an inconsistency.
Run the following script to check if there are any other password-related methods that might need similar updates:
Also applies to: 295-295, 626-626
🏁 Script executed:
#!/bin/bash
# Description: Find methods that directly access the password attribute to verify consistency
# Search for direct password attribute access patterns
rg -n --type=php -C 3 '\$this->password(?!\w)|\$this->attributes\[.password.\]' src/Auth/Models/User.phpLength of output: 274
🏁 Script executed:
#!/bin/bash
# Locate the setter and hardcoded password attribute references
rg -n --type=php 'function setPasswordAttribute' -C5 src/Auth/Models/User.php
rg -n --type=php "\$this->attributes\['password'\]" -C3 src/Auth/Models/User.phpLength of output: 297
🏁 Script executed:
#!/bin/bash
# Inspect getAuthPassword implementation for dynamic attribute usage
rg -n --type=php 'function getAuthPassword' -C10 src/Auth/Models/User.phpLength of output: 757
Update setPasswordAttribute() to use dynamic password attribute
In src/Auth/Models/User.php (lines 318–329), replace hardcoded ['password'] with the value of $this->getAuthPasswordName() for both unset and assignment so the setter matches the configurable getter.
🤖 Prompt for AI Agents
In src/Auth/Models/User.php around lines 318–329, the setPasswordAttribute
setter currently uses the hardcoded ['password'] key; change it to use a single
variable $passwordField = $this->getAuthPasswordName() and use that for both the
unset and the assignment (e.g. unset($this->$passwordField);
$this->$passwordField = Hash::make($value);), ensuring the setter uses the same
dynamic password attribute as getAuthPasswordName().
| public function bindValues($statement, $bindings) | ||
| { | ||
| foreach ($bindings as $key => $value) { | ||
| $statement->bindValue( | ||
| is_string($key) ? $key : $key + 1, | ||
| $value, | ||
| is_int($value) || is_float($value) ? PDO::PARAM_INT : PDO::PARAM_STR | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: floats bound as integers (loss of precision) and null/bool not handled
Binding floats with PDO::PARAM_INT truncates decimals. Also explicitly handle null and booleans.
public function bindValues($statement, $bindings)
{
foreach ($bindings as $key => $value) {
$statement->bindValue(
is_string($key) ? $key : $key + 1,
$value,
- is_int($value) || is_float($value) ? PDO::PARAM_INT : PDO::PARAM_STR
+ is_int($value) ? PDO::PARAM_INT
+ : (is_bool($value) ? PDO::PARAM_BOOL
+ : (is_null($value) ? PDO::PARAM_NULL
+ : PDO::PARAM_STR))
);
}
}📝 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.
| public function bindValues($statement, $bindings) | |
| { | |
| foreach ($bindings as $key => $value) { | |
| $statement->bindValue( | |
| is_string($key) ? $key : $key + 1, | |
| $value, | |
| is_int($value) || is_float($value) ? PDO::PARAM_INT : PDO::PARAM_STR | |
| ); | |
| } | |
| } | |
| public function bindValues($statement, $bindings) | |
| { | |
| foreach ($bindings as $key => $value) { | |
| $statement->bindValue( | |
| is_string($key) ? $key : $key + 1, | |
| $value, | |
| is_int($value) ? PDO::PARAM_INT | |
| : (is_bool($value) ? PDO::PARAM_BOOL | |
| : (is_null($value) ? PDO::PARAM_NULL | |
| : PDO::PARAM_STR)) | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/Database/Connections/MariaDbConnection.php around lines 70 to 79, the
current bindValues treats floats as PDO::PARAM_INT (losing decimals) and doesn't
handle nulls or booleans; change the type selection so integers use
PDO::PARAM_INT, booleans use PDO::PARAM_BOOL, nulls use PDO::PARAM_NULL, and
everything else (including floats) uses PDO::PARAM_STR to preserve precision;
keep existing key handling (is_string($key) ? $key : $key + 1) and apply the new
conditional when calling $statement->bindValue.
| */ | ||
| class Model extends EloquentModel implements ModelInterface | ||
| { | ||
| use Concerns\HasAttributes; |
There was a problem hiding this comment.
🧩 Analysis chain
Confirm HasAttributes provides required methods used below
getAttribute() (Line 1070) calls $this->hasAttribute($key), and the model relies on getDates(). Ensure Concerns\HasAttributes defines these methods after their removal from Model.
🏁 Script executed:
#!/bin/bash
# Confirm methods exist in the trait
fd -a 'HasAttributes.php'
rg -nP -C2 'trait\s+HasAttributes\b|function\s+hasAttribute\s*\(|function\s+getDates\s*\(' $(fd -a 'HasAttributes.php')Length of output: 266
Define missing hasAttribute in HasAttributes trait
getDates() is present, but hasAttribute() is not defined—calls to getAttribute() will break. Add hasAttribute() (or import it) into src/Database/Concerns/HasAttributes.php.
🤖 Prompt for AI Agents
In src/Database/Model.php around line 25, the Model imports
Concerns\HasAttributes but the HasAttributes trait in
src/Database/Concerns/HasAttributes.php is missing the hasAttribute() method
which breaks calls from getAttribute(); add a public hasAttribute(string $key):
bool method to that trait (or import the correct trait providing it) that
returns whether the given attribute/key exists (checking the internal attributes
array or attribute storage used by the trait), ensure the method signature
matches existing getAttribute usage, and run tests to verify no call sites
break.
| return collect($this->paths) | ||
| ->reduce(function ($output, $path) use ($lines, $locale, $group, $namespace) { | ||
| $winterNamespace = str_replace('.', '/', $namespace); | ||
|
|
||
| $file = "{$this->path}/{$locale}/{$namespace}/{$group}.php"; | ||
| // Look for a Winter-managed namespace | ||
| $file = "{$path}/{$locale}/{$winterNamespace}/{$group}.php"; | ||
| if ($this->files->exists($file)) { | ||
| return array_replace_recursive($lines, $this->files->getRequire($file)); | ||
| } | ||
|
|
||
| if ($this->files->exists($file)) { | ||
| return array_replace_recursive($lines, $this->files->getRequire($file)); | ||
| } | ||
| // Look for a Winter-managed namespace with a Winter-formatted locale (xx-xx instead of xx_XX) | ||
| $dashLocale = str_replace('_', '-', strtolower($locale)); | ||
| $dashFile = "{$path}/{$dashLocale}/{$winterNamespace}/{$group}.php"; | ||
| if ($dashFile !== $file && $this->files->exists($dashFile)) { | ||
| return array_replace_recursive($lines, $this->files->getRequire($dashFile)); | ||
| } | ||
|
|
||
| // Try "xx-xx" format | ||
| $locale = str_replace('_', '-', strtolower($locale)); | ||
| // Look for a vendor-managed namespace | ||
| $file = "{$path}/vendor/{$namespace}/{$locale}/{$group}.php"; | ||
| if ($this->files->exists($file)) { | ||
| return array_replace_recursive($lines, $this->files->getRequire($file)); | ||
| } | ||
|
|
||
| if ("{$this->path}/{$locale}/{$namespace}/{$group}.php" !== $file) { | ||
| $file = "{$this->path}/{$locale}/{$namespace}/{$group}.php"; | ||
|
|
||
| if ($this->files->exists($file)) { | ||
| return array_replace_recursive($lines, $this->files->getRequire($file)); | ||
| } | ||
| } | ||
|
|
||
| return $lines; | ||
| return $lines; | ||
| }, []); |
There was a problem hiding this comment.
Fix accumulator handling in loadNamespaceOverrides.
The reducer ignores $output, so any later path that lacks the file resets to the original $lines, wiping out overrides loaded from earlier paths. In multi-path setups (common in Winter), overrides effectively never apply if subsequent paths don't hit, which is a regression from the previous single-path logic. Please carry the accumulator forward (initialised with $lines) when merging each match. Example fix:
- return collect($this->paths)
- ->reduce(function ($output, $path) use ($lines, $locale, $group, $namespace) {
+ return collect($this->paths)
+ ->reduce(function ($output, $path) use ($locale, $group, $namespace) {
$winterNamespace = str_replace('.', '/', $namespace);
// Look for a Winter-managed namespace
$file = "{$path}/{$locale}/{$winterNamespace}/{$group}.php";
if ($this->files->exists($file)) {
- return array_replace_recursive($lines, $this->files->getRequire($file));
+ $output = array_replace_recursive($output, $this->files->getRequire($file));
}
// Look for a Winter-managed namespace with a Winter-formatted locale (xx-xx instead of xx_XX)
$dashLocale = str_replace('_', '-', strtolower($locale));
$dashFile = "{$path}/{$dashLocale}/{$winterNamespace}/{$group}.php";
if ($dashFile !== $file && $this->files->exists($dashFile)) {
- return array_replace_recursive($lines, $this->files->getRequire($dashFile));
+ $output = array_replace_recursive($output, $this->files->getRequire($dashFile));
}
// Look for a vendor-managed namespace
$file = "{$path}/vendor/{$namespace}/{$locale}/{$group}.php";
if ($this->files->exists($file)) {
- return array_replace_recursive($lines, $this->files->getRequire($file));
+ $output = array_replace_recursive($output, $this->files->getRequire($file));
}
- return $lines;
- }, []);
+ return $output;
+ }, $lines);🧰 Tools
🪛 PHPMD (2.15.0)
22-22: Avoid unused parameters such as '$output'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In src/Translation/FileLoader.php around lines 21 to 45, the reducer is
initialized with [] and always returns $lines when no file is found, which
discards any previously accumulated overrides; change the reducer to initialize
with $lines and when merging a found file use array_replace_recursive($output,
$this->files->getRequire($file)) (and likewise for the dash-locale and vendor
checks) so the accumulator ($output) is carried forward instead of being reset.
| $dashLocale = str_replace('_', '-', strtolower($locale)); | ||
| $dashFile = "{$path}/{$dashLocale}/{$group}.php"; | ||
| if ($dashFile !== $file && $this->files->exists($dashFile)) { | ||
| return $this->files->getRequire($dashFile); | ||
| } |
There was a problem hiding this comment.
Preserve accumulated translations for dash-formatted locales.
In loadPaths, the dash-locale fallback currently returns the raw file contents, discarding anything merged into $output from earlier paths. Use array_replace_recursive($output, ...) here as well so previously loaded segments aren’t lost.
🤖 Prompt for AI Agents
In src/Translation/FileLoader.php around lines 69 to 73, the dash-locale
fallback currently returns the file contents directly and discards any
translations already merged into $output; change the return to merge the
dash-file array into $output using array_replace_recursive($output,
<dash-file-array>) so previously loaded segments are preserved, i.e. retrieve
the dash file contents and return array_replace_recursive($output, thatContents)
instead of returning the raw file contents.
| @@ -0,0 +1,64 @@ | |||
| <?php | |||
|
|
|||
| namespace Tests\Database\Schema\Grammars; | |||
There was a problem hiding this comment.
Fix incorrect namespace declaration.
The namespace Tests\Database\Schema\Grammars is missing the Winter\Storm\ prefix, causing an inconsistency with other test classes in this PR (e.g., SqlServerSchemaGrammarTest uses Winter\Storm\Tests\Database\Schema\Grammars). This will cause autoloading and test discovery failures.
Apply this diff:
-namespace Tests\Database\Schema\Grammars;
+namespace Winter\Storm\Tests\Database\Schema\Grammars;📝 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.
| namespace Tests\Database\Schema\Grammars; | |
| namespace Winter\Storm\Tests\Database\Schema\Grammars; |
🤖 Prompt for AI Agents
In tests/Database/Schema/Grammars/MySqlSchemaGrammarTest.php around line 3, the
namespace declaration currently is "namespace Tests\Database\Schema\Grammars;"
but should include the Winter\Storm\ prefix to match other tests; update the
namespace to "Winter\Storm\Tests\Database\Schema\Grammars" (adjust the line to
include the full prefix) so autoloading and test discovery align with the rest
of the test suite.
| $this->assertSame('alter column "name" null', $parts[1]); | ||
| $this->assertSame("alter column \"name\" set default 'admin'", $parts[2]); |
There was a problem hiding this comment.
Invalid Postgres SQL in expectation ('null' is not a valid clause).
Postgres uses “drop not null” / “set not null”, not “… null”. The expected SQL should be “drop not null”. Also removes the extra space.
Apply this diff:
- $this->assertSame('alter column "name" null', $parts[1]);
+ $this->assertSame('alter column "name" drop not null', $parts[1]);🤖 Prompt for AI Agents
In tests/Database/Schema/Grammars/PostgresSchemaGrammarTest.php around lines 51
to 52, the first expectation uses invalid Postgres SQL ("alter column \"name\"
null"): replace it with the correct clause "alter column \"name\" drop not null"
(remove the extra space and use "drop not null"); leave the second expectation
unchanged.
| public function getConnection() | ||
| { | ||
| return ($this->connection)(); | ||
| } |
There was a problem hiding this comment.
Fix incorrect mock invocation syntax.
Line 39 attempts to invoke the mock as a callable with ($this->connection)(), but Mockery mocks are not directly callable. This will cause a runtime error.
Apply this diff:
public function getConnection()
{
- return ($this->connection)();
+ return $this->connection;
}📝 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.
| public function getConnection() | |
| { | |
| return ($this->connection)(); | |
| } | |
| public function getConnection() | |
| { | |
| return $this->connection; | |
| } |
🤖 Prompt for AI Agents
In tests/GrammarTestCase.php around lines 37 to 40, the getter incorrectly
attempts to invoke the mock with ($this->connection)(), which will error because
Mockery mocks are not callable; change the method to simply return the mock
object (return $this->connection;) or call the appropriate mock method if a
specific value is required, ensuring you do not use function-call syntax on the
mock.
Realised that this would be too much a BC break given that these methods are intended to be extended upon.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Halcyon/Builder.php (1)
161-175: Add null-safety check tolists()method at line 172.The review comment is correct. With
getFresh()returningarray|null(line 282), passing the result directly to theCollectionconstructor without a null check creates a potential TypeError. Theget()method properly handles this with$results ?: [](line 331), butlists()lacks this protection. Apply the suggested defensive pattern:- $collection = new Collection($results); + $collection = new Collection($results ?: []);This ensures consistency with the
get()method and prevents errors whengetFresh()returns null.src/Database/Connections/MySqlConnection.php (1)
70-79: Bug: floats bound as integers (loss of precision) and null/bool not handled.Binding floats with
PDO::PARAM_INTtruncates decimals. Also, null and boolean values should be explicitly handled with their respective PDO parameter types.Apply this diff:
public function bindValues($statement, $bindings) { foreach ($bindings as $key => $value) { $statement->bindValue( is_string($key) ? $key : $key + 1, $value, - is_int($value) || is_float($value) ? PDO::PARAM_INT : PDO::PARAM_STR + is_int($value) ? PDO::PARAM_INT + : (is_bool($value) ? PDO::PARAM_BOOL + : (is_null($value) ? PDO::PARAM_NULL + : PDO::PARAM_STR)) ); } }
♻️ Duplicate comments (22)
src/Foundation/Http/Middleware/CheckForMaintenanceMode.php (1)
26-45: Re-throw non-503 HttpExceptions to preserve error handling.Non-503 HttpExceptions are caught but not re-thrown, causing them to be silently swallowed and the method to return null. This breaks proper HTTP error handling for status codes like 404, 401, 403, etc.
Apply this diff to re-throw non-503 exceptions:
} + + throw $ex; } } }src/Halcyon/MemoryCacheManager.php (1)
10-12: Verify that MemoryRepository constructor was added to prevent fatal error.A past review flagged that
MemoryRepositorylacks a constructor accepting the$configparameter. The relevant code snippet confirms thatMemoryRepositorystill has no__constructmethod defined. Without a proper constructor, callingnew MemoryRepository($store, $config)will likely cause anArgumentCountErrorif the parentRepositoryconstructor only accepts one parameter.Run the following script to verify whether:
MemoryRepositorynow has a constructor accepting$config- The parent
Repositoryclass constructor signature supports this change#!/bin/bash # Verify MemoryRepository and parent Repository constructor signatures echo "=== Checking MemoryRepository constructor ===" rg -n -A 10 "class MemoryRepository" src/Halcyon/MemoryRepository.php | rg -A 8 "__construct" echo -e "\n=== Checking parent Repository class constructor ===" # Repository is from Illuminate\Cache, check if Storm has a custom one or uses Laravel's fd -t f "Repository.php" | xargs rg -l "namespace.*Cache" | head -5 | while read file; do echo "File: $file" rg -n -A 5 "class Repository" "$file" | rg -B 1 -A 5 "__construct" donesrc/Network/Http.php (1)
277-277: Technical improvement, but security concern persists.Changing
falseto0is technically more correct, asCURLOPT_SSL_VERIFYHOSTexpects an integer value (0, 1, or 2). However, the underlying security issue flagged in the previous review remains: SSL host verification is still disabled by default, leaving all HTTP requests vulnerable to MITM attacks unlessverifySSL()is explicitly called.composer.json (1)
60-69: Pin the dev-branch dependency to a specific commit.The
dms/phpunit-arraysubset-assertspackage uses an unstable dev branch that could break without notice. Since no stable release supports PHPUnit 11, pin the dependency to a specific commit hash.Apply this pattern:
- "dms/phpunit-arraysubset-asserts": "dev-add-phpunit-11-support", + "dms/phpunit-arraysubset-asserts": "dev-add-phpunit-11-support#abc1234",Replace
abc1234with the actual commit hash from the fork.src/Auth/Models/User.php (1)
316-325: Align setter with dynamic password column.If
$authPasswordNameis overridden, this mutator still touches the hard-codedpasswordkey, so resets and assignments leave the configured column untouched. Please drive the unset/assignment throughgetAuthPasswordName()to keep the model consistent with the new configurability.- if ($this->exists && empty($value)) { - unset($this->attributes['password']); - } else { - $this->attributes['password'] = $value; + $passwordField = $this->getAuthPasswordName(); + + if ($this->exists && empty($value)) { + unset($this->attributes[$passwordField]); + } else { + $this->attributes[$passwordField] = $value;src/Database/Relations/Concerns/DeferOneOrMany.php (1)
127-133: Don't coerce the deferred key expression to string.Switching to
->getValue()forces the builder to treat theCAST(...)fragment as an identifier, so platforms like MySQL wrap it in backticks and break the deferred BelongsToMany / MorphToMany queries. Returning the rawExpressionpreserves the original behaviour.- protected function getWithDeferredQualifiedKeyName(): string - { - return $this->parent->getConnection()->raw(DbDongle::cast( - DbDongle::getTablePrefix() . $this->related->getQualifiedKeyName(), - 'TEXT' - ))->getValue($this->parent->getGrammar()); - } + protected function getWithDeferredQualifiedKeyName(): \Illuminate\Database\Query\Expression + { + return $this->parent->getConnection()->raw(DbDongle::cast( + DbDongle::getTablePrefix() . $this->related->getQualifiedKeyName(), + 'TEXT' + )); + }src/Translation/FileLoader.php (2)
21-45: Carry the reducer accumulator forward inloadNamespaceOverrides.Returning
$lineswhenever a path misses wipes out overrides gathered from earlier paths, so multi-path namespace overrides silently fail. Seed the reducer with$linesand merge into$outputeach iteration instead of resetting it.- return collect($this->paths) - ->reduce(function ($output, $path) use ($lines, $locale, $group, $namespace) { + return collect($this->paths) + ->reduce(function ($output, $path) use ($locale, $group, $namespace) { $winterNamespace = str_replace('.', '/', $namespace); @@ - return $lines; - }, []); + return $output; + }, $lines);
69-73: Merge dash-locale overrides instead of replacing the accumulator.The dash-locale fallback currently returns the raw file contents, throwing away everything accumulated from prior paths. Merge the dash-locale array into
$outputso previously loaded segments survive.- if ($dashFile !== $file && $this->files->exists($dashFile)) { - return $this->files->getRequire($dashFile); + if ($dashFile !== $file && $this->files->exists($dashFile)) { + return array_replace_recursive($output, $this->files->getRequire($dashFile)); }src/Database/PDO/Concerns/ConnectsToDatabase.php (1)
24-26: Fix the error message to reference Winter instead of Laravel.The exception message incorrectly mentions "Laravel" but this is Winter CMS.
Apply this diff:
- if (! isset($params['pdo']) || ! $params['pdo'] instanceof PDO) { - throw new InvalidArgumentException('Laravel requires the "pdo" property to be set and be a PDO instance.'); - } + if (! isset($params['pdo']) || ! $params['pdo'] instanceof PDO) { + throw new InvalidArgumentException('Winter requires the "pdo" property to be set and be a PDO instance.'); + }src/Database/Model.php (1)
25-25: Confirm HasAttributes trait provides required methods.The Model now uses
Concerns\HasAttributestrait, but past review comments indicate thathasAttribute()method is missing from the trait. This method is called ingetAttribute()at Line 1070 and will cause a fatal error if not defined.tests/GrammarTestCase.php (1)
37-40: Fix incorrect mock invocation syntax.Line 39 attempts to invoke the mock as a callable with
($this->connection)(), but Mockery mocks are not directly callable. This will cause a runtime error whengetConnection()is called.Apply this diff:
public function getConnection() { - return ($this->connection)(); + return $this->connection; }src/Database/Schema/Grammars/PostgresGrammar.php (1)
63-70: Don't strip all single quotes from defaults; only trim surrounding quotes.The
preg_replace('#\'#', '', $value)call removes every single quote inside the string, which will corrupt values like "O'Connor" → "OConnor". Only remove wrapping quotes to avoid corrupting literals.Apply this diff:
public function getDefaultValue($value) { - if (is_string($value)) { - $value = preg_replace('#\'#', '', $value); - } - - return parent::getDefaultValue($value); + if (is_string($value) && strlen($value) >= 2 && $value[0] === "'" && substr($value, -1) === "'") { + // Trim only surrounding single quotes to avoid double-quoting while preserving inner apostrophes + $value = substr($value, 1, -1); + } + return parent::getDefaultValue($value); }src/Database/Schema/Grammars/Concerns/MySqlBasedGrammar.php (2)
36-36: Bug: Passing ColumnDefinition to wrap() will cause failure.The
wrap()method expects a string identifier, not aColumnDefinitionobject. This will produce invalid SQL or fatal errors.Apply this fix:
$sql = sprintf( '%s %s%s %s', is_null($column->renameTo) ? 'modify' : 'change', - $this->wrap($column), + $this->wrap($column->name), is_null($column->renameTo) ? '' : ' '.$this->wrap($column->renameTo), $this->getType($column) );
59-66: Bug: Regex strips all quotes, corrupting values like "O'Reilly".The current
preg_replace('#\'#', '', $value)removes every single quote, including those inside the string. This corrupts default values containing apostrophes.Apply this fix to only strip wrapping quotes:
public function getDefaultValue($value) { - if (is_string($value)) { - $value = preg_replace('#\'#', '', $value); + if (is_string($value) && strlen($value) >= 2 && $value[0] === "'" && substr($value, -1) === "'") { + $value = substr($value, 1, -1); } return parent::getDefaultValue($value); }tests/Database/Schema/Grammars/PostgresSchemaGrammarTest.php (1)
51-51: Bug: Invalid Postgres SQL syntax in test expectation.Postgres does not support
... nullas a clause. The correct syntax isdrop not nullto make a column nullable. The expectation also has an extra space.Apply this fix:
- $this->assertSame('alter column "name" null', $parts[1]); + $this->assertSame('alter column "name" drop not null', $parts[1]);tests/Database/Schema/Grammars/MySqlSchemaGrammarTest.php (1)
3-3: Fix incorrect namespace declaration.The namespace is missing the
Winter\Storm\prefix, which will cause autoloading and test discovery failures. This is inconsistent with other test classes in the codebase.Apply this diff:
-namespace Tests\Database\Schema\Grammars; +namespace Winter\Storm\Tests\Database\Schema\Grammars;src/Database/Schema/Grammars/SQLiteGrammar.php (1)
103-105: Bug: str_starts_with arguments reversed; sqlite_ indexes not filtered.The arguments to
str_starts_withare reversed, so internal sqlite_* indexes will never be filtered out. This can cause errors when recreating indexes.Apply this diff:
- $indexes = collect($indexes)->reject(fn ($index) => str_starts_with('sqlite_', $index->index))->map( + $indexes = collect($indexes)->reject(fn ($index) => str_starts_with($index->index, 'sqlite_'))->map( fn ($index) => $this->{'compile'.ucfirst($index->name)}($blueprint, $index) )->all();src/Database/PDO/Connection.php (4)
42-53: Don't rely on assert; handle PDO::exec() false explicitly.
PDO::exec()can returnfalsewithout throwing an exception. The current code usesassert()which is insufficient, and returningfalseviolates theintreturn type.Apply this diff:
public function exec(string $statement): int { try { $result = $this->connection->exec($statement); - - \assert($result !== false); - - return $result; + if ($result === false) { + $errorInfo = $this->connection->errorInfo(); + throw Exception::new(new PDOException($errorInfo[2] ?? 'PDO::exec() failed', 0)); + } + return (int) $result; } catch (PDOException $exception) { throw Exception::new($exception); } }
61-70: Guard against PDO::prepare() returning false.
PDO::prepare()returnsfalseon failure without throwing an exception. The current code can trigger a TypeError increateStatement().Apply this diff:
public function prepare(string $sql): StatementInterface { try { - return $this->createStatement( - $this->connection->prepare($sql) - ); + $pdoStmt = $this->connection->prepare($sql); + if ($pdoStmt === false) { + $errorInfo = $this->connection->errorInfo(); + throw Exception::new(new PDOException($errorInfo[2] ?? 'PDO::prepare() failed', 0)); + } + return $this->createStatement($pdoStmt); } catch (PDOException $exception) { throw Exception::new($exception); } }
78-89: Guard against PDO::query() returning false.
PDO::query()can returnfalseon failure. Avoid relying onassert()for production error handling.Apply this diff:
public function query(string $sql): ResultInterface { try { $stmt = $this->connection->query($sql); - - \assert($stmt instanceof PDOStatement); - - return new Result($stmt); + if ($stmt === false) { + $errorInfo = $this->connection->errorInfo(); + throw Exception::new(new PDOException($errorInfo[2] ?? 'PDO::query() failed', 0)); + } + return new Result($stmt); } catch (PDOException $exception) { throw Exception::new($exception); } }
158-161: quote() may return false; enforce string return.
PDO::quote()returnsstring|false. Returningfalseviolates the declared return type.Apply this diff:
public function quote($input, $type = ParameterType::STRING) { - return $this->connection->quote($input, $type); + $quoted = $this->connection->quote($input, $type); + if ($quoted === false) { + $errorInfo = $this->connection->errorInfo(); + throw Exception::new(new PDOException($errorInfo[2] ?? 'PDO::quote() failed', 0)); + } + return $quoted; }src/Database/Connections/MariaDbConnection.php (1)
70-79: Bug: floats bound as integers (loss of precision) and null/bool not handled.Binding floats with
PDO::PARAM_INTtruncates decimals. Null and boolean values should also be explicitly handled with their respective PDO parameter types.Apply this diff:
public function bindValues($statement, $bindings) { foreach ($bindings as $key => $value) { $statement->bindValue( is_string($key) ? $key : $key + 1, $value, - is_int($value) || is_float($value) ? PDO::PARAM_INT : PDO::PARAM_STR + is_int($value) ? PDO::PARAM_INT + : (is_bool($value) ? PDO::PARAM_BOOL + : (is_null($value) ? PDO::PARAM_NULL + : PDO::PARAM_STR)) ); } }
🧹 Nitpick comments (14)
tests/Scaffold/ScaffoldBaseTest.php (1)
23-29: Consider removing duplicate helper method.The
callProtectedMethodhelper is already defined in the parent class\Winter\Storm\Tests\TestCase(seetests/TestCase.phplines 26-34) with an identical implementation. You can safely remove this duplicate and rely on the inherited method.Apply this diff to remove the duplication:
- protected static function callProtectedMethod($object, $name, $params = []) - { - $className = get_class($object); - $class = new ReflectionClass($className); - $method = $class->getMethod($name); - $method->setAccessible(true); - return $method->invokeArgs($object, $params); - } -src/Support/Facades/File.php (1)
47-47: Consider using unqualified class name for consistency.Since the
FilesystemAdapterimport is now present (line 4), you could simplify the fully qualified reference here to match the style used in lines 57-58.Apply this diff for consistency:
- * @method static bool isLocalDisk(\Illuminate\Filesystem\FilesystemAdapter $disk) + * @method static bool isLocalDisk(FilesystemAdapter $disk)tests/Parse/YamlTest.php (1)
8-8: LGTM! Change correctly aligns with test-suite standardization.The update to extend
\Winter\Storm\Tests\TestCaseis correct and consistent with the broader test-suite consolidation described in the PR objectives.For improved readability and consistency with modern PHP conventions, consider adding a
usestatement at the top of the file instead of using the fully-qualified namespace in the class declaration:use Winter\Storm\Parse\Processor\YamlProcessor; use Symfony\Component\Yaml\Exception\ParseException; use Winter\Storm\Parse\Processor\Symfony3Processor; use Winter\Storm\Parse\Yaml as YamlParser; +use Winter\Storm\Tests\TestCase; -class YamlTest extends \Winter\Storm\Tests\TestCase +class YamlTest extends TestCasetests/Database/UpdaterTest.php (1)
7-7: Consider removing the nullable type since setUp() always initializes the property.The nullable type and null default appear defensive but are functionally unnecessary since PHPUnit guarantees
setUp()runs before each test method. The property is never actually null during test execution.Consider simplifying to a non-nullable property without a default value:
- protected ?Updater $updater = null; + protected Updater $updater;This relies on
setUp()to initialize the property (which it always does on line 13) and better expresses the invariant that the property is always initialized when tests run.tests/Foundation/ProviderRepositoryTest.php (1)
49-49: Optional: Fix method name typos.The method names
testOriginalFunctionaliyandtestWinterFunctionaliycontain typos (should be "Functionality"). While not new to this PR, consider correcting these for better code clarity.Also applies to: 67-67
tests/Support/CountableTest.php (1)
2-2: LGTM! Explicit base class reference improves clarity.The fully-qualified reference to
\Winter\Storm\Tests\TestCasemakes the inheritance chain explicit and aligns with the PR-wide standardization of test infrastructure.Optional: Consider adding a namespace declaration.
The file lacks a namespace declaration. Adding one (e.g.,
namespace Winter\Storm\Tests\Support;) would follow modern PHP best practices and better organize the test suite, though this may be intentional for the current test structure.src/Auth/Models/Preferences.php (2)
63-63: Modern PHP syntax: explicit constructor call.Using
new static()with explicit parentheses aligns with modern PHP style guidelines and improves consistency.
116-117: Modern PHP syntax: short array destructuring.The migration from
list(...)to[...]syntax adopts PHP 7.1+ short array destructuring, improving code readability and consistency with modern PHP practices.Also applies to: 177-177
src/Database/Traits/Revisionable.php (1)
160-161: Consider removing the redundant$this->datescheck.The condition checks
in_array($attribute, $this->getDates())followed by a direct check ofin_array($attribute, $this->dates). SincegetDates()already merges$this->dateswith timestamp columns (as seen in HasAttributes.php lines 20-32), the final check appears redundant.If the redundant check is for backward compatibility, consider documenting why it's necessary. Otherwise, simplify to:
- if (in_array($attribute, $this->getDates()) || $this->isDateCastable($attribute) || in_array($attribute, $this->dates)) { + if (in_array($attribute, $this->getDates()) || $this->isDateCastable($attribute)) { return 'date'; }tests/Database/Relations/DynamicRelationTest.php (1)
11-11: LGTM: Test inheritance standardized.The fully qualified base class name standardizes test inheritance across the suite. The import on line 9 makes this redundant, but the explicit qualification doesn't hurt.
Optionally, you could remove the import on line 9 or use the imported alias:
-class DynamicRelationTest extends \Winter\Storm\Tests\DbTestCase +class DynamicRelationTest extends DbTestCasetests/Database/Relations/AttachManyTest.php (1)
10-10: LGTM: Test inheritance standardized.The fully qualified base class name aligns with the standardization effort across test files. As with DynamicRelationTest, the import on line 8 makes this redundant but not incorrect.
Optionally simplify by using the imported alias:
-class AttachManyTest extends \Winter\Storm\Tests\DbTestCase +class AttachManyTest extends DbTestCasesrc/Database/Connections/Connection.php (1)
5-7: Add a target removal version to the deprecation notice.The
@deprecatedtag should include a version number indicating when this class will be removed to help users plan migrations.Apply this diff:
/* - * @deprecated + * @deprecated Will be removed in v2.0. Extend driver-specific connections (MySqlConnection, PostgresConnection, etc.) instead. */src/Database/Schema/Grammars/Concerns/MySqlBasedGrammar.php (1)
24-24: Unused parameter$commandcan be removed.The static analysis tool correctly identifies that
$commandis not used in this method.However, retaining it may be intentional for interface consistency with other grammar implementations. If not needed, consider removing it:
- public function compileChange(Blueprint $blueprint, Fluent $command) + public function compileChange(Blueprint $blueprint)src/Database/Connections/HasConnection.php (1)
16-16: Consider adding native type hints to properties.Both
$doctrineConnectionand$doctrineTypeMappingsrely on PHPDoc for type information. Adding native PHP type hints would improve static analysis and reduce the PHPStan issues mentioned in the PR context.Apply this diff:
- protected $doctrineConnection = null; + protected ?DoctrineConnection $doctrineConnection = null;- protected $doctrineTypeMappings = []; + protected array $doctrineTypeMappings = [];Also applies to: 23-23
| public function isDoctrineAvailable() | ||
| { | ||
| return class_exists('Doctrine\DBAL\Connection'); | ||
| } | ||
|
|
||
| /** | ||
| * Indicates whether native alter operations will be used when dropping or renaming columns, even if Doctrine DBAL is installed. | ||
| * | ||
| * @return bool | ||
| */ | ||
| public function usingNativeSchemaOperations() | ||
| { | ||
| return ! $this->isDoctrineAvailable(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify whether Doctrine DBAL is optional or required.
The runtime checks in isDoctrineAvailable() and usingNativeSchemaOperations() suggest Doctrine DBAL is optional, but the PR context indicates a required upgrade from Doctrine DBAL 2 to 3. Additionally, registerDoctrineType() (line 180-184) throws a RuntimeException if Doctrine is unavailable, further suggesting it's expected to be present.
If Doctrine DBAL is now a required dependency, these runtime checks and the exception in registerDoctrineType() may be unnecessary.
Run the following script to check if Doctrine DBAL is declared as a required or optional dependency:
🏁 Script executed:
#!/bin/bash
# Description: Check composer.json for Doctrine DBAL dependency status
# Check if doctrine/dbal is in require or require-dev
if [ -f "composer.json" ]; then
echo "=== Checking Doctrine DBAL dependency status ==="
cat composer.json | jq -r '.require["doctrine/dbal"] // "NOT IN REQUIRE"'
echo "---"
cat composer.json | jq -r '."require-dev"["doctrine/dbal"] // "NOT IN REQUIRE-DEV"'
else
echo "composer.json not found"
fiLength of output: 76
Let me verify how these methods are used and check the registerDoctrineType() method context:
🏁 Script executed:
#!/bin/bash
# Find usages of isDoctrineAvailable and usingNativeSchemaOperations
echo "=== Usages of isDoctrineAvailable() ==="
rg -n "isDoctrineAvailable\(\)" --type=php
echo ""
echo "=== Usages of usingNativeSchemaOperations() ==="
rg -n "usingNativeSchemaOperations\(\)" --type=php
echo ""
echo "=== registerDoctrineType() method (lines 180-184) ==="
rg -n -A8 "registerDoctrineType" --type=phpLength of output: 1903
Let me check if usingNativeSchemaOperations() is actually being used anywhere:
🏁 Script executed:
#!/bin/bash
# Search more broadly for usage of usingNativeSchemaOperations
echo "=== All references to usingNativeSchemaOperations ==="
rg -n "usingNativeSchemaOperations" --type=php
echo ""
echo "=== Check in test files ==="
rg -n "usingNativeSchemaOperations" --type=php -g '**test*'
echo ""
echo "=== Check in all files (any extension) ==="
rg -n "usingNativeSchemaOperations"Length of output: 320
Remove or update dead code related to optional Doctrine support.
Doctrine DBAL is a required dependency (^3.0 in composer.json), but the code maintains runtime checks suggesting it's optional:
usingNativeSchemaOperations()is never called anywhere in the codebase and always returnsfalse(dead code).registerDoctrineType()throws aRuntimeExceptionif Doctrine is unavailable, but since Doctrine is required, this exception path is unreachable.- These checks contradict the actual dependency status and should be removed or the logic clarified.
| // Doctrine v2 expects one parameter while v3 expects two. 2nd will be ignored on v2... | ||
| return $this->getDoctrineDriver()->getSchemaManager( | ||
| $connection, | ||
| $connection->getDatabasePlatform() | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify Doctrine DBAL v2/v3 compatibility approach.
The comment states "Doctrine v2 expects one parameter while v3 expects two. 2nd will be ignored on v2..." However, passing two arguments to a method expecting one in PHP will typically result in an ArgumentCountError rather than silently ignoring the extra parameter.
If this PR only targets Doctrine DBAL v3 (as suggested by the PR context of "Doctrine DBAL 2 → 3"), the comment may be outdated and the code is correct. If both v2 and v3 support is required, you'll need conditional logic based on the Doctrine version.
Run the following script to check the Doctrine DBAL version constraints and verify the compatibility approach:
🏁 Script executed:
#!/bin/bash
# Description: Check Doctrine DBAL version constraint and verify getSchemaManager signature
echo "=== Doctrine DBAL Version Constraint ==="
if [ -f "composer.json" ]; then
cat composer.json | jq -r '.require["doctrine/dbal"] // .["require-dev"]["doctrine/dbal"] // "NOT FOUND"'
fi
echo ""
echo "=== Checking for Doctrine v2/v3 compatibility code ==="
# Search for version checks or conditional logic related to Doctrine
rg -n "doctrine.*version|DBAL.*version" --type php -i
echo ""
echo "=== Check if there are any other Doctrine compatibility comments ==="
rg -n "doctrine v[0-9]|dbal v[0-9]" --type php -iLength of output: 314
Update or remove the misleading Doctrine v2 compatibility comment.
The codebase targets only Doctrine DBAL v3 (^3.0 in composer.json), with no conditional logic for v2 support. The comment at line 134 suggesting v2 compatibility is outdated and misleading.
Recommended fix: Replace the comment with clarification that the two-parameter call is required for Doctrine DBAL v3:
// Doctrine DBAL v3 requires SchemaManager to accept Connection and AbstractPlatform
return $this->getDoctrineDriver()->getSchemaManager(
$connection,
$connection->getDatabasePlatform()
);
🤖 Prompt for AI Agents
In src/Database/Connections/HasConnection.php around lines 134 to 138, the
existing comment claiming Doctrine v2 compatibility is outdated and misleading;
update it to state that Doctrine DBAL v3 requires passing both the Connection
and its AbstractPlatform, then replace the old comment with a concise
clarification that the two-parameter call is required for Doctrine DBAL v3 while
leaving the return call unchanged.
| if (! Type::hasType($name)) { | ||
| Type::getTypeRegistry() | ||
| ->register($name, is_string($class) ? new $class() : $class); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Doctrine DBAL v3-only API may break v2 compatibility.
The code uses Type::getTypeRegistry()->register() which is Doctrine DBAL v3 API. In Doctrine DBAL v2, the equivalent was Type::addType($name, $class).
Combined with the v2/v3 compatibility comment in getDoctrineSchemaManager() (line 134), there's inconsistency in the compatibility approach. If this PR drops Doctrine DBAL v2 support entirely (as suggested by "Doctrine DBAL 2 → 3" in the PR description), this is fine. However, if v2 support must be maintained, you'll need conditional logic here.
🏁 Script executed:
#!/bin/bash
# Description: Search for other uses of Doctrine v3-specific APIs that might break v2 compatibility
echo "=== Searching for Type::getTypeRegistry() usage (v3 only) ==="
rg -n "Type::getTypeRegistry|getTypeRegistry\(\)" --type php
echo ""
echo "=== Searching for Type::addType() usage (v2 API) ==="
rg -n "Type::addType|addType\(" --type php
echo ""
echo "=== Check composer.lock for installed Doctrine version ==="
if [ -f "composer.lock" ]; then
cat composer.lock | jq -r '.packages[] | select(.name == "doctrine/dbal") | {name: .name, version: .version}'
fiLength of output: 258
I need to gather more information about the Doctrine version requirements and the PR context.
Now I need to verify the composer.json to confirm the Doctrine version requirements:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check composer.json for Doctrine DBAL version requirements
echo "=== composer.json (if exists) ==="
if [ -f "composer.json" ]; then
cat composer.json | jq '.require, .["require-dev"]' 2>/dev/null || grep -i doctrine composer.json
else
echo "composer.json not found"
fi
echo ""
echo "=== Check HasConnection.php for version-related comments ==="
if [ -f "src/Database/Connections/HasConnection.php" ]; then
cat src/Database/Connections/HasConnection.php | grep -i -E "(doctrine|dbal|v2|v3|version|compatibility|support)" | head -20
fi
Length of output: 119
Let me check for documentation files that might clarify the Doctrine version strategy:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check for documentation files mentioning Doctrine version
echo "=== Checking README.md and similar ==="
fd -e md -e txt | head -20 | xargs grep -l -i "doctrine\|dbal\|v2\|v3" 2>/dev/null || echo "No docs found or no matches"
echo ""
echo "=== Check if composer.json exists elsewhere ==="
fd "composer.json" -o "composer.lock" 2>/dev/null
echo ""
echo "=== Check file context around line 186-189 in HasConnection.php ==="
if [ -f "src/Database/Connections/HasConnection.php" ]; then
sed -n '170,200p' src/Database/Connections/HasConnection.php
fi
Length of output: 893
Let me check the getDoctrineSchemaManager() method at line 134 to verify the claimed compatibility comment:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check lines around 134 in HasConnection.php for the claimed compatibility comment
if [ -f "src/Database/Connections/HasConnection.php" ]; then
echo "=== Lines 120-150 around line 134 ==="
sed -n '120,150p' src/Database/Connections/HasConnection.php
fi
Length of output: 943
Now I have the critical context! Let me verify whether there's any intent stated about dropping v2 support:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for any conditional logic or version checks in the codebase
echo "=== Search for version detection or compatibility checks ==="
rg -n "isDoctrineAvailable|getDoctrineDriver|version" --type php src/ | head -15
echo ""
echo "=== Search for TODO/FIXME about v2/v3 ==="
rg -n "TODO|FIXME" --type php src/Database/Connections/HasConnection.php
Length of output: 1549
Fix Doctrine DBAL v2 compatibility or explicitly drop v2 support.
The code uses Type::getTypeRegistry()->register() (Doctrine DBAL v3+ only), but the codebase already demonstrates v2/v3 compatibility efforts elsewhere—specifically in getDoctrineSchemaManager() at lines 136-137, which includes a comment acknowledging both versions.
This creates an inconsistency: the schema manager supports both versions, but custom type registration breaks on v2. Either:
- Add conditional logic here to use
Type::addType()for v2 andType::getTypeRegistry()->register()for v3 - Or confirm v2 support is being dropped project-wide and remove the compatibility comment from
getDoctrineSchemaManager()
🤖 Prompt for AI Agents
In src/Database/Connections/HasConnection.php around lines 186-189, the custom
type registration uses Type::getTypeRegistry()->register() which is DBAL
v3-only; update to detect DBAL version and use Type::addType() for v2 and
Type::getTypeRegistry()->register() for v3. Specifically, if
Type::getTypeRegistry method exists use the current register($name, $instance)
flow (instantiating when $class is a string); otherwise call
Type::addType($name, $className) where $className is the class string (if $class
is an object use get_class($class)), and keep the existing Type::hasType check
to avoid double registration.
| $sql = sprintf( | ||
| 'alter table %s alter column %s %s', | ||
| $this->wrapTable($blueprint), | ||
| $this->wrap($column), |
There was a problem hiding this comment.
Bug: Passing ColumnDefinition to wrap() will cause failure.
The wrap() method expects a string identifier (column name), not a ColumnDefinition object. This will produce invalid SQL.
Apply this fix:
$sql = sprintf(
'alter table %s alter column %s %s',
$this->wrapTable($blueprint),
- $this->wrap($column),
+ $this->wrap($column->name),
$this->getType($column)
);📝 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.
| $this->wrap($column), | |
| $this->wrap($column->name), |
🤖 Prompt for AI Agents
In src/Database/Schema/Grammars/SqlServerGrammar.php around line 37, the code
passes a ColumnDefinition object into $this->wrap(), which expects a string
identifier; replace that argument with the column's name string (e.g.
$column->name or $column->getName() depending on the ColumnDefinition API) so
wrap() receives the actual column name and produces valid SQL.
| public function getDefaultValue($value) | ||
| { | ||
| if (is_string($value)) { | ||
| $value = preg_replace('#\'#', '', $value); | ||
| } | ||
|
|
||
| return parent::getDefaultValue($value); | ||
| } |
There was a problem hiding this comment.
Bug: Regex strips all quotes, corrupting values like "O'Reilly".
The preg_replace('#\'#', '', $value) removes every single quote in the string, corrupting default values that contain apostrophes.
Apply this fix to only strip the wrapping quotes:
public function getDefaultValue($value)
{
- if (is_string($value)) {
- $value = preg_replace('#\'#', '', $value);
+ if (is_string($value) && strlen($value) >= 2 && $value[0] === "'" && substr($value, -1) === "'") {
+ $value = substr($value, 1, -1);
}
return parent::getDefaultValue($value);
}📝 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.
| public function getDefaultValue($value) | |
| { | |
| if (is_string($value)) { | |
| $value = preg_replace('#\'#', '', $value); | |
| } | |
| return parent::getDefaultValue($value); | |
| } | |
| public function getDefaultValue($value) | |
| { | |
| if (is_string($value) && strlen($value) >= 2 && $value[0] === "'" && substr($value, -1) === "'") { | |
| $value = substr($value, 1, -1); | |
| } | |
| return parent::getDefaultValue($value); | |
| } |
🤖 Prompt for AI Agents
In src/Database/Schema/Grammars/SqlServerGrammar.php around lines 60 to 67, the
current preg_replace strips every apostrophe inside the string (corrupting
values like "O'Reilly"); change the logic to only remove wrapping single quotes:
detect if the first and last characters are single quotes (or use a regex that
matches only leading and trailing single quotes, e.g. ^'(.*)'$) and strip those
so inner apostrophes are preserved, handling empty quoted strings correctly.
| * @param string $path | ||
| * @return string | ||
| */ | ||
| public function publicPath() | ||
| public function publicPath($path = '') | ||
| { | ||
| return $this->basePath; | ||
| return $this->joinPaths($this->basePath, $path); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify publicPath implementation aligns with Winter CMS directory structure.
The method signature correctly matches Laravel 12's API. However, the implementation uses $this->basePath directly rather than a dedicated public path property. In standard Laravel applications, the public path typically points to a public subdirectory (e.g., basePath('public')), but Winter CMS may use the base path as its public directory.
Run the following script to verify the expected public directory structure:
🏁 Script executed:
#!/bin/bash
# Description: Check if Winter CMS uses a separate 'public' directory or if public path equals base path
# Check for documentation or configuration mentioning public directory structure
rg -n -C3 "public.*path|publicPath" --type=php -g '!vendor/**' -g '!tests/**'
# Look for index.php or public-facing entry points
fd -t f "index.php" -E vendor -E node_modules -x echo "Found index.php at: {}"
# Check if there's a 'public' directory in typical Winter CMS structure
fd -t d "^public$" -d 2 -E vendor -E node_modulesLength of output: 16957
Now I need to search the codebase to see if there's a $publicPath property or if Winter CMS actually uses basePath as the public path.
publicPath method implementation does not match Winter CMS directory structure.
The publicPath method should return the path to the public/web directory, but the current implementation returns $this->joinPaths($this->basePath, $path), which returns the base path, not the public directory. Winter CMS creates a separate public directory in the project root, so publicPath() should append 'public' to the base path.
Fix required: Change line 76 to: return $this->joinPaths($this->basePath, 'public', $path);
🤖 Prompt for AI Agents
In src/Foundation/Application.php around lines 71 to 77, the publicPath() method
currently returns the base path instead of the project's public/web directory;
update the return to join the base path with the public directory and the
optional $path (i.e., call joinPaths($this->basePath, 'public', $path)) so
publicPath() correctly points to the public folder and still handles an empty
$path gracefully.
| public function testDummy() | ||
| { | ||
| $this->assertTrue(true, true); | ||
| } |
There was a problem hiding this comment.
Fix the assertion call signature.
The second parameter to assertTrue() should be a failure message string, not a boolean. Line 12 passes true as the message parameter.
Apply this diff:
- $this->assertTrue(true, true);
+ $this->assertTrue(true);Or if a message is desired:
- $this->assertTrue(true, true);
+ $this->assertTrue(true, 'Dummy assertion should pass');📝 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.
| public function testDummy() | |
| { | |
| $this->assertTrue(true, true); | |
| } | |
| public function testDummy() | |
| { | |
| $this->assertTrue(true); | |
| } |
🤖 Prompt for AI Agents
In tests/fixtures/events/EventTest.php around lines 10 to 13 the call
$this->assertTrue(true, true) uses a boolean as the second argument but
PHPUnit::assertTrue expects the second parameter to be a string message; fix by
removing the second argument so it reads $this->assertTrue(true) or, if you need
a message, replace the second argument with a string like
$this->assertTrue(true, 'optional failure message').
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Filesystem/FilesystemTest.php (1)
10-13: Call the parent setup so the Testbench app bootsSkipping
parent::setUp();means the Winter TestCase never initializes the Testbench application, so any code that expects the container/config to exist will break under Laravel 12’s harness. Please invoke the parent before creating the Filesystem instance.public function setUp(): void { + parent::setUp(); + $this->filesystem = new Filesystem(); }
♻️ Duplicate comments (19)
src/Foundation/Http/Middleware/CheckForMaintenanceMode.php (1)
26-45: Re-throw non-503 HttpExceptions to avoid silent suppression.This issue was already flagged in the previous review but remains unresolved. The catch block captures all
HttpExceptioninstances but only handles status 503. Non-503 HTTP errors (400, 401, 404, 500, etc.) are silently suppressed, causing the middleware to returnnulland breaking the expected error-handling flow.Apply the fix suggested in the previous review:
return Response::make(View::make($view, [ 'message' => $ex->getMessage() ]), 503); } + + throw $ex; } }src/Halcyon/MemoryCacheManager.php (1)
10-12: Critical: Constructor signature mismatch with MemoryRepository.This change passes a
$configparameter toMemoryRepository, but the class does not define a constructor that accepts this parameter. This will cause a fatal error at runtime when the repository is instantiated.As noted in the previous review,
MemoryRepositoryneeds a constructor defined to accept the$configparameter. The suggested fix from the prior review remains valid:public function __construct(Store $store, array $config = []) { parent::__construct($store); // Store or handle $config as needed }Verify whether
MemoryRepositoryhas been updated to accept this parameter:#!/bin/bash # Check if MemoryRepository constructor accepts $config parameter file="src/Halcyon/MemoryRepository.php" echo "=== Checking constructor in $file ===" rg -n "function __construct" "$file" -A5src/Support/helpers-array.php (1)
303-306: Implementation correct for Laravel 12; breaking change already flagged.The implementation correctly calls
Arr::shuffle($array)without a seed parameter, aligning with Laravel 12's API. The breaking change (removal of the$seedparameter) was comprehensively covered in the previous review, including verification scripts and migration guidance.src/Network/Http.php (1)
276-277: Critical: SSL verification remains disabled.Changing
CURLOPT_SSL_VERIFYHOSTfromfalseto0doesn't improve security—both values disable host verification. For proper SSL verification,CURLOPT_SSL_VERIFYHOSTshould be set to2(verify that the certificate's Common Name or Subject Alternative Name matches the requested hostname).As noted in the previous review, the current default makes all HTTPS requests vulnerable to man-in-the-middle attacks unless users explicitly call
verifySSL().Consider enabling SSL verification by default:
- curl_setopt($curl, CURLOPT_SSL_VERIFYPEER, false); - curl_setopt($curl, CURLOPT_SSL_VERIFYHOST, 0); + curl_setopt($curl, CURLOPT_SSL_VERIFYPEER, true); + curl_setopt($curl, CURLOPT_SSL_VERIFYHOST, 2);Then introduce a
noVerifySSL()method for explicit opt-out:/** * Disable SSL verification (use only for development/testing) */ public function noVerifySSL() { $this->setOption(CURLOPT_SSL_VERIFYPEER, false); $this->setOption(CURLOPT_SSL_VERIFYHOST, 0); return $this; }src/Database/PDO/Connection.php (4)
42-53: Don't rely on assert; handle PDO::exec() false explicitly
exec()can return false without throwing. Returning false violates the int return type and hides the error.
61-70: Guard against PDO::prepare() returning false
prepare()returns false on failure (no exception). The current code can trigger a TypeError increateStatement().
78-89: Guard against PDO::query() returning false
query()can return false. Avoid relying onassert().
158-161: quote() may return false; enforce string returnPDO::quote() returns string|false. Returning false violates the declared behavior.
composer.json (1)
60-60: Dev-branch dependency remains unpinnedThe dependency on
dev-add-phpunit-11-supportis still a floating dev branch, which can break without notice.src/Database/PDO/Concerns/ConnectsToDatabase.php (1)
24-26: Correct the exception branding.The guard still throws “Laravel requires…”, which is inaccurate for Winter CMS. Please update the message so diagnostics reference Winter instead.
- if (! isset($params['pdo']) || ! $params['pdo'] instanceof PDO) { - throw new InvalidArgumentException('Laravel requires the "pdo" property to be set and be a PDO instance.'); - } + if (! isset($params['pdo']) || ! $params['pdo'] instanceof PDO) { + throw new InvalidArgumentException('Winter requires the "pdo" property to be set and be a PDO instance.'); + }tests/Database/Schema/Grammars/PostgresSchemaGrammarTest.php (1)
49-52: Fix the expected nullable clause.Postgres uses
DROP NOT NULLto make a column nullable;'alter column "name" null'isn’t valid SQL and will never match the grammar output. Please assert againstdrop not null.- $this->assertSame('alter column "name" null', $parts[1]); + $this->assertSame('alter column "name" drop not null', $parts[1]);tests/GrammarTestCase.php (1)
37-40: Return the mock directly.
($this->connection)()invokes the mock as a callable and will crash at runtime. Just return the mock instance (as previously requested) so consumers get the configuredConnection.public function getConnection() { - return ($this->connection)(); + return $this->connection; }src/Database/Schema/Grammars/PostgresGrammar.php (1)
63-69: Trim only wrapping quotes when normalizing defaults.
preg_replace('#\'#', '', $value)deletes every apostrophe, so a default like'O'Connor'becomesOConnor, corrupting schema diffs. Only strip matching outer quotes instead of all occurrences.- if (is_string($value)) { - $value = preg_replace('#\'#', '', $value); - } + if (is_string($value) && strlen($value) >= 2 && $value[0] === "'" && substr($value, -1) === "'") { + $value = substr($value, 1, -1); + }src/Database/Schema/Grammars/Concerns/MySqlBasedGrammar.php (2)
33-38: Fix ColumnDefinition wrapping in ALTER statements.
wrap()expects an identifier string; passing theColumnDefinitioninstance triggers a fatal conversion error whencompileChange()runs. Use the column name (and only wraprenameTowhen present) so the ALTER statement can be generated.- $sql = sprintf( - '%s %s%s %s', - is_null($column->renameTo) ? 'modify' : 'change', - $this->wrap($column), - is_null($column->renameTo) ? '' : ' '.$this->wrap($column->renameTo), - $this->getType($column) - ); + $sql = sprintf( + '%s %s%s %s', + is_null($column->renameTo) ? 'modify' : 'change', + $this->wrap($column->name), + is_null($column->renameTo) ? '' : ' ' . $this->wrap($column->renameTo), + $this->getType($column) + );
59-63: Preserve inner quotes in default values.
preg_replace('#\'#', '', $value)removes every single quote, so defaults likeO'ReillybecomeOReilly. Strip only a single pair of wrapping quotes before delegating to the parent.- if (is_string($value)) { - $value = preg_replace('#\'#', '', $value); - } + if (is_string($value) && strlen($value) >= 2 && $value[0] === "'" && substr($value, -1) === "'") { + $value = substr($value, 1, -1); + }src/Database/Relations/Concerns/DeferOneOrMany.php (1)
127-133: Return the raw expression for deferred key lookups.By coercing the raw CAST expression to string (and typing the method as
string), the query builder now wraps it as an identifier inorWhereIn/whereNotIn, producing invalid SQL. Restore the previous behaviour by returning theExpressioninstance itself.- protected function getWithDeferredQualifiedKeyName(): string - { - return $this->parent->getConnection()->raw(DbDongle::cast( - DbDongle::getTablePrefix() . $this->related->getQualifiedKeyName(), - 'TEXT' - ))->getValue($this->parent->getGrammar()); - } + protected function getWithDeferredQualifiedKeyName() + { + return $this->parent->getConnection()->raw(DbDongle::cast( + DbDongle::getTablePrefix() . $this->related->getQualifiedKeyName(), + 'TEXT' + )); + }src/Auth/Models/User.php (1)
326-338: Keep the setter in sync with the configurable password column.The new
$authPasswordNamehook is great, butsetPasswordAttributestill hardcodes'password', so changing the column name writes the wrong attribute and leaves the real password un-hashed. UsegetAuthPasswordName()inside the mutator so reads/writes stay aligned.- if ($this->exists && empty($value)) { - unset($this->attributes['password']); - } else { - $this->attributes['password'] = $value; + $passwordField = $this->getAuthPasswordName(); + + if ($this->exists && empty($value)) { + unset($this->attributes[$passwordField]); + } else { + $this->attributes[$passwordField] = $value; // Password has changed, log out all users $this->attributes['persist_code'] = null; }tests/Database/Schema/Grammars/MySqlSchemaGrammarTest.php (1)
3-3: Fix namespace to match test autoloading.This test class sits under
tests/Database/...just like the others, which all live inWinter\Storm\Tests\…. Leaving the namespace asTests\…breaks PSR-4 resolution and PHPUnit discovery, so the suite will never execute these assertions. Please restore theWinter\Storm\Tests\Database\Schema\Grammarsprefix so the test runner can load it.-namespace Tests\Database\Schema\Grammars; +namespace Winter\Storm\Tests\Database\Schema\Grammars;src/Database/Connections/MariaDbConnection.php (1)
70-78: Bind values with the correct PDO types.Binding floats as
PDO::PARAM_INTtruncates decimals, and null / boolean values fall through toPDO::PARAM_STR. That leads to incorrect data writes and unexpected SQL semantics. Mirror PDO expectations by branching onis_null,is_bool,is_int, andis_floatseparately.- $statement->bindValue( - is_string($key) ? $key : $key + 1, - $value, - is_int($value) || is_float($value) ? PDO::PARAM_INT : PDO::PARAM_STR - ); + $statement->bindValue( + is_string($key) ? $key : $key + 1, + $value, + match (true) { + is_null($value) => PDO::PARAM_NULL, + is_bool($value) => PDO::PARAM_BOOL, + is_int($value) => PDO::PARAM_INT, + default => PDO::PARAM_STR, + } + );
🧹 Nitpick comments (16)
tests/Database/Traits/DeferredBindingTest.php (1)
9-11: Remove redundant import statement.The
usestatement on line 9 importsDbTestCase, but line 11 uses the fully-qualified\Winter\Storm\Tests\DbTestCase. Either remove the import or use the unqualified name in the class declaration.Apply this diff to remove the redundant import:
namespace Winter\Storm\Tests\Database\Traits; use Winter\Storm\Database\Models\DeferredBinding; use Winter\Storm\Database\Model; use Winter\Storm\Tests\Database\Fixtures\Post; use Winter\Storm\Tests\Database\Fixtures\Author; -use Winter\Storm\Tests\DbTestCase; class DeferredBindingTest extends \Winter\Storm\Tests\DbTestCase {src/Support/Facades/File.php (1)
47-47: Consider using the short name for consistency.Now that
FilesystemAdapteris imported at line 4, this line can use the short name instead of the fully qualified namespace to match the style used in lines 57-58.Apply this diff:
-* @method static bool isLocalDisk(\Illuminate\Filesystem\FilesystemAdapter $disk) +* @method static bool isLocalDisk(FilesystemAdapter $disk)tests/Database/Traits/SimpleTreeTest.php (1)
7-9: Consider removing the redundant import or using the imported alias.Since line 9 now uses the fully qualified class name
\Winter\Storm\Tests\DbTestCase, the import statement on line 7 is unused. For consistency, either remove the import or use the aliasDbTestCasein the class declaration.Option 1: Remove the unused import
-use Winter\Storm\Tests\DbTestCase; class SimpleTreeTest extends \Winter\Storm\Tests\DbTestCaseOption 2: Use the imported alias
class SimpleTreeTest extends \Winter\Storm\Tests\DbTestCase +class SimpleTreeTest extends DbTestCaseNote: If the fully qualified naming pattern is intentional across the PR for clarity or namespace consistency, then Option 1 would be preferred.
tests/Parse/SyntaxFieldParserTest.php (1)
379-404: Consider removing duplicate helper methods.These helper methods (
callProtectedMethod,getProtectedProperty,setProtectedProperty) duplicate functionality already available in the parent\Winter\Storm\Tests\TestCaseclass. The parent'scallProtectedMethodis visible in the code snippets and serves the same purpose.You can remove lines 379-404 and rely on the parent class implementation:
- // - // Helpers - // - - protected static function callProtectedMethod($object, $name, $params = []) - { - $className = get_class($object); - $class = new ReflectionClass($className); - $method = $class->getMethod($name); - $method->setAccessible(true); - return $method->invokeArgs($object, $params); - } - - public static function getProtectedProperty($object, $name) - { - $className = get_class($object); - $class = new ReflectionClass($className); - $property = $class->getProperty($name); - $property->setAccessible(true); - return $property->getValue($object); - } - - public static function setProtectedProperty($object, $name, $value) - { - $className = get_class($object); - $class = new ReflectionClass($className); - $property = $class->getProperty($name); - $property->setAccessible(true); - return $property->setValue($object, $value); - }tests/Validation/RuleObjectTest.php (1)
8-8: LGTM! Consider adding a use statement for consistency.The fully-qualified base class reference is correct and aligns with the PR-wide namespace refactoring for Laravel 12 compatibility.
For style consistency with the other imports (lines 3-6), you could optionally add a use statement at the top:
use Illuminate\Filesystem\Filesystem; use Winter\Storm\Translation\FileLoader; use Winter\Storm\Translation\Translator; use Winter\Storm\Validation\Factory; +use Winter\Storm\Tests\TestCase; -class RuleObjectTest extends \Winter\Storm\Tests\TestCase +class RuleObjectTest extends TestCasetests/Foundation/ApplicationTest.php (1)
6-6: Consider using ausestatement for cleaner code.The fully-qualified namespace
\Winter\Storm\Tests\TestCaseis explicit and correct, but PHP convention typically favors importing the class with ausestatement at the top of the file for improved readability.Apply this diff if you prefer the conventional approach:
+use Winter\Storm\Tests\TestCase; + -class ApplicationTest extends \Winter\Storm\Tests\TestCase +class ApplicationTest extends TestCaseNote: If the FQN usage is part of an intentional pattern across the test suite refactor, feel free to keep it as-is.
tests/Halcyon/ValidationTraitTest.php (1)
3-6: Consider applying the named class pattern totests/Database/Traits/ValidationTest.phpfor consistency.The script confirmed that only one other test file uses the anonymous class pattern:
tests/Database/Traits/ValidationTest.php(line 125). Since the named class approach is better suited for static analysis tools (relevant to the PR's PHPStan concerns), applying the same pattern to that file would improve consistency across trait tests.src/Foundation/Bootstrap/LoadConfiguration.php (1)
45-46: Consider investigating PHPStan concerns with env() calls.Both suppressions relate to
env()usage. The PHPStan baseline has grown significantly (136→513 issues); investigating and fixing the root type issues would be preferable to suppressing them.Also applies to: 56-57
src/Database/Traits/Revisionable.php (1)
160-161: Remove the redundant$this->datescheck from line 161.The condition
in_array($attribute, $this->getDates())already covers this case.getDates()returns either$this->datesdirectly (when timestamps are disabled) orarray_merge($this->dates, $defaults)(when enabled), so checkingin_array($attribute, $this->dates)separately is redundant.Simplify to:
if (in_array($attribute, $this->getDates()) || $this->isDateCastable($attribute)) {tests/Database/Traits/SortableTest.php (1)
5-7: Remove redundant import or use the imported alias consistently.The file imports
Winter\Storm\Tests\DbTestCaseon line 5 but then uses the fully-qualified name\Winter\Storm\Tests\DbTestCaseon line 7. Either remove the import and keep the fully-qualified reference, or use the imported aliasDbTestCase.Apply this diff to use the import alias:
-class SortableTest extends \Winter\Storm\Tests\DbTestCase +class SortableTest extends DbTestCaseOr remove the unused import:
-use Winter\Storm\Tests\DbTestCase; - -class SortableTest extends \Winter\Storm\Tests\DbTestCase +class SortableTest extends \Winter\Storm\Tests\DbTestCasetests/Parse/ArrayFileTest.php (1)
5-5: Remove redundant import or use the imported alias consistently.Similar to other test files in this PR, this file would benefit from consistency. The test class extends the fully-qualified
\Winter\Storm\Tests\TestCasebut there's no explicit import shown. Ensure consistency across the codebase.src/Database/Builder.php (1)
85-95: Consider adding Expression handling in exact mode.The
searchWhereInternalmethod now handlesExpressioninstances in lines 107-109 for 'any'/'all' modes, but the 'exact' mode (lines 85-95) doesn't include similar handling. If a column is passed as an Expression in exact mode, it won't be properly resolved.Consider adding Expression handling in the exact mode block:
if ($mode === 'exact') { $this->where(function (Builder $query) use ($columns, $term) { foreach ($columns as $field) { if (!strlen($term)) { continue; } + if ($field instanceof Expression) { + $field = $field->getValue($query->getQuery()->getGrammar()); + } $fieldSql = $this->query->raw(sprintf("lower(%s)", DbDongle::cast($field, 'text'))); $termSql = '%' . trim(mb_strtolower($term)) . '%'; $query->orWhere($fieldSql, 'LIKE', $termSql); } }, null, null, $boolean);tests/Database/Relations/DuplicateRelationTest.php (1)
9-11: Consider removing the redundant import.Since the class extends the fully-qualified
\Winter\Storm\Tests\DbTestCase(line 11), the import statement on line 9 is no longer necessary and could be removed for cleaner code.Apply this diff to remove the redundant import:
-use Winter\Storm\Tests\DbTestCase; - -class DuplicateRelationTest extends \Winter\Storm\Tests\DbTestCase +class DuplicateRelationTest extends \Winter\Storm\Tests\DbTestCasetests/Database/Relations/DynamicRelationTest.php (1)
9-11: Consider removing the redundant import.The import statement on line 9 is redundant since the class extends clause uses the fully-qualified class name. This pattern appears across multiple test files in this PR.
Apply this diff:
-use Winter\Storm\Tests\DbTestCase; - -class DynamicRelationTest extends \Winter\Storm\Tests\DbTestCase +class DynamicRelationTest extends \Winter\Storm\Tests\DbTestCasetests/Database/Relations/BelongsToTest.php (1)
8-10: Consider removing the redundant import.The import statement on line 8 is unnecessary since the extends clause on line 10 uses the fully-qualified class name.
Apply this diff:
-use Winter\Storm\Tests\DbTestCase; - -class BelongsToTest extends \Winter\Storm\Tests\DbTestCase +class BelongsToTest extends \Winter\Storm\Tests\DbTestCasetests/Database/Traits/RevisionableTest.php (1)
7-9: Consider removing the redundant import.The import on line 7 is unnecessary since the extends clause uses the fully-qualified class name. This redundancy appears across multiple test files in this PR.
Apply this diff:
-use Winter\Storm\Tests\DbTestCase; - -class RevisionableTest extends \Winter\Storm\Tests\DbTestCase +class RevisionableTest extends \Winter\Storm\Tests\DbTestCase
| $ast = $parser->parse($contents); | ||
| } catch (Error $e) { | ||
| throw new SystemException($e); | ||
| throw new SystemException($e); |
There was a problem hiding this comment.
Fix type mismatch in SystemException constructor.
The Error object is passed directly to SystemException, but the constructor expects a string message as the first parameter and calls HtmlBuilder::clean($message) on it. This will cause a type error at runtime.
Apply this diff to fix the type mismatch:
- throw new SystemException($e);
+ throw new SystemException($e->getMessage(), $e->getCode(), $e);📝 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.
| throw new SystemException($e); | |
| throw new SystemException($e->getMessage(), $e->getCode(), $e); |
🤖 Prompt for AI Agents
In src/Config/ConfigWriter.php around line 37, the code throws the Error object
directly into SystemException which expects a string message; change the throw
to pass the error message string (e.g. $e->getMessage()) as the first parameter
and, if SystemException supports a previous exception parameter, pass the
original $e as the second parameter so HtmlBuilder::clean receives a string and
the original error is preserved.
| * @return int|false The exit code to return or false to continue the normal execution | ||
| */ | ||
| public function handleSignal(int $signal, /* int|false $previousExitCode = 0 */) | ||
| public function handleSignal(int $signal, int|false $previousExitCode = 0): int|false |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify unused parameter is required for interface compliance.
The $previousExitCode parameter is never used in the method body. If this signature is required by the SignalableCommandInterface or parent class contract, the parameter should remain. Otherwise, consider removing it.
Run the following script to check if this method signature matches a parent or interface requirement:
🏁 Script executed:
#!/bin/bash
# Check if handleSignal is defined in any parent class or interface
# Search for handleSignal method definitions in interfaces and parent classes
rg -n --type=php -A3 'function handleSignal' -g '!HandlesCleanup.php'
# Search for SignalableCommandInterface
rg -n --type=php -A10 'interface SignalableCommandInterface'Length of output: 0
I need to search more broadly to find the interface definition and understand the class context. Let me try a different approach:
---
🏁 Script executed:
```shell
#!/bin/bash
# Find HandlesCleanup.php and examine its structure
find . -name "HandlesCleanup.php" -type f
# Search for SignalableCommandInterface without exclusions
rg -n --type=php 'SignalableCommandInterface'
# Search for handleSignal more broadly
rg -n --type=php 'handleSignal'
# Search for interface definitions in console directory
fd -e php . src/Console | head -20
Length of output: 695
Now let me examine the actual method implementation:
---
🏁 Script executed:
```shell
#!/bin/bash
# View the handleSignal method implementation in HandlesCleanup.php
cat src/Console/Traits/HandlesCleanup.php | sed -n '40,70p'
# Also check if $previousExitCode is used anywhere in the file
rg -n 'previousExitCode' src/Console/Traits/HandlesCleanup.php
Length of output: 1031
Let me verify the Symfony interface requirement:
---
🌐 Web query:
```web
Symfony SignalableCommandInterface handleSignal method signature
💡 Result:
Signature (Symfony 6.3+):
public function handleSignal(int $signal): int|false
(Older Symfony versions used : void; returning no value is deprecated since 6.3.) [1][2]
Remove the unused $previousExitCode parameter to comply with SignalableCommandInterface.
The Symfony interface signature is public function handleSignal(int $signal): int|false, but the implementation includes an extra parameter that is not used and violates the interface contract. Remove int|false $previousExitCode = 0 from line 46.
🧰 Tools
🪛 PHPMD (2.15.0)
46-46: Avoid unused parameters such as '$previousExitCode'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In src/Console/Traits/HandlesCleanup.php around line 46, the method signature
for handleSignal includes an unused extra parameter `int|false $previousExitCode
= 0` which violates Symfony's SignalableCommandInterface; remove the
`$previousExitCode` parameter so the signature becomes `public function
handleSignal(int $signal): int|false`, and update any internal references (none
expected) and method calls to match the interface.
| $this->app->singleton('db.transactions', function ($app) { | ||
| return new DatabaseTransactionsManager; | ||
| }); |
There was a problem hiding this comment.
Inject the database manager into DatabaseTransactionsManager.
DatabaseTransactionsManager expects the DatabaseManager in its constructor. Instantiating it with no arguments will immediately throw a TypeError when the binding is resolved.
- $this->app->singleton('db.transactions', function ($app) {
- return new DatabaseTransactionsManager;
- });
+ $this->app->singleton('db.transactions', function ($app) {
+ return new DatabaseTransactionsManager($app['db']);
+ });📝 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.
| $this->app->singleton('db.transactions', function ($app) { | |
| return new DatabaseTransactionsManager; | |
| }); | |
| $this->app->singleton('db.transactions', function ($app) { | |
| return new DatabaseTransactionsManager($app['db']); | |
| }); |
🤖 Prompt for AI Agents
In src/Database/DatabaseServiceProvider.php around lines 76 to 78, the singleton
binding returns new DatabaseTransactionsManager with no arguments but the class
requires a DatabaseManager in its constructor; change the factory to resolve and
pass the database manager from the container (e.g.
$app->make(\Illuminate\Database\DatabaseManager::class) or $app['db']) into the
DatabaseTransactionsManager constructor so the dependency is injected when the
singleton is created.
| $changes = [$this->compileDropDefaultConstraint($blueprint, $command)]; | ||
| $schema = $this->connection->getSchemaBuilder(); | ||
| $table = $blueprint->getTable(); | ||
|
|
||
| $oldColumns = collect($schema->getColumns($table)); | ||
|
|
||
| foreach ($blueprint->getChangedColumns() as $column) { | ||
| $sql = sprintf( | ||
| 'alter table %s alter column %s %s', | ||
| $this->wrapTable($blueprint), | ||
| $this->wrap($column), | ||
| $this->getType($column) | ||
| ); | ||
|
|
||
| $oldColumn = $oldColumns->where('name', $column->name)->first(); | ||
| if (!$oldColumn instanceof ColumnDefinition) { | ||
| $oldColumn = new ColumnDefinition($oldColumn); | ||
| } | ||
|
|
||
| foreach ($this->modifiers as $modifier) { | ||
| if (method_exists($this, $method = "modify{$modifier}")) { | ||
| $mod = strtolower($modifier); | ||
| $col = isset($oldColumn->{$mod}) && !isset($column->{$mod}) ? $oldColumn : $column; | ||
| $sql .= $this->{$method}($blueprint, $col); | ||
| } | ||
| } | ||
|
|
||
| $changes[] = $sql; | ||
| } |
There was a problem hiding this comment.
Initialize $changes without creating nested arrays.
compileDropDefaultConstraint may return an array of drop statements (Laravel 12 does this). Wrapping that array in another array leaves $changes shaped like [ [ ... ], 'alter ...' ], so the schema builder later tries to execute an array as SQL. Cast to array instead of wrapping it to keep the list flat.
- $changes = [$this->compileDropDefaultConstraint($blueprint, $command)];
+ $changes = (array) $this->compileDropDefaultConstraint($blueprint, $command);📝 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.
| $changes = [$this->compileDropDefaultConstraint($blueprint, $command)]; | |
| $schema = $this->connection->getSchemaBuilder(); | |
| $table = $blueprint->getTable(); | |
| $oldColumns = collect($schema->getColumns($table)); | |
| foreach ($blueprint->getChangedColumns() as $column) { | |
| $sql = sprintf( | |
| 'alter table %s alter column %s %s', | |
| $this->wrapTable($blueprint), | |
| $this->wrap($column), | |
| $this->getType($column) | |
| ); | |
| $oldColumn = $oldColumns->where('name', $column->name)->first(); | |
| if (!$oldColumn instanceof ColumnDefinition) { | |
| $oldColumn = new ColumnDefinition($oldColumn); | |
| } | |
| foreach ($this->modifiers as $modifier) { | |
| if (method_exists($this, $method = "modify{$modifier}")) { | |
| $mod = strtolower($modifier); | |
| $col = isset($oldColumn->{$mod}) && !isset($column->{$mod}) ? $oldColumn : $column; | |
| $sql .= $this->{$method}($blueprint, $col); | |
| } | |
| } | |
| $changes[] = $sql; | |
| } | |
| $changes = (array) $this->compileDropDefaultConstraint($blueprint, $command); | |
| $schema = $this->connection->getSchemaBuilder(); | |
| $table = $blueprint->getTable(); | |
| $oldColumns = collect($schema->getColumns($table)); | |
| foreach ($blueprint->getChangedColumns() as $column) { | |
| $sql = sprintf( | |
| 'alter table %s alter column %s %s', | |
| $this->wrapTable($blueprint), | |
| $this->wrap($column), | |
| $this->getType($column) | |
| ); | |
| $oldColumn = $oldColumns->where('name', $column->name)->first(); | |
| if (!$oldColumn instanceof ColumnDefinition) { | |
| $oldColumn = new ColumnDefinition($oldColumn); | |
| } | |
| foreach ($this->modifiers as $modifier) { | |
| if (method_exists($this, $method = "modify{$modifier}")) { | |
| $mod = strtolower($modifier); | |
| $col = isset($oldColumn->{$mod}) && !isset($column->{$mod}) ? $oldColumn : $column; | |
| $sql .= $this->{$method}($blueprint, $col); | |
| } | |
| } | |
| $changes[] = $sql; | |
| } |
🤖 Prompt for AI Agents
In src/Database/Schema/Grammars/SqlServerGrammar.php around lines 27 to 55,
$changes is being initialized as an array that contains the result of
compileDropDefaultConstraint, but that method can already return an array, which
produces a nested array and later causes the schema builder to attempt executing
an array as SQL; change the initialization to flatten the result by casting to
array or merging (e.g. $changes = (array)
$this->compileDropDefaultConstraint($blueprint, $command);) so $changes is a
flat list of SQL strings and subsequent code can append alter statements
normally.
| // @phpstan-ignore-next-line | ||
| $reflector = new ReflectionClass($this); |
There was a problem hiding this comment.
🧩 Analysis chain
Consider investigating the root cause rather than suppressing.
The PHPStan suppression is part of a broader baseline expansion (136→513 issues). The PR discussion suggests triaging and fixing these issues rather than blanket-baselining them.
Run the following script to check what PHPStan error is being suppressed here:
🏁 Script executed:
#!/bin/bash
# Remove the phpstan-ignore and run analysis to see the actual error
vendor/bin/phpstan analyze src/Extension/ExtendableTrait.php --level=5 --no-progress 2>&1 | grep -A3 "line 562"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if phpstan exists and what version
if command -v vendor/bin/phpstan &> /dev/null; then
echo "=== PHPStan Version ==="
vendor/bin/phpstan --version
else
echo "PHPStan not found in vendor/bin"
fi
# Try running phpstan on the file without grep filter
echo ""
echo "=== PHPStan analysis output ==="
vendor/bin/phpstan analyze src/Extension/ExtendableTrait.php --level=5 --no-progress 2>&1 | head -50Length of output: 130
🏁 Script executed:
#!/bin/bash
# Check the actual file content around the line in question
echo "=== File content around lines 560-565 ==="
sed -n '560,565p' src/Extension/ExtendableTrait.php | cat -nLength of output: 253
🏁 Script executed:
#!/bin/bash
# Look for any phpstan configuration files
echo "=== PHPStan config files ==="
find . -maxdepth 2 -name "phpstan.neon*" -o -name ".phpstan.*" | head -20Length of output: 43
🏁 Script executed:
#!/bin/bash
# Get broader context around the suppression - full method
echo "=== Context: Full method containing line 562 ==="
sed -n '550,575p' src/Extension/ExtendableTrait.php | cat -nLength of output: 1110
🏁 Script executed:
#!/bin/bash
# Check class/trait definition at top of file
echo "=== File header and class/trait definition ==="
head -50 src/Extension/ExtendableTrait.php | cat -nLength of output: 1768
🏁 Script executed:
#!/bin/bash
# Check phpstan.neon configuration
echo "=== PHPStan configuration ==="
cat ./phpstan.neonLength of output: 567
🏁 Script executed:
#!/bin/bash
# Search for other phpstan-ignore suppressions in the same file
echo "=== Count of phpstan suppressions in this file ==="
grep -n "phpstan-ignore" src/Extension/ExtendableTrait.php | wc -l
echo ""
echo "=== All phpstan-ignore suppressions in this file ==="
grep -n "phpstan-ignore" src/Extension/ExtendableTrait.phpLength of output: 151
Fix the parameter type annotation instead of suppressing the error.
The suppression masks a legitimate type safety issue. The parameter $instance is typed as ?object (generic object), but line 15 calls $reflector->getParentClass(), which only exists on ReflectionClass. The fix is to change the parameter type from ?object $instance to ?ReflectionClass $instance so PHPStan can verify type safety, then remove the suppression at line 561.
🤖 Prompt for AI Agents
In src/Extension/ExtendableTrait.php around lines 561-562, remove the "//
@phpstan-ignore-next-line" and change the parameter type for $instance from
?object to ?ReflectionClass in the method signature that uses
$reflector->getParentClass(); update imports to include ReflectionClass (or use
\ReflectionClass) and adjust any callers if they currently pass a generic object
so they provide a ReflectionClass or null; this fixes the type-safety issue and
lets PHPStan validate the code.
| * @param string $path | ||
| * @return string | ||
| */ | ||
| public function publicPath() | ||
| public function publicPath($path = '') | ||
| { | ||
| return $this->basePath; | ||
| return $this->joinPaths($this->basePath, $path); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that publicPath should return basePath instead of a public directory path.
The method name and docblock indicate this should return "the path to the public / web directory," but the implementation returns $this->basePath (when $path is empty). In standard Laravel, publicPath() returns the path to the public directory (e.g., /var/www/html/public), not the application base path (e.g., /var/www/html).
If Winter CMS intentionally places public files at the base path (no separate public directory), this is correct but should be documented. Otherwise, this appears to be a bug that could break file path resolution.
Additionally, this method uses joinPaths while other path methods in this class use PathResolver::join (see line 86 in langPath). Consider using PathResolver::join for consistency.
Run the following script to check how other Winter CMS/Storm path methods are implemented and whether a public directory exists:
🏁 Script executed:
#!/bin/bash
# Check if there's a publicPath property or constant that should be used
rg -n "publicPath\s*=" --type=php
# Check how publicPath was used in the old Laravel/Illuminate base class
rg -n "function publicPath" --type=php -A 5
# Look for references to 'public' directory in the codebase
rg -n -C3 "['\"]/public['\"]" --type=php | head -50
# Check other path resolution patterns in Application.php
rg -n "PathResolver::join.*basePath" --type=phpLength of output: 1894
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for tests or usages of publicPath() method
rg -n "publicPath\(\)" --type=php -B2 -A2
# Check if there are any tests for the publicPath method
rg -n "publicPath" tests/ --type=php
# Look for any constants or properties defining public directory structure
rg -n "'public'|\"public\"" src/Foundation/Application.php -C2
# Check the joinPaths vs PathResolver::join usage in the file
rg -n "joinPaths|PathResolver::join" src/Foundation/Application.php
Length of output: 188
🏁 Script executed:
#!/bin/bash
# Search for usages and tests of publicPath() method
echo "=== publicPath() usages in codebase ==="
rg -n "publicPath\(\)" --type=php -B2 -A2
echo "=== publicPath tests ==="
rg -n "publicPath" tests/ --type=php -C2
echo "=== joinPaths method definition ==="
rg -n "function joinPaths|def joinPaths" --type=php -A5
echo "=== PathResolver::join vs joinPaths usage in Application.php ==="
rg -n "joinPaths|PathResolver::join" src/Foundation/Application.php -C1Length of output: 1608
🏁 Script executed:
#!/bin/bash
# Find joinPaths method definition - check if it's in Application.php or parent class
echo "=== joinPaths definition in Application.php ==="
rg -n "function joinPaths|public function joinPaths" src/Foundation/Application.php -A3
# Check parent class or trait for joinPaths
echo "=== joinPaths in parent Application class or traits ==="
rg -n "joinPaths" src/Foundation/Application.php -B5 | head -20
# Look at class definition and extends
echo "=== Application class definition and parent ==="
head -30 src/Foundation/Application.php | grep -A5 "class Application"
# Look for any public directory constant or property
echo "=== public directory references ==="
rg -n "public.*=.*['\"]public" src/Foundation/Application.phpLength of output: 480
🏁 Script executed:
#!/bin/bash
# Find ApplicationBase import
echo "=== ApplicationBase import ==="
head -50 src/Foundation/Application.php | grep -E "use.*ApplicationBase|use.*Illuminate"
# Check if there's any documentation or comments about publicPath design
echo "=== Comments/docs around publicPath ==="
sed -n '65,80p' src/Foundation/Application.php
# Check git history for publicPath changes
echo "=== Recent publicPath changes ==="
git log -p --all -S "publicPath" -- src/Foundation/Application.php | head -100
# Look for any README or documentation about directory structure
echo "=== Directory structure docs ==="
find . -maxdepth 2 -name "*.md" -type f | xargs grep -l "public\|directory\|structure" 2>/dev/null | head -5Length of output: 671
🏁 Script executed:
#!/bin/bash
# Check if there's a public directory in the repo
echo "=== Looking for public directories ==="
find . -maxdepth 3 -type d -name "public" 2>/dev/null
# Check if there's a storage/public or any public-like structure
echo "=== Directory structure at root ==="
ls -la | head -20
# Check what Laravel's publicPath typically expects
echo "=== Testing if joinPaths works like PathResolver::join ==="
rg -n "class.*joinPaths|function joinPaths" --type=php -A5 vendor/ 2>/dev/null | head -30
# Look for configuration files that might define paths
echo "=== Config files mentioning paths ==="
find . -maxdepth 3 -name "config.php" -o -name "*config*.php" 2>/dev/null | head -5Length of output: 1290
🏁 Script executed:
#!/bin/bash
# Check if there's a public directory in the repo
echo "=== Looking for public directories ==="
find . -maxdepth 3 -type d -name "public" 2>/dev/null
# Check directory structure at root
echo "=== Root directory structure ==="
ls -la . | grep -E "^d" | head -20
# Check if joinPaths is standard Laravel utility
echo "=== Checking if joinPaths is from Laravel base class ==="
rg -n "joinPaths" vendor/laravel/framework/src/Illuminate/Foundation/Application.php -A2 2>/dev/null | head -20
# Look for how publicPath is typically used in Laravel/Illuminate
echo "=== Searching for public_path usage in codebase ==="
rg -n "public_path\(\)" --type=php -C2 | head -30Length of output: 865
🌐 Web query:
What does Laravel's publicPath method return and how is it typically used?
💡 Result:
Laravel's publicPath (and the public_path() helper) returns the fully-qualified filesystem path to your application's public directory (e.g. /var/www/html/project/public). You can also pass a filename to get a path inside that directory: public_path('css/app.css') → /…/project/public/css/app.css. (laravel.com)
Typical uses
- Build filesystem paths to public assets or files you need to read/write (images, generated files). (laravel.com)
- Create responses that serve files from the public directory (or pass paths to file helpers).
- Combine with URLs or asset helpers when you need the server path (not the web URL). (laravel.com)
Example
- public_path() // returns public dir
- public_path('uploads/image.jpg') // returns full path to that file. (laravel.com)
Citations:
- 1: https://laravel.com/docs/11.x/helpers?utm_source=openai
- 2: https://laravel.com/docs/11.x/helpers?utm_source=openai
- 3: https://laravel.com/docs/11.x/helpers?utm_source=openai
- 4: https://laravel.com/docs/11.x/helpers?utm_source=openai
🏁 Script executed:
#!/bin/bash
# Final check: Does Application.php define a $publicPath property?
echo "=== Properties in Application.php ==="
rg -n "protected.*publicPath|private.*publicPath|public.*publicPath" src/Foundation/Application.php
# Check pattern of other path properties
echo "=== Path properties like pluginsPath ==="
rg -n "protected.*Path|private.*Path" src/Foundation/Application.php | head -20Length of output: 385
Fix publicPath() implementation to append the '/public' subdirectory and use PathResolver::join for consistency.
The current implementation returns $this->basePath instead of the public directory path, which breaks the Laravel publicPath() contract. Laravel's publicPath method should return the fully-qualified filesystem path to the public directory (e.g., /var/www/html/project/public), not the base path.
Additionally, this method is inconsistent with all other path methods in this class:
- All other path methods (pluginsPath, themesPath, tempPath, uploadsPath, mediaPath) use
PathResolver::join($this->basePath, '/subdirectory') - All other path methods have corresponding protected properties ($pluginsPath, $themesPath, etc.)
- publicPath() has neither a property nor uses PathResolver::join
Suggested fix: Either add a $publicPath property and follow the pattern of other methods, or at minimum change line 76 to:
return PathResolver::join($this->basePath, '/public', $path);🤖 Prompt for AI Agents
In src/Foundation/Application.php around lines 71 to 77, publicPath()
incorrectly returns the base path and doesn't follow the class pattern; add a
protected $publicPath property (like other path properties) and implement
publicPath($path = '') to return PathResolver::join($this->basePath, '/public',
$path) so it yields the fully-qualified public directory path and matches the
other path methods' use of PathResolver::join.
| * @param \Winter\Storm\Mail\MailManager $manager | ||
| * @return \Winter\Storm\Support\Testing\Fakes\MailFake | ||
| */ | ||
| public static function fake() | ||
| public static function fake(MailManager $manager) | ||
| { | ||
| static::swap($fake = new MailFake); | ||
| static::swap($fake = new MailFake($manager)); | ||
|
|
||
| return $fake; |
There was a problem hiding this comment.
Keep Mail::fake() backward compatible.
Requiring callers to pass a MailManager breaks the longstanding Mail::fake() API used across Winter and downstream apps. Have fake() resolve the manager from the container when none is supplied so existing tests keep working.
- public static function fake(MailManager $manager)
+ public static function fake(?MailManager $manager = null)
{
- static::swap($fake = new MailFake($manager));
+ $manager ??= static::getFacadeApplication()->make(MailManager::class);
+
+ static::swap($fake = new MailFake($manager));📝 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.
| * @param \Winter\Storm\Mail\MailManager $manager | |
| * @return \Winter\Storm\Support\Testing\Fakes\MailFake | |
| */ | |
| public static function fake() | |
| public static function fake(MailManager $manager) | |
| { | |
| static::swap($fake = new MailFake); | |
| static::swap($fake = new MailFake($manager)); | |
| return $fake; | |
| * @param \Winter\Storm\Mail\MailManager $manager | |
| * @return \Winter\Storm\Support\Testing\Fakes\MailFake | |
| */ | |
| public static function fake(?MailManager $manager = null) | |
| { | |
| $manager ??= static::getFacadeApplication()->make(MailManager::class); | |
| static::swap($fake = new MailFake($manager)); | |
| return $fake; |
🤖 Prompt for AI Agents
In src/Support/Facades/Mail.php around lines 35 to 42, make fake() backward
compatible by allowing the MailManager parameter to be optional: change the
signature to accept MailManager|null (or no type hint) with a default null, and
when null resolve the manager from the container (e.g.,
Container::getInstance()->make(MailManager::class) or app(MailManager::class)).
Keep the existing static::swap($fake = new MailFake($manager)) and return $fake,
and update the docblock to indicate the manager is optional/nullable so existing
callers can call Mail::fake() without passing a manager.
| class EventTest extends \Winter\Storm\Tests\TestCase | ||
| { | ||
| public function __construct() | ||
| { | ||
| parent::__construct("testDummy"); | ||
| } | ||
|
|
||
| public function testDummy() | ||
| { | ||
| $this->assertTrue(true, true); | ||
| } |
There was a problem hiding this comment.
Critical: Event fixture incorrectly extends TestCase.
This file is an event fixture used in tests/Events/DispatcherTest.php (line 46: $dispatcher->dispatch(new EventTest)), but it has been changed to extend \Winter\Storm\Tests\TestCase. Event fixtures should be simple POPOs (Plain Old PHP Objects), not test cases.
Additionally, the code has multiple issues:
- Line 7: Hardcoding
"testDummy"in the parent constructor is non-standard and serves no purpose for an event fixture - Line 12:
$this->assertTrue(true, true)has incorrect signature - the second parameter should be a message string, not a boolean
Revert this file to a simple event class:
<?php
-class EventTest extends \Winter\Storm\Tests\TestCase
+class EventTest
{
- public function __construct()
- {
- parent::__construct("testDummy");
- }
-
- public function testDummy()
- {
- $this->assertTrue(true, true);
- }
}🤖 Prompt for AI Agents
In tests/fixtures/events/EventTest.php around lines 3 to 13, the file was
incorrectly changed to extend \Winter\Storm\Tests\TestCase and include
test-specific code; revert it to a plain event POPO: remove the extends clause,
remove the constructor and any test methods/assertions, and restore it as a
simple class (optionally with any public properties or methods the
DispatcherTest expects) so it can be instantiated with new EventTest() during
dispatching.
| public function setUp(): void | ||
| { | ||
| $traitName = 'Winter\Storm\Support\Traits\Emitter'; | ||
| $this->traitObject = $this->getObjectForTrait($traitName); | ||
| $this->traitObject = new class { | ||
| use \Winter\Storm\Support\Traits\Emitter; | ||
| }; | ||
| } |
There was a problem hiding this comment.
Add parent::setUp() call to ensure proper test initialization.
The anonymous class approach correctly replaces the deprecated PHPUnit getObjectForTrait() method. However, the parent::setUp() call is missing, which could prevent proper test environment initialization when extending TestbenchTestCase.
Apply this diff to add the parent setUp call:
public function setUp(): void
{
+ parent::setUp();
+
$this->traitObject = new class {
use \Winter\Storm\Support\Traits\Emitter;
};
}📝 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.
| public function setUp(): void | |
| { | |
| $traitName = 'Winter\Storm\Support\Traits\Emitter'; | |
| $this->traitObject = $this->getObjectForTrait($traitName); | |
| $this->traitObject = new class { | |
| use \Winter\Storm\Support\Traits\Emitter; | |
| }; | |
| } | |
| public function setUp(): void | |
| { | |
| parent::setUp(); | |
| $this->traitObject = new class { | |
| use \Winter\Storm\Support\Traits\Emitter; | |
| }; | |
| } |
🤖 Prompt for AI Agents
In tests/Support/EmitterTest.php around lines 23 to 28, the setUp() override is
missing a call to parent::setUp(), which can prevent proper testbench
initialization; modify the setUp() method to call parent::setUp() at the start
of the method, then proceed to instantiate the anonymous class that uses
\Winter\Storm\Support\Traits\Emitter so the test environment is initialized
before creating traitObject.
Replaces #173, continuing the work done by @mjauvin @bennothommo & @wverhoogt
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores