fix: phpstan errors #281
fix: phpstan errors #281
Conversation
Co-authored-by: halnique <[email protected]>
# Conflicts: # src/Schema/Builder.php # src/Schema/Grammar.php
WalkthroughThe changes update schema metadata handling in the codebase. Metadata extraction methods for tables, columns, indexes, and foreign keys are refactored to use object properties and return richer, more structured arrays. SQL grammar methods are expanded for detailed metadata queries. Tests are updated and extended to reflect these changes. Changes
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
| - | ||
| message: '#^Closure invoked with 2 parameters, 3 required\.$#' | ||
| identifier: arguments.count | ||
| count: 1 | ||
| path: src/Schema/Builder.php | ||
|
|
||
| - | ||
| message: '#^Parameter \#2 of closure expects Closure, Closure\|null given\.$#' | ||
| identifier: argument.type | ||
| count: 1 | ||
| path: src/Schema/Builder.php |
There was a problem hiding this comment.
This error will be fixed with this PR. #279
| /** @var list<array{ | ||
| * name: string, | ||
| * schema: string|null, | ||
| * schema_qualified_name: string, | ||
| * size: int|null, | ||
| * comment: string|null, | ||
| * collation: string|null, | ||
| * engine: string|null, | ||
| * parent: string|null | ||
| * }> $tables | ||
| */ |
There was a problem hiding this comment.
adds parent: string|null to shape.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/Schema/Grammar.php (1)
69-82: Missing schema filter & unused$schemaargument incompileColumns()Just like
compileTables(), the first parameter is never referenced.
Either:
- remove the parameter (breaking change – adjust callers), or
- add a predicate:
and table_schema = ….This prevents future confusion and eliminates phpstan’s warning.
src/Query/Processor.php (1)
171-183: Guard against empty delimiter falloutOnce
compileForeignKeys()is fixed (see previous comment) theexplode(',', …)logic works.
If not fixed, this returns an array with one long element. Consider defensive code:'columns' => $result->columns === '' ? [] : explode(',', $result->columns), 'foreign_columns' => $result->foreign_columns === '' ? [] : explode(',', $result->foreign_columns),Not critical after the grammar patch, but keeps the processor resilient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
Dockerfile(1 hunks)phpstan.neon(1 hunks)src/Query/Processor.php(2 hunks)src/Schema/Builder.php(3 hunks)src/Schema/Grammar.php(2 hunks)tests/Schema/BuilderTestLast.php(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run-PHPUnit
🔇 Additional comments (9)
Dockerfile (1)
37-37: Version pinning for protobuf PECL extensionPinning the protobuf extension to a specific version (4.30.2) is a good practice for reproducible builds. This ensures consistent behavior across development and production environments.
src/Schema/Builder.php (4)
62-62: Good use of more specific methodChanging from
getIndexestogetIndexListingis appropriate since you only need the index names, not the full metadata objects. This should improve performance slightly since less data needs to be processed.
85-95: Well-documented table structureThe detailed PHPDoc type annotation for the
$tablesvariable provides excellent documentation about the expected structure of each table entry. This improves code readability and helps with static analysis.
133-133: Appropriate parameter simplificationChanged from passing the entire foreign key array to only passing the name string when calling
dropForeign. This is more efficient and aligns with how the method is typically used.
96-96:✅ Verification successful
Missing getTables method
Line 96 calls
$this->getTables()but no implementation of this method is present in the file. Based on PHPStan ignores and test modifications, this appears to be an issue.
🏁 Script executed:
#!/bin/bash # Check if getTables method exists in this class or parent classes rg -A 3 "function getTables" src/Length of output: 35
🏁 Script executed:
#!/bin/bash # Search for getTables declaration across entire repo rg -n "function getTables" . # Show the top of src/Schema/Builder.php to see namespace and inheritance head -n 30 src/Schema/Builder.php # Locate the class declaration to check for parent class or traits rg -n "class .*Builder" -n src/Schema/Builder.phpLength of output: 1044
🌐 Web query:
Does the Laravel Illuminate\\Database\\Schema\\Builder class define a getTables method?💡 Result:
The
Illuminate\Database\Schema\Builderclass in Laravel does define agetTables()method. This method retrieves the tables that belong to the database as an array[7].Here is a summary of related methods in the class:
- getTables() — Gets the tables that belong to the database[7].
- getTableListing() — Gets the names of the tables in the database[1][7].
- There’s also a deprecated getAllTables() method[7].
So, if you are looking to obtain the list of tables in your database via the schema builder,
getTables()is indeed available as a method on this class[7].Citations:
- 1: https://laravel.com/api/11.x/Illuminate/Database/Schema/Builder.html
- 2: https://laravel.com/docs/12.x/migrations
- 3: https://laravel.com/docs/5.0/schema
- 4: https://laracasts.com/discuss/channels/general-discussion/how-to-include-schema-builder-in-a-controller
- 5: https://laravel.io/forum/07-28-2016-is-it-possible-to-get-metadata-describing-the-database-schema-of-an-eloquent-db
- 6: https://laravel.com/docs/11.x/migrations
- 7: https://api.laravel.com/docs/10.x/Illuminate/Database/Schema/Builder.html
- 8: https://laraveldoctrine.org/docs/current/migrations/builder
- 9: https://laravel-docs.readthedocs.io/en/latest/migrations/
- 10: https://laravel.com/api/9.x/Illuminate/Database/Schema/Builder.html
Inherited getTables method present
The call to
$this->getTables()is valid: it’s defined on the parent classIlluminate\Database\Schema\Builder, so no implementation is needed in your subclass.No changes required.
tests/Schema/BuilderTestLast.php (4)
216-226: Updated test for table structureThe test has been properly updated to verify the detailed structure of table metadata, including schema, parent, size, comment, collation, and engine properties.
229-250: Good test coverage for nullable columnsThe new test provides specific coverage for nullable columns with primary keys, validating all the metadata properties including type, collation, and default values.
252-273: Well-structured test for columns with defaultsThis test properly verifies columns with default values, ensuring that the default value is correctly represented in the column metadata.
335-363: Comprehensive foreign key metadata testThe new test for foreign keys thoroughly validates all aspects of foreign key metadata, including name, columns, referenced table and columns, and update/delete actions.
Merging 8.x fixes and also fix laravel 12 specific errors in 62bc09e.
Summary by CodeRabbit