Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Route model binding fails when additional route parameters are present #52253

Closed
colincameron opened this issue Jul 24, 2024 · 6 comments
Closed

Comments

@colincameron
Copy link
Contributor

Laravel Version

11.17.0

PHP Version

8.3.9

Database Driver & Version

No response

Description

When using a resource controller within a route group that has a route parameter, the wrong parameter is sent to the resource controller method.

For example:

Route::domain('{subdomain}.' . config('app.url'))->group(function() {
    Route::resource('people', PersonController::class);
});

In PersonController::show(Person $person), the subdomain route parameter is passed, instead of the correct Person model. This results in the error "App\Http\Controllers\PersonController::show(): Argument #1 ($person) must be of type App\Models\Person, string given"

Steps To Reproduce

Example repository: https://github.com/colincameron/route-binding-example

Migrate and seed the database

When served using Herd, visit http://test.route-binding-example.test/people and click on one of the names.

This should show the person name from the PersonController::show() method, but instead, the string "test" is passed to the method, causing a type mismatch error.

@colincameron
Copy link
Contributor Author

I've tried to debug this through src/Illuminate/Routing/ControllerDispatcher.php but I'm a bit lost as to what happens in ResolvesRouteDependencies resolveMethodDependencies() and transformDependency(), and why the parameters aren't being mapped correctly in there.

@jimmyaldape
Copy link

#52261 fixes this issue however tests are failing so there is more to consider. will continue tomorrow.

@colincameron
Copy link
Contributor Author

I've added simpler examples to routes/web.php that show the problem.

Route::get('working/{extraparameter}/people/{person}', function(string $extraparameter, Person $person) {
    return $person->name;
});

Route::get('/fails/{extraparameter}/people/{person}', function(Person $person) {
    return $person->name;
});

In these examples I can see why the behaviour may be expected, but when the extra parameter is captured in a route group as in the original example it feels more like a bug.

Perhaps the solution is to re-order parameters to match the function definition. In ResolvesRouteDependencies.php line 49, we are already iterating through the parameters in the correct order using $reflector->getParameters().

@colincameron
Copy link
Contributor Author

For my specific example, where I use the subdomain parameter in my CaptureSubdomainOrganisation middleware, I can use $request->route()->forgetParameter('subdomain');

I'm happy for this to be closed if it's considered not to be a bug.

@driesvints
Copy link
Member

This is the expected behaviour. The subdomain param is always passed to the controller method. You need to add it first here: https://github.com/colincameron/route-binding-example/blob/main/app/Http/Controllers/PersonController.php#L41

@colincameron
Copy link
Contributor Author

Thanks for the clarification. In my case, forgetting the parameter works better as I have dozens of methods that would need an unnecessary parameter added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants