[5.5] Route model binding with key in route definition #18197

sailingdeveloper · 2017-03-03T16:33:56Z

This allows for the following:

Route::get('events/{event:slug}', 'EventController@show')->name('event.show'); // Finds Event by slug.
Route::get('admin/events/{event}', 'Admin\EventController@show')->name('admin.event.show'); // Finds Event by ID (or whatever is defined in getRouteKeyName()).

route('event.show', $event) -> http://events.dev/events/laracon-2016
route('admin.event.show', $event) -> http://events.dev/events/142

I realise that this is far from pretty or the ideal implementation, but I am very open to suggestions/edits from the community!

devcircus · 2017-03-03T20:51:48Z

I like this. Definitely needs some tests though.

sailingdeveloper · 2017-03-03T21:47:06Z

I will definitely do that as soon as possible

sailingdeveloper · 2017-03-04T14:39:11Z

Thinking I might create a dedicated RouteParameter class to tidy up the explode(':') and other things. Good idea, or not?

lucasmichot · 2017-03-06T18:32:56Z

src/Illuminate/Routing/RouteParameterBinder.php

-            return is_string($value) && strlen($value) > 0;
-        });
+        foreach ($parameterNames as $value => $name) {
+            $name_without_key = preg_replace('/:\w+/', null, $name);


Use camel-case variable instead: $nameWithoutKey

lucasmichot · 2017-03-06T18:33:48Z

src/Illuminate/Routing/UrlGenerator.php

+            if (isset($routeParameters[$index])) {
+                $key = $routeParameters[$index];
+            }
+            $parts = explode(':', isset($key) ? $key : $index);


It's looking like this code is duplicated...

https://github.com/sailingdeveloper/framework/blob/4320e80dcfcd1be5082a4f0f4c2071ceee415e0b/src/Illuminate/Routing/ImplicitRouteBinding.php#L21-L23

lucasmichot · 2017-03-06T18:37:35Z

src/Illuminate/Routing/UrlGenerator.php

            if ($parameter instanceof UrlRoutable) {
-                $parameters[$key] = $parameter->getRouteKey();
+                if ($parameter instanceof Model) {


This would imply to require illuminate/database to illuminate/routing, I am not sure this is worth it.
For now only \Illuminate\Database\Eloquent\Model implements \Illuminate\Contracts\Routing\UrlRoutable.
Why not just checking that getAttribute method key exists ?

I agree and I changed it, but for the record: it's also being used here: https://github.com/laravel/framework/blob/5.4/src/Illuminate/Routing/ImplicitRouteBinding.php#L5
illuminate/database is not in the composer.json though...

lucasmichot · 2017-03-06T18:41:39Z

Thinking I might create a dedicated RouteParameter class to tidy up the explode(':') and other things. Good idea, or not?

I would go for it

sailingdeveloper · 2017-03-07T14:36:18Z

@lucasmichot Done, what do you think?

lucasmichot · 2017-03-07T14:54:31Z

@lucasmichot Done, what do you think?

👍

This would imply to require illuminate/database to illuminate/routing, I am not sure this is worth it.

Damn, did not see that. I suppose you can thus check that $parameter is an instanceof of \Illuminate\Database\Eloquent\Model then...
You should probably add "illuminate/database": "5.4.*" to src/Illuminate/Routing/composer.json, it has probably been forgotten

Remove return type for `parameter()` method

lucasmichot · 2017-03-08T18:43:05Z

src/Illuminate/Routing/RouteParameter.php

+     * RouteParameter constructor.
+     *
+     * @param  string $parameter
+     * @param  mixed  $value


Missing @return void

lucasmichot · 2017-03-08T18:45:04Z

src/Illuminate/Routing/UrlGenerator.php

@@ -359,15 +360,22 @@ protected function formatAction($action)
     * Format the array of URL parameters.
     *
     * @param  mixed|array  $parameters
+     * @param  Route        $route


@param \Illuminate\Routing\Route $route

lucasmichot · 2017-03-15T16:27:01Z

src/Illuminate/Routing/Route.php

@@ -426,6 +426,18 @@ public function signatureParameters($subClass = null)
    }

    /**
+     * Get the parameters as instances of RouteParameter.
+     *
+     * @return RouteParameter[]


We usually use @return array notation

lucasmichot · 2017-03-15T16:27:54Z

src/Illuminate/Routing/RouteParameter.php

+     */
+    protected function parts()
+    {
+        return explode(':', $this->parameter);


return explode(':', $this->parameter, 2); maybe?

It doesn't really matter, but added anyway.

lucasmichot · 2017-03-15T16:28:54Z

tests/Routing/RoutingRouteTest.php

@@ -1495,6 +1516,10 @@ class RoutingTestExtendedUserModel extends RoutingTestUserModel
 {
 }

+class RoutingTestWithRouteKey extends RoutingTestUserModel
+{


we use an empty comment for empty classes, as for RoutableModelStub

lucasmichot · 2017-03-15T16:31:07Z

tests/Routing/RouteParameterTest.php

+    public function testParsingName()
+    {
+        $parameter = new RouteParameter('baz');
+        $this->assertEquals('baz', $parameter->name());


I suppose al assertEquals comparing a string can be replace by stricter assertSame assertions ?

If you say so 😄
Changed.

…/framework into route-model-binding-key

lucasmichot · 2017-03-17T06:46:36Z

A conflict needs to be solved @sailingdeveloper

sailingdeveloper · 2017-03-17T06:50:47Z

@lucasmichot I don't see it, can you give me a line number?

lucasmichot · 2017-03-17T06:56:07Z

Line 38

sailingdeveloper · 2017-03-17T07:19:39Z

Either I'm blind, or we're not looking at the same file(s). Can you tell me what the conflict is, by screenshot or some other way?

lucasmichot · 2017-03-17T07:21:58Z

In src/Illuminate/Routing/ImplicitRouteBinding.php

lucasmichot · 2017-03-17T06:47:30Z

src/Illuminate/Routing/ImplicitRouteBinding.php

-                ! $route->parameter($parameter->name) instanceof Model) {
-                $model = $container->make($class->name);
+            foreach ($routeParameters as $routeParameter) {
+                if ($routeParameter->name() == $parameter->name &&


Why not using a stricter comparison === ?

taylorotwell · 2017-03-23T16:00:56Z

I think I'm going to leave this to manual use of bind for now.

sailingdeveloper · 2017-03-23T16:07:46Z

That's a bummer. Too bad

mokhosh · 2018-08-06T09:23:41Z

Any news on this feature?

sandervankasteel · 2018-09-17T20:31:58Z

This would have been so sweet to have in Laravel

ZacharyDuBois · 2018-10-10T21:31:11Z

Would really like to see this implemented. Adds a really elegant way for slugs to be used on a user facing interface and IDs for an admin facing interface. It also would allow the admin interface to change a slug without the need to redirect to the new slug on form submit.

Thijs de Maa added 2 commits March 3, 2017 17:33

Route model binding specifying key in route definition

62847da

Style CI Fixes

eed4dbf

sailingdeveloper mentioned this pull request Mar 3, 2017

Route model binding with key in route definition #18196

Closed

GrahamCampbell changed the title ~~Route model binding with key in route definition~~ [5.5] Route model binding with key in route definition Mar 3, 2017

sailingdeveloper added 2 commits March 4, 2017 15:20

Tests

7154661

Style CI Fixes

4320e80

lucasmichot reviewed Mar 6, 2017

View reviewed changes

sailingdeveloper and others added 3 commits March 7, 2017 15:15

RouteParameter class.

88ec85b

Style CI Fixes

d262a98

Style CI Fixes

03821e2

Thijs de Maa and others added 2 commits March 7, 2017 15:57

Define illuminate/database as a dependency for illuminate/routing

769e522

Update RouteParameter.php

f5d5a88

Remove return type for `parameter()` method

lucasmichot reviewed Mar 15, 2017

View reviewed changes

Thijs de Maa added 2 commits March 15, 2017 19:27

Review by @lucasmichot

4f3a8fe

Merge branch 'route-model-binding-key' of github.com:sailingdeveloper…

0f55791

…/framework into route-model-binding-key

lucasmichot reviewed Mar 17, 2017

View reviewed changes

taylorotwell closed this Mar 23, 2017

BrandonSurowiec mentioned this pull request Oct 31, 2019

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

Merged

[5.5] Route model binding with key in route definition #18197

[5.5] Route model binding with key in route definition #18197

Conversation

sailingdeveloper commented Mar 3, 2017

devcircus commented Mar 3, 2017

sailingdeveloper commented Mar 3, 2017

sailingdeveloper commented Mar 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasmichot commented Mar 6, 2017

sailingdeveloper commented Mar 7, 2017

lucasmichot commented Mar 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasmichot commented Mar 17, 2017

sailingdeveloper commented Mar 17, 2017

lucasmichot commented Mar 17, 2017 • edited

sailingdeveloper commented Mar 17, 2017

lucasmichot commented Mar 17, 2017 • edited

Choose a reason for hiding this comment

taylorotwell commented Mar 23, 2017

sailingdeveloper commented Mar 23, 2017

mokhosh commented Aug 6, 2018

sandervankasteel commented Sep 17, 2018

ZacharyDuBois commented Oct 10, 2018

sailingdeveloper commented Mar 4, 2017 •

edited

lucasmichot commented Mar 7, 2017 •

edited

lucasmichot commented Mar 17, 2017 •

edited

lucasmichot commented Mar 17, 2017 •

edited