Fixed a bug in binding the URL parameters for API resource routes

[Khaled-Farhat]

This pull request fixes #275. The bug is that Scribe uses the id as the default URL parameter when no inline binding is applied, but the user might be using other column by overriding the getRouteKeyName() method in the model.

Before:

\Camel\Extraction\ExtractedEndpointData@normalizeResourceParamName will apply the inline binding if it exists, otherwise it will use the id as the default URL parameter.

After:

1. Camel\Extraction\ExtractedEndpointData@normalizeResourceParamName will apply the inline binding if it exists, otherwise it will not alter the URL parameter name, as it is not its responsibility to guess the proper name.
2. Then, as the UrlParameters\GetFromLaravelAPI strategy will iterate over the method arguments, it can use the type-hinted variables to call the getRouteKeyName() method in the desired model. Then, it will bind the URL parameter name and set the parameter description.

Note: The current logic if there are neither inline binding nor type-hinted variables is to use the id, but the new logic in that case is to not alter the URL parameters. I updated the tests to use the new logic.

[shalvah]

The problem is that we don't want this. This sued to be the case before, but a number of people pointed out that it was unintuitive. It makes sense from the Laravel perspective, since we know about route model binding, but the reader of the API docs wants to know the value that goes in there (the user ID), not just the model.

[Khaled-Farhat]

I agree that using only the model name is not intuitive, but that is not the case when using type-hinted variables in the controller action.
In the test case you pointed out, there are no type-hinted variables in the controller action, and that is the reason why the URL parameter was user.
But when using type-hinted variables in the controller action, the URL parameter will be user_id (or user_slug if the developer customized the route key name to slug). I think this behavior satisfies both the API docs reader and the developer since he can customize the route key name.

[shalvah]

Ah, I don't agree there.

since he can customize the route key name

Yes, but will he? It would be annoying to ask a developer to go through their routes and add :id for every parameter they use. Especially as Laravel defaults to using the id as the route key, it makes no sense that we can't.

[shalvah]

From your original comment:

The bug is that Scribe uses the id as the default URL parameter when no inline binding is applied, but the user might be using other column by overriding the getRouteKeyName() method in the model.

I think we should focus on this. See if the model has getRouteKeyName() defined or a custom key in the route, and use that. Otherwise, we stick to id. We may just need to move the order of operations around to achieve this.

[Khaled-Farhat]

So you want to stick with the id even for varaibles that are not type-hinted? as the id will be used by default for the type-hinted variables because getRouteKeyName() is defined in Illuminate\Database\Eloquent\Model to return the id.

[Khaled-Farhat]

Sure. The current behavior:

1. If there is an inline binding in the route, use it.
2. Else, if it is a resource route parameter, use the id.
3. Otherwise, keep the same parameter name.

What I will do in my new pull request, is to add a new step before the second step to check if there is a type-hinted variable in the controller action with the same name as the route segment name. This is the same behavior as the laravel implicit binding:

Laravel automatically resolves Eloquent models defined in routes or controller actions whose type-hinted variable names match a route segment name.

---

Case 1: If there is an inline binding in the route.

Route::apiResource('posts', PostController::class)->only('show')->parameters(['posts' => 'post:slug']);
Route::get('posts/{post:slug}/comments', function() {});

Current behavior: use inline binding. Current URIs: posts/{slug} and posts/{post_slug}/comments.
The new behavior is the same.

Case 2: No inline binding exists, but model binding exists.

Assume that Post@getRouteKeyName returns slug, and PostController@show uses model binding (Post $post).

Route::apiResource('posts', PostController::class)->only('show').
Route::get('posts/{post}/comments', function(Post $post) {});

Current behavior: use the id for resource parameters, and keep the same name for non-resource parameters.
Current URIs: posts/{id} and posts/{post}/comments.
New behavior: call getRouteKeyName().
New URIs: posts/{slug} and posts/{post_slug}/comments.

Case 3: Resource route parameter. No inline binding exists. No model binding exists.

Assume that PostController@show does not use model binding.

Route::apiResource('posts', PostController::class)->only('show');

Current behavior: use the id. Current URI: posts/{id}.
The new behavior is the same.

Case 4: Non-Resource route. No inline binding exists. No model binding exists.

Route::get('posts/{post}/commets', function($postId) {});

Current behavior: keep the same parameter name. Current URI: posts/{post}.
The new behavior is the same.

[Khaled-Farhat]

I have another question about the testing, Is there a problem if I added 3 fixtures to test the new behavior?

The fixtures I want to add:

• TestPost model that overrides getRouteKeyName() to return slug.
• TestPostController with one method that has a type-hinted variable (TestPost $post). I will use it to test generating the URI posts/{slug}.
• TestPostUserController with one method that uses two type-hinted variables (TestPost $post, TestUser $user). I will use it to test generating the URI posts/{post_slug}/users/{id} from a nested resource route.

[shalvah]

Yes, that sounds good. We probably need to clean up the code to make it less confusing, perhaps with some inline documentation of the examples. Also, I think the responsibility is half-shared; IIRC, there's somewhere else in the code where the URI is "fixed" slightly, which may change the results from here.

[shalvah]

Yeah, you can add whatever you need.

[shalvah]

Oh, I just noticed your PR edits both locations (GetFromLaravelAPI and ExtractedEndpointData), which is good. I wish there was a way for that logic to remain in one place (ideally the GetFromLaravelAPI strategy), but the URL is one of the basic things about the endpoint, so it needs to be fixed as early as possible. Plus, even though they can, I'd prefer to avoid modifying the endpoint object directly inside a strategy.