feat: better phpstan support part2 #237
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several changes across multiple files, primarily focusing on enhancing type safety and clarity in method signatures within the Changes
Possibly related PRs
Suggested reviewers
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 (
|
| @@ -371,6 +371,7 @@ protected function addInterleaveToTable(Blueprint $blueprint) | |||
| protected function addRowDeletionPolicy(Blueprint $blueprint) | |||
| { | |||
| if (! is_null($command = $this->getCommandByName($blueprint, 'rowDeletionPolicy'))) { | |||
| /** @var RowDeletionPolicyDefinition $command */ | |||
There was a problem hiding this comment.
should use assert but that will break compatibility so it will be done in a later version
![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
🧹 Outside diff range and nitpick comments (4)
src/Schema/RowDeletionPolicyDefinition.php (1)
25-33: Consider enhancing type safety with policy constants and validation.While the current implementation is functional, consider these improvements for better type safety and developer experience:
- Add constants for valid policy values to prevent magic strings
- Add constructor validation for the properties
Here's a suggested implementation:
/** * @property string $column * @property string $policy * @property int $days * @extends Fluent<string, mixed> */ class RowDeletionPolicyDefinition extends Fluent { + public const POLICY_DELETE = 'DELETE'; + // Add other valid policy constants here + + /** + * @param array<string, mixed> $attributes + */ + public function __construct(array $attributes = []) + { + parent::__construct($attributes); + + if (!isset($this->column)) { + throw new \InvalidArgumentException('Column must be specified'); + } + if (!isset($this->policy)) { + throw new \InvalidArgumentException('Policy must be specified'); + } + if (!isset($this->days) || $this->days <= 0) { + throw new \InvalidArgumentException('Days must be a positive integer'); + } + if (!in_array($this->policy, [self::POLICY_DELETE], true)) { + throw new \InvalidArgumentException('Invalid policy value'); + } + } }src/Schema/ColumnDefinition.php (1)
25-41: Consider adding method-level type hints.While the property-level PHPDoc annotations are excellent for static analysis, consider whether any methods need to be added with proper type hints to further enhance type safety. The empty class extension pattern is valid, but documenting why we're not adding any additional functionality would improve maintainability.
Add a class-level PHPDoc comment explaining the purpose:
/** + * Extends the base column definition to provide strict typing information + * for Cloud Spanner specific column properties. + * * @property string $name * @property string $type * @property string|null $arrayTypesrc/Schema/Grammar.php (2)
Line range hint
541-733: Consider using an abstract method pattern for type definitions.While the type hints improve type safety, the repetitive pattern across type methods suggests an opportunity for better abstraction. Consider introducing an abstract method pattern or strategy pattern to handle different column types more elegantly.
Example implementation:
+interface ColumnTypeHandler { + public function getTypeDefinition(ColumnDefinition $column): string; +} +class StringColumnTypeHandler implements ColumnTypeHandler { + public function getTypeDefinition(ColumnDefinition $column): string { + return "string({$column->length})"; + } +} class Grammar extends BaseGrammar { + private array $typeHandlers = []; + + public function __construct() { + $this->typeHandlers['string'] = new StringColumnTypeHandler(); + // Add other handlers + } + + protected function getColumnType(string $type, ColumnDefinition $column): string { + return $this->typeHandlers[$type]->getTypeDefinition($column); + }
Line range hint
897-904: Add proper string escaping for default values.The
formatStringValuemethod doesn't escape special characters in string values, which could lead to SQL injection or syntax errors if the string contains quotes.Apply this fix:
protected function formatStringValue(Fluent $column, mixed $value): string { assert(is_string($value)); - return '"' . $value . '"'; + return '"' . str_replace('"', '\"', $value) . '"'; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
phpstan.neon(0 hunks)src/Schema/ColumnDefinition.php(1 hunks)src/Schema/Grammar.php(36 hunks)src/Schema/RowDeletionPolicyDefinition.php(1 hunks)
💤 Files with no reviewable changes (1)
- phpstan.neon
🔇 Additional comments (4)
src/Schema/RowDeletionPolicyDefinition.php (1)
1-24: LGTM! File structure follows best practices.
The file includes proper licensing, strict type declarations, and follows PSR-4 autoloading standards.
src/Schema/ColumnDefinition.php (2)
19-21: LGTM! Good use of strict typing.
The strict typing declaration and namespace are correctly defined, which aligns well with the PR's objective of improving PHPStan support.
26-37: Verify property types against Cloud Spanner's constraints.
Let's verify that the property types align with Cloud Spanner's constraints and supported features.
✅ Verification successful
Based on the examination of the Grammar.php file and other results, I can now provide a definitive response about the property types alignment with Cloud Spanner's constraints.
Property types are correctly aligned with Cloud Spanner's data type system
The property types in ColumnDefinition.php perfectly match Cloud Spanner's supported data types and constraints:
string: Maps to STRING(n) or STRING(MAX)arrayType: Supports Cloud Spanner's ARRAY type with proper inner type handlingnullable: Matches Spanner's NULL/NOT NULL constraint supportdefault: Properly handles all Spanner's supported default valueslength: Used for STRING(n) and BYTES(n) specificationsprecision/scale: Used for NUMERIC typeuseCurrent: Properly implemented for TIMESTAMP columnsvirtualAs/storedAs: Matches Spanner's computed column support
The implementation in Grammar.php shows proper type mapping and constraint handling for all these properties, with correct SQL generation for Cloud Spanner's syntax.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for Cloud Spanner column type definitions and constraints
# in the codebase to verify alignment with the property types.
# Test: Look for column type definitions
rg -l "column.*type" --type php
# Test: Look for Cloud Spanner specific type constraints
rg -l "spanner.*constraint" --type php
Length of output: 97
Script:
#!/bin/bash
# Let's examine the actual column type implementations and constraints
# Check for Cloud Spanner specific type definitions
rg "Cloud.*Spanner.*type" --type php -A 5
# Look for type mapping or type conversion logic
rg "protected\s+function\s+type" --type php -A 10
# Check Grammar.php for type definitions since it was found in previous search
cat src/Schema/Grammar.php
# Look for any Spanner-specific column definitions
rg "spanner.*column" --type php -i
Length of output: 38691
src/Schema/Grammar.php (1)
Line range hint 171-201: LGTM! Row deletion policy implementation looks solid.
The type hints and assertions for RowDeletionPolicyDefinition improve type safety while maintaining proper error handling for unknown policies.
Also applies to: 374-380
taka-oyama
changed the base branch from
master
to
feature/phpstan-support-part1
# Conflicts: # phpstan.neon
taka-oyama
force-pushed
the
feature/phpstan-support-part2
branch
from
e9da42e to
0e8c1f0
Compare
Part1 #236 needs to be merged first.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
ColumnDefinitionandRowDeletionPolicyDefinitionclasses to enhance database schema definitions.Bug Fixes
Illuminate\Support\Fluentclass, improving error reporting accuracy.Refactor
Grammarclass to improve type safety and clarity regarding expected parameters.