Fix Route::middleware() chain inference on Psalm dev-master#889
Merged
Fix Route::middleware() chain inference on Psalm dev-master#889
Route::middleware() chain inference on Psalm dev-master#889Conversation
…master
The plugin stub for `Illuminate\Routing\Route::middleware()` previously restated
Laravel's source `@return ($middleware is null ? array : $this)` (as `string[]`
instead of `array`). On `vimeo/psalm` dev-master, the stub/source merge of two
identical conditional return types collapses chained calls like
`Route::post(...)->middleware(['web'])->withoutScopedBindings()` to the null
branch, breaking every subsequent call.
Discriminating tests confirmed `@psalm-variadic` is not the cause; removing the
stub method entirely fixes the chain (Laravel's source resolves correctly) but
loses the variadic call form `->middleware('a', 'b')` that the plugin originally
added the stub to support.
Workaround: drop the conditional from the stub (`@return $this`), keep
`@psalm-variadic`. Trade-off: the no-arg getter form `$route->middleware()` now
resolves to `$this` instead of `array<array-key, string>` — that form is rarely
called from user code.
Proper fix lives in `vimeo/psalm`. Refs #888.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR works around a regression on vimeo/psalm dev-master where merging Laravel source + plugin stubs causes Illuminate\Routing\Route::middleware() chains to infer the wrong (array) branch, breaking fluent chaining and consumers expecting a Route.
Changes:
- Simplifies the plugin stub for
Route::middleware()to always return$this(setter branch) to avoid the conditional-return merge bug. - Adds a new PHPT regression test covering array/string/variadic middleware setter calls, fluent chaining after
middleware([...]), facade chains, and passing the result intoRouteCollectionInterface::add(). - Documents guidance for stub authors to avoid re-declaring conditional
@returntypes that can collide during stub/source merges.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/Type/tests/RouteMiddlewareTest.phpt | Adds regression coverage for Route::middleware() fluent chaining and facade/consumer scenarios. |
| stubs/common/Routing/Route.stubphp | Changes Route::middleware() stub return type to $this to avoid conditional merge collapse. |
| docs/contributing/README.md | Adds contributor guidance about conditional @return collisions during Psalm stub/source merge. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Per Copilot review on #889: 'identical conditional' was misleading because the previous stub used `string[]` on the null branch while Laravel's source uses `array`. The shared piece is the `$middleware is null` check itself. Reword both the stub comment and the contributing-docs note to reflect that.
Collaborator
Author
|
Upstream issue filed: vimeo/psalm#11837 with a self-contained, Laravel-free reproducer. The stub comment in this PR was updated (commit |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue to Solve
On
vimeo/psalmdev-master, method chains involvingIlluminate\Routing\Route::middleware()infer the wrong branch of the plugin stub's conditional return type. Calls with non-null array arguments resolve toarray<array-key, string>instead of$this, breaking every subsequent chained call (->withoutScopedBindings(),->name(...), etc.) and any code passing the result toRouteCollectionInterface::add(Route).Related
Fixes #888
Solution Description
Discriminating tests confirmed the trigger is the duplicate conditional return type between the plugin stub (
($middleware is null ? string[] : $this)) and Laravel's source (($middleware is null ? array : $this)). Psalm dev-master mishandles the merge.@psalm-variadicwas ruled out as the cause.The proper layer for this is
vimeo/psalmitself. As a plugin-side workaround until the upstream bug is fixed, this PR drops the conditional from the stub (@return $this) and retains@psalm-variadic. Trade-off: the no-arg getter form$route->middleware()now resolves to$thisinstead ofarray<array-key, string>. That form is rarely called from user code.A regression test (
tests/Type/tests/RouteMiddlewareTest.phpt) covers all reported failure modes (chained method call, facade chain,RouteCollectionInterface::addconsumer) and pins the no-arg getter trade-off so future contributors can't silently re-trigger the bug by restoring the conditional. The new test fails on dev-master without the stub change (verified) and passes on both7.0.0-beta19and dev-master with it.A short note was added to
docs/contributing/README.mdwarning future stub authors against restating identical conditional return types from Laravel source.Checklist
tests/Type/)