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

[7.x] Accept field for Route binding #30471

Merged
merged 1 commit into from
Nov 22, 2019
Merged

[7.x] Accept field for Route binding #30471

merged 1 commit into from
Nov 22, 2019

Conversation

browner12
Copy link
Contributor

Scenario

Assume we have a Model called App\Page with the following table structure.

  • id
  • title
  • slug

When accessing it in a user facing Route via implicit binding (example.com/my-page-slug) we want to query the Model via the slug property, but when accessing it from an administration facing Route via implicit binding (example.com/admin/pages/1/edit) we want to query off the default id property.

We currently have the ability to change which field the Model uses to query off of by overriding the getRouteKeyName() method. Unfortunately, this is global and does not allow us to do it selectively.

What I would propose is we can override the query field via the Route definition for Route localized needs.

Route::resource('admin.pages, 'AdminPageController');

Route::get('{page}', 'PageController@show')->by(['page' => 'slug']);

Proposed Solution

This is part 1 of a 2 part solution. This PR allows us pass an override $field to the resolveRouteBinding() method. The default will be null so existing calls will not be affected, and the fallback for the parameter will be the default getRouteKeyName().

Part 2 will require passing the override values in from the Route definition. My first thought would be to add these as more "actions", but I'd be open to other suggestions. Also open to naming, since I don't have any good ones yet.

Master

PRing to master since there are Contract changes.

when resolving the binding for a Route, allow passing an override parameter for the queries field name, instead of the default one on the Model.
@devcircus
Copy link
Contributor

Not necessarily against the changes but it seems like this goes against the nature of "implicit" binding, however, laravel already allows you to explicitly define route model binding via a service provider, which would probably work well in this situation.

@browner12
Copy link
Contributor Author

I guess there's different levels of implicit. This lies somewhere in-between.

correct, that it the solution I currently use where I designate a custom route slug that has its own logic in the RouteServiceProvider.

Route::get('{slug}', 'PageController@show');
public function boot()
{
    parent::boot();

    Route::bind('slug', function ($value) {
        return \App\Page::where('slug', $value)->first() ?? null;
    });
}

It's just a little more verbose of a solution, so was hoping we could build something a little more terse.

@GrahamCampbell GrahamCampbell changed the title [7.x] accept field for Route binding [7.x] Accept field for Route binding Oct 31, 2019
@BrandonSurowiec
Copy link
Contributor

BrandonSurowiec commented Oct 31, 2019

There was a previous proposal (#18197) for syntax like:

Route::get('{page:slug}', 'PageController@show');

But I like your solution just as much.

@taylorotwell
Copy link
Member

I wouldn't mind revisiting that syntax @BrandonSurowiec. I think it is pretty clean.

@browner12
Copy link
Contributor Author

yah, that's not bad. I'm open to other syntaxes if we can get this to work.

@taylorotwell
Copy link
Member

I like the syntax in #18197 best, I think at the time I just thought that PR seemed to touch quite a few files to make it work.

@taylorotwell
Copy link
Member

@browner12 do you want to try that other syntax in the PR I linked or do you want to move on from this PR?

@browner12
Copy link
Contributor Author

sure, let me give it a shot this week.

@@ -21,8 +21,9 @@ public function getRouteKeyName();
/**
* Retrieve the model for a bound value.
*
* @param mixed $value
* @param mixed $value
* @param string $field
Copy link
Member

Choose a reason for hiding this comment

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

string|null

@@ -1471,12 +1471,13 @@ public function getRouteKeyName()
/**
* Retrieve the model for a bound value.
*
* @param mixed $value
* @param mixed $value
* @param string $field
Copy link
Contributor

Choose a reason for hiding this comment

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

string|null

@@ -32,12 +32,13 @@ public function getRouteKeyName()
/**
* Retrieve the model for a bound value.
*
* @param mixed $value
* @param mixed $value
* @param string $field
Copy link
Contributor

Choose a reason for hiding this comment

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

string|null

@taylorotwell taylorotwell merged commit e1f8eef into laravel:master Nov 22, 2019
@taylorotwell
Copy link
Member

I've written the other half of this. Will link them together now.

@browner12
Copy link
Contributor Author

sorry for the delay, things got busy at work. thanks for taking charge on the other part of this.

@browner12 browner12 deleted the route-binding branch November 22, 2019 18:48
@Daniel-Mendes
Copy link

And if I want to use an Accessor like getDateAttribute, that isn't a column of my database, will it work ?

@deleugpn
Copy link
Contributor

I knew I wasn't crazy and I had seen this PR in the past, but it was hard to find it, so I'm just adding a few keywords here to try and make it easier to search GitHub

keywords: Route Binding, Substitute Bindings, Route Middleware, Custom Route Binding.

@sailingdeveloper
Copy link
Contributor

Haven't used Laravel in a while and picking it back up now, it's really nice to see that the idea I proposed in #18197 was actually implemented, thanks @browner12 and @taylorotwell!

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.

10 participants