Support PHPStan 2.x #245
Support PHPStan 2.x #245
Conversation
WalkthroughThis pull request introduces comprehensive type refinements and documentation improvements across multiple files in the Spanner database library. The changes primarily focus on enhancing type safety, updating PHPStan configurations, and improving method signatures. Key modifications include updating the PHPStan version, refining type annotations for method parameters and return types, and introducing more precise type definitions. The changes span various components of the library, including connection handling, query building, schema management, and model interactions, without fundamentally altering the core functionality of the existing code. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
zeriyoshi
force-pushed
the
phpstan2
branch
4 times, most recently
from
c5244b2 to
92ab8d5
Compare
zeriyoshi
added
enhancement
| @@ -1,43 +1,4 @@ | |||
| parameters: | |||
| level: max | |||
| level: 9 | |||
There was a problem hiding this comment.
PHPStan 2.0 max is level 10 (previously 9)
https://phpstan.org/blog/phpstan-2-0-released-level-10-elephpants
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/Eloquent/Concerns/DoesNotAutoIncrement.php (1)
49-51: Consider adding type for $attributes parameter instead of ignoring the error.Rather than using
@phpstan-ignore missingType.iterableValue, consider adding a proper type annotation for the$attributesparameter to maintain type safety.src/Query/Parameterizer.php (1)
63-64: Consider using type-cast instead of assertion.While the assertion helps PHPStan understand the type, consider using a type-cast for better runtime performance:
- assert(is_string($newQuery)); + $newQuery = (string)$newQuery;src/SpannerServiceProvider.php (1)
96-98: Consider using type-cast instead of PHPDoc assertion.While the PHPDoc assertion helps PHPStan understand the type, consider using array type-casting for better runtime type checking:
- /** - * @var TConfig - */ + /** @var array<string, mixed> $defaults */ + $defaults = [ + 'prefix' => '', + 'name' => $name, + 'cache_path' => null, + 'session_pool' => [], + ]; + return array_replace($defaults, array_intersect_key($config, $defaults));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
composer.json(1 hunks)phpstan.neon(1 hunks)src/Concerns/ManagesMutations.php(5 hunks)src/Concerns/ManagesPartitionedDml.php(1 hunks)src/Concerns/ManagesTransactions.php(4 hunks)src/Connection.php(16 hunks)src/Eloquent/Concerns/DoesNotAutoIncrement.php(1 hunks)src/Eloquent/Model.php(4 hunks)src/Events/MutatingData.php(1 hunks)src/Query/ArrayValue.php(1 hunks)src/Query/Builder.php(6 hunks)src/Query/Concerns/UsesMutations.php(3 hunks)src/Query/Concerns/UsesPartitionedDml.php(1 hunks)src/Query/Grammar.php(5 hunks)src/Query/Parameterizer.php(2 hunks)src/Query/Processor.php(5 hunks)src/Schema/Blueprint.php(3 hunks)src/Schema/Builder.php(3 hunks)src/Schema/ChangeStreamDefinition.php(1 hunks)src/Session/SessionInfo.php(1 hunks)src/SpannerServiceProvider.php(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Concerns/ManagesPartitionedDml.php
🔇 Additional comments (50)
phpstan.neon (1)
2-2: Verify the impact of stricter type checking.The change from
level: maxtolevel: 9and removal ofignoreErrorsindicates a move towards stricter type checking. This is a positive change that will help catch potential type-related issues early.Run the following script to verify the impact:
src/Query/ArrayValue.php (1)
25-27: LGTM! Improved type safety with precise array type annotation.The added PHPDoc with
array<array-key, mixed>type annotation enhances type safety and static analysis capabilities.src/Events/MutatingData.php (1)
36-36: Verify the impact of broadening the array type.The type change from
list<array<string, mixed>>toarray<array-key, mixed>broadens the accepted data structure. Please ensure this doesn't introduce potential issues with data validation.Run the following script to verify the usage:
Also applies to: 44-44
✅ Verification successful
Type change is appropriate and maintains type safety
The broader type
array<array-key, mixed>better reflects the actual implementation:
- All inputs are normalized through
prepareForMutation- Single records are automatically wrapped into a list structure
- The code enforces consistent data structure regardless of input type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how $values is used across the codebase # Search for MutatingData instantiation rg -A 5 'new MutatingData\(' # Search for references to $values property rg -A 5 'MutatingData.*->values'Length of output: 1608
Script:
#!/bin/bash # Check MutatingData class implementation and related type hints ast-grep --pattern 'class MutatingData { $$$ }' # Search for $dataSet construction and type hints rg -B 3 -A 3 '\$dataSet\s*=' # Search for method signatures with $dataSet parameter ast-grep --pattern 'function $_($$$dataSet$$$) { $$$ }'Length of output: 3513
src/Eloquent/Concerns/DoesNotAutoIncrement.php (1)
40-41: LGTM! Improved return type annotation.The
@return neverannotation correctly indicates that this method throws an exception and never returns.src/Query/Concerns/UsesPartitionedDml.php (3)
24-26: LGTM! Good type safety improvement.The addition of the
TConnectiontemplate type and updating the property type improves type safety and makes the code more maintainable.
32-32: LGTM! Better type specification.The change from
arraytoarray<string, mixed>provides better type information for static analysis.
21-21: Verify compatibility with Illuminate Grammar.The change from
Colopl\Spanner\Query\GrammartoIlluminate\Database\Query\Grammars\Grammaris significant. Ensure that all Spanner-specific SQL generation features are compatible with the base Illuminate Grammar class.✅ Verification successful
The Grammar import change appears to be safe
The switch from
Colopl\Spanner\Query\GrammartoIlluminate\Database\Query\Grammars\Grammaris safe as the trait only uses Grammar for type hinting and doesn't rely on any Spanner-specific functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any Spanner-specific methods in the old Grammar class that might be needed rg -l "class.*Grammar.*extends" | xargs rg "function.*Override"Length of output: 65
Script:
#!/bin/bash # Find Spanner Grammar class definition and content ast-grep --pattern 'class Grammar extends $_' # Find how Grammar is used in UsesPartitionedDml rg -B2 -A2 "Grammar" "src/Query/Concerns/UsesPartitionedDml.php" # Look for any methods in Grammar classes ast-grep --pattern 'class Grammar { $$$ function $name($_) { $$$ } $$$ }'Length of output: 385
Script:
#!/bin/bash # Look for any method implementations in UsesPartitionedDml ast-grep --pattern 'trait UsesPartitionedDml { $$$ function $name($_) { $$$ } $$$ }' # Find any direct usage of $grammar property rg "\\\$grammar->" "src/Query/Concerns/UsesPartitionedDml.php"Length of output: 159
src/Query/Concerns/UsesMutations.php (2)
20-20: LGTM! Good type reusability.The import and reuse of
TDataSettype fromManagesMutationsimproves consistency and maintainability across the codebase.Also applies to: 25-26
32-32: LGTM! Consistent type usage.The consistent use of
TDataSettype across all mutation methods (insertUsingMutation,updateUsingMutation,insertOrUpdateUsingMutation) improves type safety.Also applies to: 41-41, 50-50
src/Session/SessionInfo.php (1)
63-65: LGTM! Better type inference.The introduction of an intermediate variable with explicit type annotation improves static analysis capabilities without affecting runtime behavior.
src/Eloquent/Model.php (2)
35-35: LGTM! More precise array type.The change from
string[]tolist<string>provides better type information by explicitly indicating a sequential array.
46-46: Verify scalar type restriction compatibility.The change from
mixedtoscalartype for route binding and attribute casting might be too restrictive. Please verify that:
- Route binding values are always scalar (int, float, string, or bool)
- No array or object values are used as route parameters
- Attribute casting doesn't need to handle array or object values
Also applies to: 60-60, 73-73
✅ Verification successful
The scalar type restriction is appropriate and safe
The codebase analysis confirms that:
- Route binding primarily handles database key lookups with scalar values
- Type casting is focused on primitive type conversions (e.g., string to INT64)
- No evidence of array or object values being used in route parameters or attribute casting
The change from
mixedtoscalaraligns with the actual usage patterns in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential non-scalar route bindings rg -A 3 "resolveRouteBinding|resolveChildRouteBinding" | grep -v "scalar" # Check for potential array or object attribute casting rg -A 2 "protected \$casts\s*=" | grep -E "array|json|object|collection"Length of output: 1240
Script:
#!/bin/bash # Find parent class and its route binding implementation ast-grep --pattern 'class Model extends $_' # Check tryCastAttribute implementation and usage ast-grep --pattern 'tryCastAttribute($_,$_)' # Look for array/object type hints in related methods rg "(?:array|object)\s+\$value" src/Length of output: 130
src/Schema/ChangeStreamDefinition.php (1)
26-31: Verify the impact of nullable properties.The change to make all properties nullable might affect existing code that assumes these properties are always set. Please ensure that:
- This change is intentionally made to match Cloud Spanner's behavior
- All consuming code properly handles null values
✅ Verification successful
Nullable properties are safely implemented
The change to nullable properties is safe because:
- All property access is encapsulated through fluent setter methods
- No direct property access found in the codebase
- The class properly extends Fluent and follows the builder pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct property access without null checks rg -A 2 '\$(?:retentionPeriod|valueCaptureType|excludeTtlDeletes|excludeInsert|excludeUpdate|excludeDelete)\b(?!\s*\?\?|\s*\?:|null\s*!==)'Length of output: 574
Script:
#!/bin/bash # Search for class usage and property access patterns echo "=== Searching for class usage ===" rg "ChangeStreamDefinition" -A 2 echo -e "\n=== Searching for property access patterns ===" # Search for each property individually with simple patterns for prop in "retentionPeriod" "valueCaptureType" "excludeTtlDeletes" "excludeInsert" "excludeUpdate" "excludeDelete"; do echo -e "\nSearching for \$$prop:" rg "\\\$$prop" -A 2 done echo -e "\n=== Searching for property assignments ===" ast-grep --pattern 'this->$_ = $_'Length of output: 7913
src/Query/Parameterizer.php (1)
38-39: LGTM! Type annotations are now more precise.The array types are now properly specified with key and value types, improving type safety.
src/Query/Processor.php (3)
30-32: LGTM! Improved type annotations for processSelect.The array types are now properly specified, improving type safety.
54-58: Excellent use of template types!The template type annotation for
processColumnclearly specifies the conditional return types based on input type.
75-86: LGTM! Detailed array structure documentation.The return type structure is now clearly documented with all possible fields and their types.
src/SpannerServiceProvider.php (1)
33-42: LGTM! Well-structured config type definition.The
TConfigtype alias clearly defines the expected structure of configuration arrays.src/Concerns/ManagesMutations.php (2)
31-33: Well-defined type alias enhances code clarity.The introduction of
TDataSettype alias provides clear type constraints for the data structure used in mutations.
123-124: Return type enhancement improves type safety.The return type
array<array-key, iterable<array-key, mixed>>provides more precise type information for the mutation preparation result.src/Query/Grammar.php (2)
131-134: Enhanced type safety for whereInUnnest parameters.The type definition
array{ values: Nested, column: string, not: bool }provides clear constraints for the where clause structure.
192-193: Explicit return type annotation for supportsSavepoints.The
@return falseannotation clearly documents the method's behavior.src/Concerns/ManagesTransactions.php (2)
43-43: Improved type safety for commit options.The type annotation
array<string, mixed>|nullprovides clear constraints for the commit options structure.
87-87: Enhanced null safety with null coalescing operator.The addition of null coalescing operator (
??) ingetName()calls prevents potential null reference issues.Also applies to: 179-179
src/Query/Builder.php (3)
29-30: Well-structured generic type implementation.The introduction of
TConnectiontemplate type and its usage in properties and traits improves type safety and maintainability.Also applies to: 37-37, 45-45
140-141: Enhanced type safety for whereIn values parameter.The type constraint
array<int, mixed>|Arrayable<int, mixed>clearly defines the expected structure of the values parameter.
230-231: Precise return type for runSelect method.The return type
array<int, array<array-key, mixed>>clearly defines the structure of the result set.src/Schema/Blueprint.php (11)
65-67: LGTM! Type annotation enhancement.The parameter type annotation for
$indexis now more specific and accurate, improving type safety.
253-253: LGTM! Enhanced type safety for options parameter.The type annotation for the
$optionsparameter is now more specific witharray<array-key, mixed>.
Line range hint
42-42: LGTM! Required import for type refinements.Added import for
Expressionclass to support type refinements in method signatures.
Line range hint
239-244: LGTM! Enhanced type safety for table method.The method signature now includes precise type hints for the
$tableparameter and return type.
253-253: LGTM! Enhanced return type.The return type is now more specific with
QueryBuilder<Connection>.
Line range hint
261-275: LGTM! Improved type annotations.Enhanced type annotations for
$bindingsparameter and return types in select and cursor methods.
Line range hint
321-322: LGTM! Simplified return handling.The statement method now returns
truedirectly after executing operations, improving code clarity.Also applies to: 329-330
Line range hint
637-637: LGTM! Added type annotation.Added type annotation for the row variable in executePartitionQuery.
Line range hint
690-690: LGTM! Simplified row count calculation.Simplified the row count calculation by directly using array_sum.
121-121: Verify the necessity of PHPStan ignore.The PHPStan ignore comment suggests a potential type mismatch. Let's verify if this is necessary or if we can fix the underlying type issue.
✅ Verification successful
PHPStan ignore is necessary and correctly used.
The ignore directive is required to support Cloud Spanner's string column type specifications, particularly the special 'max' length value, which differs from the parent implementation's type expectations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the parent class implementation and usage of the string method # to understand why the PHPStan ignore is needed. # Check the parent class implementation ast-grep --pattern 'class BaseBlueprint { $$$ public function string($column, $length = null) { $$$ } $$$ }' # Check the usage of the string method rg -A 5 '->string\('Length of output: 550
Script:
#!/bin/bash # Find Blueprint class implementation and string method ast-grep --pattern 'class Blueprint { $$$ public function string($$$) { $$$ } $$$ }' # Find string method usages with proper escaping rg '\->string\(' -A 5 # Find any related PHPStan ignore comments rg '@phpstan-ignore'Length of output: 11867
Script:
#!/bin/bash # Get the string method implementation and surrounding context cat src/Schema/Blueprint.php # Find any related type definitions rg -g '*.php' -A 3 'string\(' src/Schema/Length of output: 14299
Line range hint
112-112: Verify the necessity of PHPStan ignore.The PHPStan ignore comment suggests a type mismatch with the parent constructor. Let's verify if this can be resolved.
composer.json (1)
23-23: LGTM! Updated PHPStan to version 2.The update to PHPStan version 2 aligns with the type refinements across the codebase.
src/Connection.php (8)
42-42: LGTM! Type safety improvements.The new import and PHPStan ignore comment are correctly added to support type refinements.
Also applies to: 112-112
239-244: LGTM! Enhanced type safety with generic type hints.The method signatures now correctly specify that
QueryBuilderis parameterized withConnection, improving type safety.Also applies to: 253-253
261-263: LGTM! Improved array type hints.The method signatures now correctly specify array key and value types, enhancing type safety.
Also applies to: 273-275
314-315: LGTM! Simplified return statements.The method implementation is now more straightforward, returning
truedirectly after executing statements.Also applies to: 321-322, 329-330
338-339: LGTM! Enhanced type hints.The method signature now correctly specifies array key and value types.
610-610: LGTM! Improved request options initialization.The null coalescing operator provides a more concise way to set default values.
637-637: LGTM! Added type hint for row variable.The type hint improves type safety by specifying array key and value types.
690-690: LGTM! Simplified row count calculation.Removed unnecessary null coalescing operator as
rowCounts()always returns an array.src/Schema/Builder.php (3)
50-50: LGTM! Added return type hint.The type hint improves type safety by specifying the structure of returned arrays.
87-87: LGTM! Added PHPStan ignore comment.The comment is necessary as PHPStan cannot infer that
$this->resolveris a property.
140-140: LGTM! Added type annotation for connection variable.The type hint improves type safety by specifying that
$connectionis an instance ofConnection.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Query/Builder.php (2)
49-50: LGTM! Consider standardizing array type hints.The type annotations for insert/update operations are precise and well-documented. For consistency, consider using
array-keyinstead ofarray-keyin some places to maintain a uniform style across the codebase.Also applies to: 58-59, 73-75, 97-100
139-140: Consider adding runtime type validation for Arrayable values.The type annotations for
whereInandwhereInUnnestcorrectly specifyarray<int, mixed>|Arrayable<int, mixed>. However, when anArrayableis provided, consider validating that itstoArray()output matches the expected structure to catch potential runtime issues.public function whereIn($column, $values, $boolean = 'and', $not = false) { + if ($values instanceof Arrayable) { + $array = $values->toArray(); + if (!array_is_list($array)) { + throw new InvalidArgumentException('Arrayable must convert to a list array'); + } + $values = $array; + } // If parameter is over the limit...Also applies to: 181-186
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Connection.php(16 hunks)src/Query/Builder.php(7 hunks)src/Query/Concerns/UsesPartitionedDml.php(2 hunks)src/Query/Grammar.php(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Query/Concerns/UsesPartitionedDml.php
- src/Connection.php
- src/Query/Grammar.php
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run-PHPUnit
🔇 Additional comments (2)
src/Query/Builder.php (2)
29-31: LGTM! Well-structured type annotations.The addition of the
TConnectiontemplate type and its application to the$connectionproperty improves type safety and helps PHPStan better understand the connection type throughout the class.Also applies to: 44-47
229-230: LGTM! Clear and precise return type annotation.The return type
array<int, array<array-key, mixed>>accurately describes the structure of the result set from the select operation.
Co-authored-by: Takayasu Oyama <taka.oyama@gmail.com>
|
@taka-oyama |
PHPStan 2.0 Released 🎉
https://phpstan.org/
Summary by CodeRabbit
Release Notes
Type Safety Improvements
Dependency Updates
Configuration Changes
Development Tooling
These changes primarily focus on improving type safety, code quality, and static analysis capabilities within the project.