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

Add full support for implicit route model binding #315

Merged
merged 20 commits into from
Sep 4, 2020

Conversation

bakerkretzmar
Copy link
Collaborator

@bakerkretzmar bakerkretzmar commented Aug 14, 2020

This PR adds support for implicit route model binding. It builds on #307, which added support for custom scoped route model bindings defined directly in the route pattern.

When generating Ziggy's list of routes, we'll now resolve all route model bindings and pass them to Ziggy's javascript output. This lays the foundation for making Ziggy's detection of 'model' objects passed in as route parameters way simpler and more powerful. Right now we're sniffing for an id property (or a custom binding key if the parameter is scoped), but this only works for models with the route key id—after this PR, we'll be able to use every model's actual route key name.

Given this model and route...

class User extends Model
{
    public function getRouteKeyName()
    {
        return 'uuid';
    }
}

Route::get('users/{user}', function (User $user) {
    return $user;
})->name('users.show');

...Ziggy's namedRoutes will now contain an entry that looks like this...

{
    // ...
    'users.show': {
        uri: 'users/{user}',
        methods: ['GET', 'HEAD'],
        domain: null,
        bindings: {
            user: 'uuid', // this is new
        },
    }
}

...which means we'll eventually be able to pass a JSON user object directly into Ziggy's route() function as a parameter, even though that model doesn't use id as its route key:

route('users.show', user);
// would return something like 'https://ziggy.dev/users/f7e781fe-9d66-4967-a9b3-39ec609b04f2'

This implementation requires reflecting into the parameters of all routes in the application and instantiating an Eloquent model for every bound parameter, which could conceivably have an impact on performance in apps with a lot of routes. This would only become an issue if it happened on every request, so to mitigate that, this PR also changes the @routes Blade directive to output Ziggy's javascript directly instead of just embedding a call to our BladeRouteGenerator. This means that Ziggy's entire output will be cached automatically inside the application's compiled Blade views. Note that this means users will have to run php artisan view:clear after changing any of their routes. I'll add a note about it to the Readme before we release v1.

Huge thanks to @taylorotwell for pointing me in the right direction to get this working!

Closes #308, see also #143 and #301.

@bakerkretzmar bakerkretzmar added this to the v1.0 milestone Aug 14, 2020
@bakerkretzmar
Copy link
Collaborator Author

After some (very naive) load testing of this approach, it looks like the performance hit is significant. It depends on the number of models in the app, not the number of routes—if hundreds of routes bind the same model, that model only gets booted and instantiated once, but if an app has a hundred models, they'll all get booted and instantiated even if they're only bound to one or two routes each, which can add up quickly.

'Caching' Ziggy's output like this PR does works but feels hacky. At the very least we'll want to make sure we're never doing it in local environments.

@bakerkretzmar
Copy link
Collaborator Author

bakerkretzmar commented Sep 4, 2020

I found a way around booting every model, so now we only boot the ones that don't use the default id route key name.

This appears to add about 1-4ms to the response time per model in the app that does use a custom route key and is not already being booted for the current request (i.e. models not bound to the current route).

Update: in addition to this change, replacing the collection mapping with foreach loops increases performance significantly, and caching generated Blade views in production should make this a non-issue anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for custom Eloquent route keys
1 participant