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

[5.4] Feature/Bug: Allow ImplicitRouteBinding to match camelized method parameter names #18307

Merged

Conversation

ralphschindler
Copy link
Contributor

Problem:

Route::resource('foo-bar', 'FooBarController')

will not route implicitly bound model parameters to

<?php
namespace App\Http\Controllers;

use App\Models\FooBar;

class FooBarController extends Controller
{
    public function show(FooBar $fooBar)
    {
        // ...
    }
    // ...
}

since ImplicitRouteBinding will not match the auto-generated foo_bar resource parameter name to the controller method name $fooBar.

The provided PR ensures this will work in a non-breaking way (original explicit matching remains priority matching, and only in the case of non-matching exact will the snake_case version of a method name be attempted to be matched.

(I feel this could qualify as a bug since my expectation is that camelized method parameter naming should work and Route::resource() doesn't allow for hinting a proper parameter name.)

@ralphschindler ralphschindler changed the title Feature/Bug: Allow ImplicitRouteBinding to match camelized method names Feature/Bug: Allow ImplicitRouteBinding to match camelized method parameter names Mar 12, 2017
@ralphschindler ralphschindler changed the base branch from 5.4 to master March 12, 2017 17:33
- allow ImplicitRouteBinding to match compound name ('foo_bar') route parameters to method/function naming ('fooBar')
@ralphschindler ralphschindler changed the base branch from master to 5.4 March 12, 2017 18:30
@ralphschindler ralphschindler force-pushed the fix/resource-routing-parameter-names branch from 0892287 to 6f7d60c Compare March 12, 2017 18:32
@ralphschindler ralphschindler changed the title Feature/Bug: Allow ImplicitRouteBinding to match camelized method parameter names [5.4] Feature/Bug: Allow ImplicitRouteBinding to match camelized method parameter names Mar 13, 2017
@taylorotwell
Copy link
Member

I agree this should be fixed. Will look this over, thanks.

@ralphschindler
Copy link
Contributor Author

Thanks @taylorotwell. As I thought through the problem, this felt like the most appropriate area to solve the problem, to me at the time.

Should you require any discussion on it, I am available inside #internals & #laravel5 in larachat.

@lagbox
Copy link
Contributor

lagbox commented Mar 16, 2017

What do you mean by "Route::resource() doesn't allow for hinting a proper parameter name"?
Im not sure why we would expect 'foo_bar' to match 'fooBar'.

@taylorotwell taylorotwell merged commit a51a6d1 into laravel:5.4 Mar 16, 2017
@lagbox
Copy link
Contributor

lagbox commented Mar 16, 2017

Not that someone should have been doing this, but this is broken now:
tags/{tag_id} => function (SomeModel $tagId, Tag $tag_id)
Will not resolve now, but before you would have had a new instance of SomeModel as $tagId and $tag_id would be the actual Tag. Its possible someone could be using dep injection to get an instance of a model.

No one is expecting a binding match when the parameter names aren't an exact match. Contrived example or not, what was allowed before and worked, doesn't now.

I'm just a worrier :-).

@ralphschindler ralphschindler deleted the fix/resource-routing-parameter-names branch March 16, 2017 16:32
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.

3 participants