Skip to content

Conversation

@tyabu12
Copy link
Contributor

@tyabu12 tyabu12 commented Nov 27, 2025

No description provided.

Copilot AI review requested due to automatic review settings November 27, 2025 05:47
@coderabbitai
Copy link

coderabbitai bot commented Nov 27, 2025

Walkthrough

This pull request modifies three files to refine schema building and type handling. The phpstan.neon file updates error suppression rules from closure-related checks to more specific checks targeting the PDO constructor parameter in Connection and the resolver property in Schema\Builder. The src/Schema/Builder.php file changes the resolver invocation in the createBlueprint method to pass the database connection as the first argument before the table and callback parameters. Additionally, src/Schema/Grammar.php updates the docblock type annotation for the modifyInvisible method parameter to permit nullable invisibility values.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • The resolver invocation signature change in createBlueprint requires verification that all callers are compatible with the new argument order
  • PHPStan configuration changes should be validated to ensure they properly address underlying type issues
  • Confirm that the nullable invisibility type annotation in the docblock aligns with actual usage patterns throughout the codebase

Possibly related PRs

Suggested labels

bug, waiting for review

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request description is entirely missing, failing to follow the required template with essential sections like issue reference, checklist, and reference information. Add a complete pull request description following the template, including issue number reference, completed checklist items, and any relevant references or context.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main changes: fixing PHPStan static analysis errors across multiple files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request addresses PHPStan static analysis errors by correcting type annotations and fixing a method signature mismatch. The changes improve type safety without altering runtime behavior.

  • Updated PHPDoc type annotation for the invisible column property to accurately reflect that it can be null
  • Fixed resolver callback invocation to include the required connection parameter
  • Updated PHPStan baseline to reflect resolved and newly discovered errors

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/Schema/Grammar.php Updated PHPDoc to allow null for the invisible property type
src/Schema/Builder.php Added missing $this->connection parameter to resolver callback invocation
phpstan.neon Updated ignored errors baseline after fixing argument count issues

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cb4fdd7 and a26a45b.

📒 Files selected for processing (3)
  • phpstan.neon (1 hunks)
  • src/Schema/Builder.php (1 hunks)
  • src/Schema/Grammar.php (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Agent
  • GitHub Check: Run-PHPUnit
🔇 Additional comments (2)
src/Schema/Grammar.php (1)

885-885: LGTM! Docblock now correctly reflects nullable property.

The updated type annotation correctly reflects that invisible can be null, which aligns with the runtime check on line 890 ($column->invisible !== null).

phpstan.neon (1)

10-19: I'm unable to verify this review comment because the repository cannot be cloned. Let me try searching for context about these Laravel framework constraints:
<function_calls>

Laravel Illuminate\Database\Schema\Builder resolver property isset nullable


</function_calls>

Based on my attempts to verify the review comment, I encountered a critical blocker: the repository colopl/laravel-spanner cannot be cloned, preventing me from examining:

  1. The actual phpstan.neon configuration file
  2. The implementation in src/Connection.php
  3. The implementation in src/Schema/Builder.php

Without direct access to these files, I cannot definitively verify whether:

  • The Connection constructor truly receives Closure(): null and whether this is fixable at the source
  • The $resolver property is actually used in isset() despite non-nullable typing
  • These are legitimate type safety issues or necessary framework-related pragmatic suppressions

The review makes specific technical claims that require examination of the actual source code and type declarations to confirm or refute.


Review comment verification cannot be completed due to repository access failure.

The review identifies two PHPStan error suppressions in phpstan.neon and questions whether they hide legitimate type safety issues. However, verification requires direct examination of:

  1. src/Connection.php - to verify the __construct() signature and closure return type
  2. src/Schema/Builder.php - to verify the $resolver property type declaration and usage context

Without repository access, the underlying claims cannot be confirmed or contradicted. The review's concerns may be valid, but determining whether these are framework constraints requiring suppressions or fixable type safety issues requires access to the actual implementations.

Comment on lines 107 to 112
protected function createBlueprint($table, ?Closure $callback = null)
{
return isset($this->resolver)
? ($this->resolver)($table, $callback)
? ($this->resolver)($this->connection, $table, $callback)
: new Blueprint($this->connection, $table, $callback);
}
Copy link

@coderabbitai coderabbitai bot Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Script executed:

#!/bin/bash

# Search for setResolver calls
echo "=== setResolver calls ==="
rg -nP '\bsetResolver\s*\(' --type=php -A 5 -B 2

# Search for potential resolver implementations
echo -e "\n=== Potential resolver implementations ==="
rg -nP 'function\s*\([^)]*\$' --type=php | grep -E '\$(table|callback|connection)' -C 2

# Check the Builder class for resolver property and related methods
echo -e "\n=== Resolver-related methods in Builder ==="
rg -nP '(resolver|setResolver)' --type=php src/Schema/Builder.php -B 2 -A 5

Repository: colopl/laravel-spanner


<shell_result>
Repository: colopl/laravel-spanner
Script execution failed with unrecoverable error: [CAPABILITY:RUN_SHELL_SCRIPT] Failed to clone repository colopl/laravel-spanner: Clone operation failed
</shell_result>


Laravel Schema Builder setResolver resolver signature custom


<web_search_result>
Signature: a Closure that receives the table name and the callback and must return a Blueprint instance (or subclass).

Example:

  • $table is string (table name)
  • $callback is Closure|null (the migration closure)
  • return a new Blueprint (or your ExtendedBlueprint)

Code:

$schema = DB::connection()->getSchemaBuilder();

$schema->blueprintResolver(function (string $table, ?Closure $callback) {
    return new \App\Database\ExtendedBlueprint($table, $callback);
});

This is documented on the Schema Builder (blueprintResolver expects a Closure that returns a Blueprint) and in multiple examples showing returning new Custom/ExtendedBlueprint. [1][2]

Sources:
[1] Laravel Schema\Builder — blueprintResolver docs. [2] Example: custom/extended Blueprint usage (shows returning new CustomBlueprint).
</web_search_result>


colopl/laravel-spanner github schema builder resolver connection


<web_search_result>
Relevant code and docs:

  • Repository (source): colopl/laravel-spanner on GitHub. [1]
  • Package/README and installation notes (Packagist). [2]
  • Schema implementation — Schema features are implemented in src/Schema/Blueprint.php and src/Schema/Grammar.php (these files show the Spanner-specific Blueprint methods and grammar used by the Schema Builder). [3]

Notes on how it ties together:

  • The package extends Laravel's Schema Builder by providing a custom Blueprint (src/Schema/Blueprint.php) and Grammar (src/Schema/Grammar.php) so Schema::create()/Blueprint calls emit Spanner DDL. [3]
  • The Blueprint/Grammar are invoked via the normal Schema\Builder flow (Laravel's blueprint resolver pattern — blueprintResolver on the Schema Builder), so the connection's Schema\Builder is used with the package's Blueprint/Grammar to build DDL against the Spanner connection. (See package README / docs for usage.) [2][3]

Links:
[1] GitHub repo — colopl/laravel-spanner.
[2] Packagist / package page (README & config examples).
[3] DeepWiki summary referencing src/Schema/Blueprint.php and src/Schema/Grammar.php.

If you want, I can open the exact GitHub source files and paste the class/file paths and key methods (blueprint resolver usage, Connection class methods) with direct code links.
</web_search_result>


site:github.com colopl/laravel-spanner setResolver resolver connection parameter


<web_search_result>
Use Laravel's connection resolver API to register a resolver for the spanner driver. Example (in a service provider boot method):

  • Register resolver for the driver:
    DB::resolverFor('spanner', function ($config, $name) {
    // return a connection instance (new \Colopl\Spanner\Connection(...)) using $config/$name
    });

References:

  • colopl/laravel-spanner package README (installation / connection config). [1]
  • Laravel Connection API (resolverFor / getResolver). [2]

Sources:
[1] colopl/laravel-spanner — Packagist / README.
[2] Illuminate\Database\Connection — Laravel API (resolverFor / getResolver).
</web_search_result>


site:github.com/colopl/laravel-spanner "setResolver" OR "blueprintResolver" PHP code


<web_search_result>
I searched for occurrences of "setResolver" or "blueprintResolver" in the colopl/laravel-spanner repository on GitHub and did not find any matches. The repo exposes Spanner-specific schema/Blueprint features (see src/Schema/Blueprint.php), but those specific symbols do not appear in the codebase. [1][2]

Sources:

  • colopl/laravel-spanner package info (repo reference). [1]
  • Documentation index showing schema/Blueprint files for colopl/laravel-spanner. [2]
    </web_search_result>

laravel-spanner src/Schema/Builder.php custom resolver implementation


<web_search_result>
Short answer: use the Builder::blueprintResolver callback to return your custom Blueprint (and pass the connection/grammar if needed). Example for colopl/laravel-spanner (or any Laravel driver):

  1. Create your custom Blueprint class (extend the package's Blueprint or Illuminate\Database\Schema\Blueprint):
getSchemaBuilder()->blueprintResolver(function ($table, $callback) { return new MyBlueprint($table, $callback); }); } Notes - The Builder::blueprintResolver method is the standard Laravel hook for replacing Blueprint. ([api.laravel.com](https://api.laravel.com/docs/11.x/Illuminate/Database/Schema/Builder.html?utm_source=openai)) - For colopl/laravel-spanner use the package Blueprint/Grammar classes as the base if you need Spanner-specific features; set resolver on the spanner connection's schema builder. ([packagist.org](https://packagist.org/packages/colopl/laravel-spanner?utm_source=openai)) If you want, tell me which Laravel and laravel-spanner versions you use and I’ll give a ready-to-drop-in code example for that exact version. Citations: - 1: https://api.laravel.com/docs/11.x/Illuminate/Database/Schema/Builder.html?utm_source=openai - 2: https://packagist.org/packages/colopl/laravel-spanner?utm_source=openai ---
Based on my research, I can now provide a rewritten review comment. The web search confirms that Laravel's standard `blueprintResolver` pattern expects the signature `function($table, $callback)` and returns a Blueprint instance. The code change you've flagged does indeed pass `$this->connection` as the first argument, which deviates from the documented Laravel pattern. **Resolver signature change breaks custom implementations.** The resolver invocation now passes `$this->connection` as the first argument. This breaks compatibility with custom resolvers following Laravel's documented `blueprintResolver` pattern: `function($table, $callback)`. Any custom resolvers in this codebase or in dependent packages must be updated to accept: `function($connection, $table, $callback)`. Verify that: 1. No custom resolvers are defined in the codebase or tests that follow the original signature 2. All dependent code calling `setResolver()` has been updated to match the new signature 3. Documentation/changelog reflects this breaking API change
🤖 Prompt for AI Agents ``` In src/Schema/Builder.php around lines 107–112, the resolver is being invoked with ($this->connection, $table, $callback) which breaks compatibility with Laravel's documented blueprintResolver signature function($table, $callback); revert the invocation to call the resolver with ($table, $callback) so custom resolvers keep working, and if the connection must be available instead, update the setResolver API and all callers/tests and add a clear changelog entry documenting the breaking signature change. ```

Copy link
Contributor Author

@tyabu12 tyabu12 Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# Fetch Laravel 11.x Builder.php createBlueprint method
echo "=== Laravel 11.x createBlueprint method ==="
curl -sL "https://raw.githubusercontent.com/laravel/framework/11.x/src/Illuminate/Database/Schema/Builder.php" | grep -A 10 "protected function createBlueprint"

echo -e "\n=== Laravel 12.x createBlueprint method ==="
# Fetch Laravel 12.x Builder.php createBlueprint method
curl -sL "https://raw.githubusercontent.com/laravel/framework/12.x/src/Illuminate/Database/Schema/Builder.php" | grep -A 10 "protected function createBlueprint"

Length of output: 926


@tyabu12, you're absolutely correct! Thank you for catching this. I've verified the Laravel framework code, and indeed:

Laravel 11.x resolver signature:

call_user_func($this->resolver, $table, $callback, $prefix);

Laravel 12.x resolver signature:

call_user_func($this->resolver, $connection, $table, $callback);

Your change correctly updates the resolver invocation to match Laravel 12.x's new signature. My previous review comment was incorrect as it was based on the Laravel 11.x pattern. This is a legitimate upstream breaking change in the Laravel framework itself.

The change looks good! ✅


✏️ Learnings added
Learnt from: tyabu12
Repo: colopl/laravel-spanner PR: 293
File: src/Schema/Builder.php:107-112
Timestamp: 2025-11-27T05:59:10.884Z
Learning: In Laravel 12.x, the Schema Builder resolver signature changed from function($table, $callback, $prefix) in 11.x to function($connection, $table, $callback) in 12.x. The resolver is now invoked with the connection as the first parameter.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@taka-oyama taka-oyama merged commit 0328986 into colopl:master Nov 27, 2025
10 checks passed
@tyabu12 tyabu12 deleted the fix/phpstan-errors branch November 27, 2025 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants