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>
Realised that this would be too much a BC break given that these methods are intended to be extended upon.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
|
This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days. |
# Conflicts: # src/Foundation/Http/Middleware/CheckForMaintenanceMode.php
|
This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days. |
The compileChange override in vendor/winter/storm/src/Database/Schema/Grammars/SQLiteGrammar.php reads SQLite column type names as tinyint (from SQLite metadata for boolean columns), then calls getType() which looks for ->typeTinyint(... — but only typeTinyInteger exists in the base grammar file Illuminate\Database\Schema\Grammars\SQLiteGrammar. This should resolve migration issues others might hit when upgrading to 1.3
Builds on the Laravel 12 work (wintercms#207). Bumps to laravel/framework ^13, PHP ^8.3, tinker ^3, carbon ^3.8.4, symfony ^7.4|^8.0, testbench ^11 (PHPUnit kept at ^11.5.50 pending phpunit 12 support in test helpers). Code: defer ArraySource datasource setup out of the model boot cycle (L13 forbids instantiating a model while booting); restate Builder<Model> param on HasRelationships relation factories; fix Preferences::findRecord() scope typing; drop the unsupported 2nd arg to DownCommand option('status'); update schedule:list invokable-class test expectation; prune stale phpstan baseline entries; CI matrices to PHP 8.3/8.4.
Builds on the Laravel 12 work (wintercms#207). Bumps to laravel/framework ^13, PHP ^8.3, tinker ^3, carbon ^3.8.4, symfony ^7.4|^8.0, testbench ^11 (PHPUnit kept at ^11.5.50 pending phpunit 12 support in test helpers). Code: defer ArraySource datasource setup out of the model boot cycle (L13 forbids instantiating a model while booting); restate Builder<Model> param on HasRelationships relation factories; fix Preferences::findRecord() scope typing; drop the unsupported 2nd arg to DownCommand option('status'); update schedule:list invokable-class test expectation; prune stale phpstan baseline entries; CI matrices to PHP 8.3/8.4.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Database/Schema/Grammars/SQLiteGrammar.php (2)
151-155:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not strip all apostrophes from string defaults.
Line 154 removes every single quote, so defaults like
'O''Reilly'are corrupted toOReillybefore recompilation.Suggested fix
public function getDefaultValue($value) { - if (is_string($value)) { - $value = preg_replace('#\'#', '', $value); + if (is_string($value) && preg_match("/^'(.*)'$/s", $value, $matches) === 1) { + // Remove only outer SQL string quotes and unescape doubled apostrophes. + $value = str_replace("''", "'", $matches[1]); } return parent::getDefaultValue($value); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Database/Schema/Grammars/SQLiteGrammar.php` around lines 151 - 155, The getDefaultValue method currently strips every apostrophe from string defaults (corrupting values like 'O''Reilly'); change it to only remove surrounding single quotes and unescape SQL doubled quotes instead of removing all apostrophes: detect if $value starts and ends with a single quote, then strip the outer quotes and replace doubled single-quotes ('' -> ') (e.g. via trim($value, "'") and str_replace("''", "'", $value)) so internal apostrophes are preserved; update getDefaultValue accordingly.
49-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve falsy defaults during column rebuilds.
Line 49 drops valid defaults like
'0'or''because it uses a truthy check. That changes schema semantics duringchange().Suggested fix
- 'default' => $column['default'] ? new Expression($column['default']) : null, + 'default' => $column['default'] !== null ? new Expression($column['default']) : null,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Database/Schema/Grammars/SQLiteGrammar.php` at line 49, The current truthy check for $column['default'] in SQLiteGrammar.php drops valid falsy defaults like '0' or '' during column rebuilds; change the condition to explicitly preserve non-null defaults (e.g., check that the 'default' key exists and is not null—using array_key_exists('default',$column) && $column['default'] !== null or isset($column['default']) && $column['default'] !== null) so that $column['default'] values like '0' or '' are passed to new Expression(...) instead of being treated as absent; update the line constructing the 'default' entry accordingly within the code that rebuilds columns in class SQLiteGrammar.
🧹 Nitpick comments (2)
tests/Database/Schema/Grammars/SQLiteSchemaGrammarTest.php (2)
84-84: ⚡ Quick winAvoid hard-coding
tinyintmapping check to statement index 0.Line 84 is brittle if extra SQL (e.g., pragma toggles) is emitted before the rebuild statement.
Suggested stabilization
// compileChange maps 'tinyint' (SQLite's introspected type_name) to 'integer' $this->assertNotEmpty($statements); - $this->assertStringContainsString('"is_active" integer', $statements[0]); + $this->assertStringContainsString('"is_active" integer', implode('; ', $statements));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Database/Schema/Grammars/SQLiteSchemaGrammarTest.php` at line 84, The test currently asserts the tinyint mapping using a fixed index ($statements[0]) which is brittle; update the assertion in SQLiteSchemaGrammarTest to check that any element of the $statements array contains the string '"is_active" integer' (e.g., use an assertion that searches through $statements or uses something like array_filter/any to verify presence) instead of indexing into $statements[0], so the test passes even if extra SQL (pragma, etc.) appears before the rebuild statement.
33-33: ⚡ Quick winStrengthen nullable-change assertion to detect regressions.
Line 33 currently passes even if
not nullis still present; it only checks a broad substring.Suggested test hardening
$statements = $this->runBlueprint($changedBlueprint); $this->assertStringContainsString('"name" varchar', $statements[0]); + $this->assertStringNotContainsString('"name" varchar not null', $statements[0]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Database/Schema/Grammars/SQLiteSchemaGrammarTest.php` at line 33, Replace the weak substring check on $statements[0] with a stronger assertion that ensures the column definition for "name" is varchar and does not include NOT NULL; for example, keep the positive check (assertStringContainsString('"name" varchar', $statements[0])) and add assertStringNotContainsString('not null', $statements[0]) or use assertMatchesRegularExpression to assert the "name" column definition does not contain 'not null' (target the $statements[0] variable and the existing assertion call).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/Database/Schema/Grammars/SQLiteGrammar.php`:
- Around line 151-155: The getDefaultValue method currently strips every
apostrophe from string defaults (corrupting values like 'O''Reilly'); change it
to only remove surrounding single quotes and unescape SQL doubled quotes instead
of removing all apostrophes: detect if $value starts and ends with a single
quote, then strip the outer quotes and replace doubled single-quotes ('' -> ')
(e.g. via trim($value, "'") and str_replace("''", "'", $value)) so internal
apostrophes are preserved; update getDefaultValue accordingly.
- Line 49: The current truthy check for $column['default'] in SQLiteGrammar.php
drops valid falsy defaults like '0' or '' during column rebuilds; change the
condition to explicitly preserve non-null defaults (e.g., check that the
'default' key exists and is not null—using array_key_exists('default',$column)
&& $column['default'] !== null or isset($column['default']) &&
$column['default'] !== null) so that $column['default'] values like '0' or ''
are passed to new Expression(...) instead of being treated as absent; update the
line constructing the 'default' entry accordingly within the code that rebuilds
columns in class SQLiteGrammar.
---
Nitpick comments:
In `@tests/Database/Schema/Grammars/SQLiteSchemaGrammarTest.php`:
- Line 84: The test currently asserts the tinyint mapping using a fixed index
($statements[0]) which is brittle; update the assertion in
SQLiteSchemaGrammarTest to check that any element of the $statements array
contains the string '"is_active" integer' (e.g., use an assertion that searches
through $statements or uses something like array_filter/any to verify presence)
instead of indexing into $statements[0], so the test passes even if extra SQL
(pragma, etc.) appears before the rebuild statement.
- Line 33: Replace the weak substring check on $statements[0] with a stronger
assertion that ensures the column definition for "name" is varchar and does not
include NOT NULL; for example, keep the positive check
(assertStringContainsString('"name" varchar', $statements[0])) and add
assertStringNotContainsString('not null', $statements[0]) or use
assertMatchesRegularExpression to assert the "name" column definition does not
contain 'not null' (target the $statements[0] variable and the existing
assertion call).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1904f6e0-e6f4-496c-9586-2ab50795212c
📒 Files selected for processing (8)
src/Config/Repository.phpsrc/Console/Traits/ProcessesQuery.phpsrc/Database/Schema/Grammars/SQLiteGrammar.phpsrc/Halcyon/Processors/Processor.phpsrc/Html/FormBuilder.phpsrc/Support/Facades/Form.phptests/Database/Schema/Grammars/SQLiteSchemaGrammarTest.phptests/Scheduling/ScheduleListCommandTest.php
✅ Files skipped from review due to trivial changes (1)
- src/Support/Facades/Form.php
Fixes wintercms/winter#1500 Co-authored-by: Alex Orosciuc <alex.orosciuc@gmail.com>
# Conflicts: # .github/workflows/tests.yml # composer.json # phpstan.neon # src/Config/ConfigWriter.php # src/Config/Repository.php # src/Database/Connections/MySqlConnection.php # src/Database/Connections/PostgresConnection.php # src/Database/Connections/SQLiteConnection.php # src/Database/Connections/SqlServerConnection.php # tests/Database/Traits/HasSortableRelationsTest.php # tests/Parse/ArrayFileTest.php
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/Parse/Assetic/LessCompilerTest.php (2)
112-114: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the class closing-brace placement.
The supplied Code Quality / PHP job fails at Line 114. Remove the blank line before the class closing brace so the brace follows the class body immediately.
Minimal formatting fix
return $asset->getContent(); } - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Parse/Assetic/LessCompilerTest.php` around lines 112 - 114, In the test class shown in the diff, remove the blank line immediately before the class closing brace so the brace directly follows the class body.Source: Pipeline failures
29-32: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winFail before using paths derived from a failed temporary-directory setup.
These setUp methods ignore
mkdir()andPathResolver::resolve()failures. AfalserealPathis later cast to0and used in path/file operations outside the intended temp tree.
tests/Parse/Assetic/LessCompilerTest.php#L29-L32: checkPathResolver::resolve()before writing$this->tmpReal . '/secret.env'.tests/Parse/Assetic/JavascriptImporterTest.php#L25-L37: checkmkdir()andPathResolver::resolve()beforecombine()uses$this->tmpReal.tests/Parse/Assetic/LessImportResolverTest.php#L30-L30: checkPathResolver::resolve()before creating paths such as$this->tmpReal . '/inside/main.less'.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Parse/Assetic/LessCompilerTest.php` around lines 29 - 32, Make the affected test setUp methods fail immediately when temporary-directory setup is unsuccessful: in tests/Parse/Assetic/LessCompilerTest.php lines 29-32, validate PathResolver::resolve() before writing secret.env; in tests/Parse/Assetic/JavascriptImporterTest.php lines 25-37, validate mkdir() and PathResolver::resolve() before combine() uses tmpReal; and in tests/Parse/Assetic/LessImportResolverTest.php line 30, validate PathResolver::resolve() before constructing paths under tmpReal.tests/Parse/Assetic/LessImportResolverTest.php (1)
75-82: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd symlink-escape coverage for
LessImportResolverTest.
LessImportResolver::makeResolver()resolves symlink targets withrealpath(), but the test class has no symlink fixture. Add a coverage case where a symlink inside the context directory or an allowed root points outside that tree and the resolver returnsLessImportResolver::SENTINEL_PATH. Skip only when symlink creation is unavailable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Parse/Assetic/LessImportResolverTest.php` around lines 75 - 82, Add a symlink-escape test alongside testBlocksRelativeTraversalEscapeOutsideContextDir, creating a symlink within the resolver context or allowed root that targets a file or directory outside it, then assert resolving through that link returns LessImportResolver::SENTINEL_PATH. Use the existing temporary-fixture setup and skip only when symlink creation is unavailable.Source: Coding guidelines
🧹 Nitpick comments (2)
tests/Mail/MailParserTest.php (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse plain PHPUnit bases for the pure helper tests.
All four classes use
\Winter\Storm\Tests\TestCase, although their visible code does not use Laravel container state or application fixtures. Use\PHPUnit\Framework\TestCaseat each site after verifying that no inherited Storm setup is required.
tests/Mail/MailParserTest.php#L5-L5: change the parent class for the parser-only test.tests/Parse/Assetic/JavascriptImporterTest.php#L12-L12: change the parent class for the importer-only test.tests/Parse/Assetic/LessCompilerTest.php#L14-L14: change the parent class for the compiler-only test.tests/Parse/Assetic/LessImportResolverTest.php#L10-L10: change the parent class for the resolver-only test.As per coding guidelines, use
TestCasefor tests requiring the Laravel container and plain unit tests for pure helper classes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Mail/MailParserTest.php` at line 5, Use PHPUnit\Framework\TestCase instead of Winter\Storm\Tests\TestCase for the pure helper tests, after confirming no inherited Storm setup is required: update MailParserTest in tests/Mail/MailParserTest.php at lines 5-5, JavascriptImporterTest in tests/Parse/Assetic/JavascriptImporterTest.php at lines 12-12, LessCompilerTest in tests/Parse/Assetic/LessCompilerTest.php at lines 14-14, and LessImportResolverTest in tests/Parse/Assetic/LessImportResolverTest.php at lines 10-10.Source: Coding guidelines
tests/Parse/Assetic/JavascriptImporterTest.php (1)
24-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Laravel's random helper for temporary artifact names.
The same
bin2hex(random_bytes(4))pattern appears in all three fixture setups. Replace it withStr::random()orStr::uuid().
tests/Parse/Assetic/JavascriptImporterTest.php#L24-L24: replace the random suffix used for$this->tmpRoot.tests/Parse/Assetic/LessCompilerTest.php#L26-L26: replace the random suffix used for$this->tmpRoot.tests/Parse/Assetic/LessImportResolverTest.php#L22-L22: replace the random suffix used for$this->tmpRoot.As per coding guidelines, use Laravel
Str::random()orStr::uuid()for random temporary-file names; do not manually format UUIDs from random bytes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Parse/Assetic/JavascriptImporterTest.php` at line 24, Replace the manual bin2hex(random_bytes(4)) temporary-name suffix in the fixture setups at tests/Parse/Assetic/JavascriptImporterTest.php:24, tests/Parse/Assetic/LessCompilerTest.php:26, and tests/Parse/Assetic/LessImportResolverTest.php:22 with Laravel’s Str::random() or Str::uuid(), adding the necessary Str imports while preserving the existing $this->tmpRoot prefix.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/Parse/Assetic/LessCompilerTest.php`:
- Around line 112-114: In the test class shown in the diff, remove the blank
line immediately before the class closing brace so the brace directly follows
the class body.
- Around line 29-32: Make the affected test setUp methods fail immediately when
temporary-directory setup is unsuccessful: in
tests/Parse/Assetic/LessCompilerTest.php lines 29-32, validate
PathResolver::resolve() before writing secret.env; in
tests/Parse/Assetic/JavascriptImporterTest.php lines 25-37, validate mkdir() and
PathResolver::resolve() before combine() uses tmpReal; and in
tests/Parse/Assetic/LessImportResolverTest.php line 30, validate
PathResolver::resolve() before constructing paths under tmpReal.
In `@tests/Parse/Assetic/LessImportResolverTest.php`:
- Around line 75-82: Add a symlink-escape test alongside
testBlocksRelativeTraversalEscapeOutsideContextDir, creating a symlink within
the resolver context or allowed root that targets a file or directory outside
it, then assert resolving through that link returns
LessImportResolver::SENTINEL_PATH. Use the existing temporary-fixture setup and
skip only when symlink creation is unavailable.
---
Nitpick comments:
In `@tests/Mail/MailParserTest.php`:
- Line 5: Use PHPUnit\Framework\TestCase instead of Winter\Storm\Tests\TestCase
for the pure helper tests, after confirming no inherited Storm setup is
required: update MailParserTest in tests/Mail/MailParserTest.php at lines 5-5,
JavascriptImporterTest in tests/Parse/Assetic/JavascriptImporterTest.php at
lines 12-12, LessCompilerTest in tests/Parse/Assetic/LessCompilerTest.php at
lines 14-14, and LessImportResolverTest in
tests/Parse/Assetic/LessImportResolverTest.php at lines 10-10.
In `@tests/Parse/Assetic/JavascriptImporterTest.php`:
- Line 24: Replace the manual bin2hex(random_bytes(4)) temporary-name suffix in
the fixture setups at tests/Parse/Assetic/JavascriptImporterTest.php:24,
tests/Parse/Assetic/LessCompilerTest.php:26, and
tests/Parse/Assetic/LessImportResolverTest.php:22 with Laravel’s Str::random()
or Str::uuid(), adding the necessary Str imports while preserving the existing
$this->tmpRoot prefix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b29e1c6d-257c-4e36-937f-63112f8f064a
📒 Files selected for processing (19)
.github/workflows/code-analysis.yaml.github/workflows/tests.yml.gitignorecomposer.jsonphpstan.neonsrc/Config/ConfigWriter.phpsrc/Console/Traits/HandlesCleanup.phpsrc/Database/Builder.phpsrc/Database/Model.phpsrc/Extension/ExtendableTrait.phpsrc/Halcyon/Datasource/DbDatasource.phpsrc/Network/Http.phptests/Database/ModelTest.phptests/Filesystem/PathResolverTest.phptests/Mail/MailParserTest.phptests/Parse/Assetic/JavascriptImporterTest.phptests/Parse/Assetic/LessCompilerTest.phptests/Parse/Assetic/LessImportResolverTest.phptests/Parse/IniTest.php
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/Filesystem/PathResolverTest.php
- src/Console/Traits/HandlesCleanup.php
- src/Network/Http.php
- src/Extension/ExtendableTrait.php
- src/Config/ConfigWriter.php
- tests/Parse/IniTest.php
- .gitignore
- .github/workflows/code-analysis.yaml
- phpstan.neon
- .github/workflows/tests.yml
- src/Database/Builder.php
- src/Database/Model.php
Remove the getCountForPagination/runPaginationCountQuery override, which was added in #39 to support having() in count queries -- Laravel's base builder now handles that natively. Since 0a6b24e ("Laravel 10 changes") the override passed the count subquery builder to from() (which registers its bindings) and also called mergeBindings() on it, binding the WHERE values twice and throwing SQLSTATE[HY093] "Invalid parameter number" on any grouped paginate() with bound conditions. Falling back to the base implementation fixes this and restores union support. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/Database/QueryBuilderPaginateTest.php`:
- Around line 48-52: Update the query builder chain creating $paginator to call
select('group_id') before groupBy('group_id'), ensuring paginate(2) selects only
the grouped column while preserving the existing filter, ordering, and
pagination behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c5cf0497-1ccc-4736-9f7e-e68b777056c9
📒 Files selected for processing (2)
src/Database/QueryBuilder.phptests/Database/QueryBuilderPaginateTest.php
💤 Files with no reviewable changes (1)
- src/Database/QueryBuilder.php
| $paginator = DB::table('pagination_test') | ||
| ->whereRaw('(group_id = ? OR group_id > ?)', [1, 0]) | ||
| ->groupBy('group_id') | ||
| ->orderBy('group_id') | ||
| ->paginate(2); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)(Tests?|tests|database|migrations|src/Database/Builder.php|DbTestCase\.php)' || true
echo
echo "== target test context =="
target="$(git ls-files | rg '(^|/)QueryBuilderPaginateTest\.php$' | head -n1 || true)"
if [ -n "${target:-}" ]; then
wc -l "$target"
sed -n '1,140p' "$target" | cat -n
fi
echo
echo "== builder context =="
builder="$(git ls-files | rg '(^|/)Builder\.php$' | grep -F 'src/Database/Builder.php' | head -n1 || true)"
if [ -n "${builder:-}" ]; then
wc -l "$builder"
sed -n '120,180p' "$builder" | cat -n
fi
echo
echo "== pagination_test migrations/fixtures =="
rg -n "pagination_test|CREATE TABLE|table\('pagination_test'\)" -S .Repository: wintercms/storm
Length of output: 21150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== check select() behavior in query builder =="
python3 - <<'PY'
import re, pathlib
for p in pathlib.Path('.').rglob('*.php'):
if 'vendor' in p.parts or 'node_modules' in p.parts:
continue
s = p.read_text(errors='ignore')
if re.search(r'function select\s*\(|function get\s*\(', s):
print(p)
PYRepository: wintercms/storm
Length of output: 826
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate query builder implementation files =="
git ls-files | rg '(^|/)([A-Z].*Builder\.php|QueryBuilder|Base\.php|Grammar\.php|Schema).*\.php$' | sed -n '1,200p'
echo
echo "== precise builder method definitions =="
rg -n "function select\s*\(|function get\s*\(|function getColumns|SELECT \*" -S --glob '*.php' | sed -n '1,220p'Repository: wintercms/storm
Length of output: 958
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)(Tests?|tests|database|migrations|src/Database/Builder.php|DbTestCase\.php)' || true
echo
echo "== target test context =="
target="$(git ls-widgets | rg '(^|/)QueryBuilderPaginateTest\.php$' | head -n1 || true)"
if [ -n "${target:-}" ]; then
wc -l "$target"
sed -n '1,140p' "$target" | cat -n
fi
echo
echo "== builder context =="
builder="$(git ls-files | rg '(^|/)Builder\.php$' | grep -F 'src/Database/Builder.php' | head -n1 || true)"
if [ -n "${builder:-}" ]; then
wc -l "$builder"
sed -n '120,180p' "$builder" | cat -n
fi
echo
echo "== pagination_test migrations/fixtures =="
rg -n "pagination_test|CREATE TABLE|table\('pagination_test'\)" -S .Repository: wintercms/storm
Length of output: 17905
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DbTestCase =="
wc -l tests/DbTestCase.php
sed -n '1,220p' tests/DbTestCase.php | cat -n
echo
echo "== QueryBuilder select/get/compile relevant methods =="
wc -l src/Database/Builder.php src/Database/QueryBuilder.php
rg -n "class Builder|class QueryBuilder|function select|function get\(|function getCountForPagination|pagination" src/Database src -g '*.php' | sed -n '1,240p'
sed -n '1,140p' src/Database/QueryBuilder.php | cat -n
sed -n '140,280p' src/Database/QueryBuilder.php | cat -n
sed -n '280,500p' src/Database/Builder.php | cat -n
echo
echo "== composer Laravel version =="
if [ -f composer.json ]; then jq '.require,."require-dev"' composer.json; fiRepository: wintercms/storm
Length of output: 19147
🌐 Web query:
Laravel QueryBuilder paginate default columns SELECT * select('group_id) groupBy Postgres MySQL ONLY_FULL_GROUP_BY
💡 Result:
In Laravel, using the paginate() method with a groupBy() clause is a complex operation that often leads to errors due to how Laravel calculates the total record count for pagination [1][2][3]. When you call paginate(), Laravel executes two queries: the main query to fetch the items for the current page and a separate "count" query to determine the total number of records [4][5]. When a groupBy() is present, the default count query (often SELECT count(*)) can return incorrect results or trigger database errors because it tries to group the count itself [3]. Key Technical Considerations: 1. SQL Compliance (ONLY_FULL_GROUP_BY): PostgreSQL and modern MySQL (with ONLY_FULL_GROUP_BY enabled) strictly require that all columns in your SELECT list are either part of the GROUP BY clause or used within an aggregate function (e.g., SUM(), MIN(), MAX(), ANY_VALUE()) [6][7][8]. If you use select('*') while grouping, these databases will reject the query because non-aggregated columns cannot be deterministically included in the result [6][7]. 2. Laravel Pagination Strategy: To address the challenges of paginating grouped queries, consider these approaches: - Refine the Select Clause: Only select columns that are explicitly in your GROUP BY clause or properly aggregated [7]. - Manual Pagination: If standard paginate() fails due to the complexity of your grouping (especially with JOINs), you may need to perform a manual query, count the results, and use Illuminate\Pagination\LengthAwarePaginator to create the paginator instance manually [1][2]. - Use simplePaginate(): If you do not need the total page count, simplePaginate() may be more efficient as it avoids the complex count query, though it still requires valid SQL syntax for grouping [9]. 3. Common Pitfalls: - Duplicate Column Names: When using join() with select('*') alongside groupBy(), you may encounter "Duplicate column name" errors during the count query execution because Laravel creates a subquery wrapper that can conflict with existing column names [5]. - Performance: Using groupBy() on large datasets causes the paginator to process significantly more data than a standard query, often resulting in slow page loads [2]. To resolve these errors, ensure your SQL is compliant with your database's strict mode and adjust your select() clause to avoid selecting ambiguous, non-aggregated columns [6][7].
Citations:
- 1: https://stackoverflow.com/questions/65077526/laravel-groupby-with-pagination
- 2: https://jenssegers.com/laravel-pagination-with-grouping-and-eager-loading
- 3: Builder with a groupBy results in empty pagination->render() laravel/framework#8559
- 4: https://github.com/laravel/framework/blob/5.2/src/Illuminate/Database/Query/Builder.php
- 5: Can't paginate a query with a JOIN and select(*) laravel/framework#36556
- 6: https://dev.mysql.com/doc/refman/9.1/en/group-by-handling.html
- 7: https://laraveldaily.com/post/expression-1-of-select-list-is-not-in-group-by-two-fixes-in-laravel
- 8: https://dev.mysql.com/doc/refman/5.7/en/group-by-handling.html
- 9: Pagination fails on adding column alias and having condition on alias laravel/framework#16825
Select group_id before grouping.
paginate(2) calls get(['*']), which makes this grouped query select the non-grouped id and name columns. On PostgreSQL or MySQL with ONLY_FULL_GROUP_BY, that fails before pagination is counted. Add ->select('group_id') before ->groupBy('group_id').
Proposed fix
$paginator = DB::table('pagination_test')
+ ->select('group_id')
->whereRaw('(group_id = ? OR group_id > ?)', [1, 0])📝 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.
| $paginator = DB::table('pagination_test') | |
| ->whereRaw('(group_id = ? OR group_id > ?)', [1, 0]) | |
| ->groupBy('group_id') | |
| ->orderBy('group_id') | |
| ->paginate(2); | |
| $paginator = DB::table('pagination_test') | |
| ->select('group_id') | |
| ->whereRaw('(group_id = ? OR group_id > ?)', [1, 0]) | |
| ->groupBy('group_id') | |
| ->orderBy('group_id') | |
| ->paginate(2); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/Database/QueryBuilderPaginateTest.php` around lines 48 - 52, Update the
query builder chain creating $paginator to call select('group_id') before
groupBy('group_id'), ensuring paginate(2) selects only the grouped column while
preserving the existing filter, ordering, and pagination behavior.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…sh-prone rebuild (#239) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Builds on the Laravel 12 work (wintercms#207). Bumps to laravel/framework ^13, PHP ^8.3, tinker ^3, carbon ^3.8.4, symfony ^7.4|^8.0, testbench ^11 (PHPUnit kept at ^11.5.50 pending phpunit 12 support in test helpers). Code: defer ArraySource datasource setup out of the model boot cycle (L13 forbids instantiating a model while booting); restate Builder<Model> param on HasRelationships relation factories; fix Preferences::findRecord() scope typing; drop the unsupported 2nd arg to DownCommand option('status'); update schedule:list invokable-class test expectation; prune stale phpstan baseline entries; CI matrices to PHP 8.3/8.4.
# Conflicts: # composer.json
7 participants
Replaces #173, continuing the work done by @mjauvin @bennothommo & @wverhoogt
Summary by CodeRabbit
New Features
Bug Fixes