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

[8.x] Enforce implicit Route Model scoping #39440

Merged
merged 3 commits into from
Nov 4, 2021
Merged

[8.x] Enforce implicit Route Model scoping #39440

merged 3 commits into from
Nov 4, 2021

Conversation

claudiodekker
Copy link
Contributor

This PR implements the functionality requested in #32144.

With this PR, any Route (or route group) with the scoping action set to true will tell the framework to scope the second Eloquent model as such that it must be a child of the previous Eloquent model in the same route, without the need to provide a slug.

use App\Models\Post;
use App\Models\User;

Route::group(['scoping' => true], function () {
    Route::get('/users/{user}/posts/{post}', function (User $user, Post $post) {
        return $post;
    });
});

This is useful when:

  • You always want this behaviour everywhere, or in a subset of routes (extra / automatic safety net)
  • You don't want to look silly by specifying the id slug everywhere just to enable this behaviour
    (Route::get('/users/{user}/posts/{post:id}', function (User ...)

While by itself this is only a somewhat useful feature, I think it would be quite useful when also providing the ability to enable it application-wide using a class-level property in the RouteServiceProvider, essentially making it very easy to either opt-in or opt-out, and to what extent:

    /**
     * Define the routes for the application.
     *
     * @param  \Illuminate\Routing\Router $router
     * @return void
     */
    public function map(Router $router)
    {
        $router->group([
            'namespace' => $this->namespace,
            'middleware' => 'web',
            'scoping' => $this->scoping,            // <----------------
        ], function ($router) {
            require base_path('routes/web.php');
        });

        $router->group([
            'middleware' => ['api'],
            'namespace' => 'App\Http\ApiControllers',
            'scoping' => $this->scoping,
            'prefix' => 'api/v1',
        ], function ($router) {
            require base_path('routes/api.php');
        });
    }
}

@GrahamCampbell GrahamCampbell changed the title Enforce implicit Route Model scoping [8.x] Enforce implicit Route Model scoping Nov 1, 2021
@claudiodekker claudiodekker marked this pull request as draft November 2, 2021 10:39
@claudiodekker claudiodekker marked this pull request as ready for review November 2, 2021 11:45
@bastien-phi
Copy link
Contributor

Actually, you don't need to give the model route key in order to scope the post to the given user

https://github.com/laravel/framework/blob/8.x/src/Illuminate/Database/Eloquent/Model.php#L1933

You can just define your parameter with an extra :

Route::get('/users/{user}/posts/{post:}', function (User $user, Post $post) {
    // $post belongs to $user and route parameter is the default one
    return $post;
});

Maybe it just deserves to be documented ;-)

@claudiodekker
Copy link
Contributor Author

claudiodekker commented Nov 3, 2021

Not sure if trolling, @bastien-phi

That's how it works right now, which is documented. You are providing a key, but because that key is an empty string, it falls back to the Eloquent model key (likely "id") and uses that instead.

Furthermore, this still feels like a hacky way to enable this feature's behaviour. This PR enables the possibility to enable it application wide by default using a feature flag.

@bastien-phi
Copy link
Contributor

@claudiodekker

I was just saying that passing {post:} without slug will fallback to the model route key (not the model key) and this behavior is not documented. (solving You don't want to look silly by specifying the id slug everywhere just to enable this behaviour)

This PR enables the possibility to enable it application wide by default using a feature flag.

Indeed, not possible for now. Good job !

@taylorotwell
Copy link
Member

taylorotwell commented Nov 4, 2021

Made a few tweaks here.

You can now do:

Route::get('/posts/{post}/comments/{comment}', function (Post $post, Comment $comment) {
    return $comment;
})->scopeBindings();

Or:

Route::scopeBindings()->group(function () {
    Route::get('/posts/{post}/comments/{comment}', function (Post $post, Comment $comment) {
        return $comment;
    });
});

@taylorotwell taylorotwell merged commit d1093f5 into laravel:8.x Nov 4, 2021
@claudiodekker claudiodekker deleted the enforce-scoping branch November 4, 2021 21:03
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.

4 participants