tighten / ziggy

Use your Laravel routes in JavaScript.
MIT License
3.83k stars 247 forks source link

Model binding through custom route key (getRouteKeyName) only works on first level of url #707

Closed pixelreverb closed 6 months ago

pixelreverb commented 7 months ago

Ziggy version

v1.8.1

Laravel version

v10.39.0

Description

I skimmed through the other issues and although some sound similar to my use case, I did not find an exact match. If there already exists an issue which addresses my topic, please feel free to just link to it and close this issue. Thanks.

Description

When using a multi-level route setup only the binding for the first model will be resolved by evaluating the getRouteKeyName. Other models will fall back to using the id even if getRouteKeyName is redefined within the model.

Also, it seems to be an issue when a single resource is accessed via two named routes that partly contain the same name.

Explanation

Let's assume the following setup:

There are two models organization and registration_token. Both of them attempt to overwrite the default id with a custom value through getRouteKeyName.

Also, the registration_tokens can be accessed in two ways. Namely, globally by directly accessing /registration_tokens/{registration_tokens} and by being bound to an organization like /organizations/{organization}/registration_tokens/{registration_token}. The targeted controller is the same for both approaches and takes care of the presence of an organization.

The routes in this example are defined as resources via Route::resource('...'), but I assume that the issue also applies when routes are defined in a group.

Organization model

class Organization extends Model
{
    use HasFactory;

    // ...

    public function getRouteKeyName(): string
    {
        return 'slug';
    }

    // ...
}

RegistrationToken model

class RegistrationToken extends Model
{
    use HasFactory;

    // ...

    public function getRouteKeyName(): string
    {
        return 'token';
    }

    // ...
}

Route configuration

Route::resource('organizations.registration_tokens', RegistrationTokenController::class)
    ->only(['index', 'create', 'store', 'show', 'destroy'])
    ->middleware(['verified']);

Route::resource('registration_tokens', RegistrationTokenController::class)
    ->only(['index', 'create', 'store', 'show', 'destroy'])
    ->middleware(['verified', 'role:system_admin']);

From the above configuration I would expect to access a registration token in the following two ways:

Unfortunately, this is not possible. The custom route key name for an organization will be resolved, but in both cases the custom route key of a registration token model is not expected by the generated routes.

And when used with InertiaJS/React the custom route key name of an organization is properly placed. Whereas the registration token will use the id.

The Ziggy generated routes for the above scenario are ziggy.js

// ...
'organizations.registration_tokens.show': {
    uri: 'organizations/{organization}/registration_tokens/{registration_token}',
    methods: ['GET', 'HEAD'],
    parameters: ['organization', 'registration_token'],
    bindings: { organization: 'slug' },
},
// ...
'registration_tokens.show': {
    uri: 'registration_tokens/{registration_token}',
    methods: ['GET', 'HEAD'],
    parameters: ['registration_token'],
},
// ...

Assumptions/Ideas

Having a glance at the Ziggy.php file, I assume that there would need to be some sort of recursive evaluation to travel multi-level urls in resolveBindings().

Further, I assume that currently already present bindings will be replaced when a route with the same binding parameter is evaluated a second time, registration_token in my case.

Please, correct me if I'm wrong and thanks for your help.

Ziggy call and context

Ziggy is used as part of a Laravel application in combination with InertiaJS and React.
The issue appears when Ziggy generates the routes and tries to resolve the bindings.

Ziggy configuration

// ...
'organizations.registration_tokens.show': {
    uri: 'organizations/{organization}/registration_tokens/{registration_token}',
    methods: ['GET', 'HEAD'],
    parameters: ['organization', 'registration_token'],
    bindings: { organization: 'slug' },
},
// ...
'registration_tokens.show': {
    uri: 'registration_tokens/{registration_token}',
    methods: ['GET', 'HEAD'],
    parameters: ['registration_token'],
},
// ...

Route definition

Route::resource('organizations.registration_tokens', RegistrationTokenController::class)
    ->only(['index', 'create', 'store', 'show', 'destroy'])
    ->middleware(['verified']);

Route::resource('registration_tokens', RegistrationTokenController::class)
    ->only(['index', 'create', 'store', 'show', 'destroy'])
    ->middleware(['verified', 'role:system_admin']);
bakerkretzmar commented 6 months ago

What is the specific error you're encountering here? Is there an exception being thrown anywhere or is the issue just that you don't see the binding in Ziggy's output? Do the routes themselves work as expected in your app?

This looks like an issue with the definition of the route or model, not with Ziggy, and I added a test for this particular setup locally and it passes (will PR it tomorrow). Can you share more of the real code where you're having this issue in your app, and/or a minimal reproduction? Thanks.

pixelreverb commented 6 months ago

I use a Laravel project together with React and InertiaJS. The configuration of the project is pretty much as it is after a fresh install.

The specific problem I encounter is that the binding is missing, which according to the above mentioned definitions should be present.

I would expect the generate configuration to look something like:

'organizations.registration_tokens.show': {
    uri: 'organizations/{organization}/registration_tokens/{registration_token}',
    methods: ['GET', 'HEAD'],
    parameters: ['organization', 'registration_token'],
    bindings: { organization: 'slug', registration_token: 'token' },  // <- 'registration_token' is missing for me
},
// ...
'registration_tokens.show': {
    uri: 'registration_tokens/{registration_token}',
    methods: ['GET', 'HEAD'],
    parameters: ['registration_token'],
    bindings: { registration_token: 'token' },  // <- complete binding is missing for me
},

The routes themselves work when I would rely on the id. There is no error or exception thrown.

I would like to access the routes in the form of

When the ziggy configuration is used from React/InertiaJS side, it always resolves this url to

Which is completely understandable as id is the fallback/default and the bindings are not generated.

Somehow the return value of getRouteKeyName for the RegistrationToken model is ignored.


What I found out is that I can get the binding to token at least for the route GET /registration_tokens/{registration_token} when I define the route as

Route::middleware(['verified', 'role:system_admin'])->group(function () {
    Route::get('/registration_tokens/{registration_token:token}', [RegistrationTokenController::class, 'show'])->name('registration_tokens.show');
    // ...
}

Which generates

'registration_tokens.show': {
    uri: 'registration_tokens/{registration_token}',
    methods: ['GET', 'HEAD'],
    parameters: ['registration_token'],
    bindings: { registration_token: 'token' }, 
},

But I assume that in this case the binding is taken from the route definition and not by evaluating getRouteKeyName. I really would like to continue using Route::resource(...) and not need to define each route separately.

I hope this helps. I will checkout your test and setup a minimal working example on the weekend. Thanks.

pixelreverb commented 6 months ago

As promised, you can have a look at a minimal working example here: mwe-ziggy-model-binding. It also contains my last set of generated routes at resources/js/ziggy.js. Thanks.

bakerkretzmar commented 6 months ago

Thanks a lot for the reproduction! Here's a PR that fixes your issue: https://github.com/pixelreverb/mwe-ziggy-model-binding/pull/1.

Your route parameters and controller method arguments must be exactly the same for route model binding to work, down to casing and spelling; your route parameter is registration_token but your controller argument is $registrationToken so they aren't recognized as being related to each other (that's how that Laravel feature works, it has nothing to do with Ziggy).

Update your organizations.registration_tokens route definition to organizations.registrationTokens (or update your RegistrationTokenController method arguments to $registration_token) and your example should work fine—but just for the nested resource.

Your registrationTokens resource route needs its own separate controller, because route model binding also relies on the position of the route parameters and controller arguments, so those routes won't be bound correctly because the first controller method argument ($organization) doesn't correspond to the first route parameter (registration_token).

Let me know if any of that isn't clear!

pixelreverb commented 6 months ago

Thanks for your reply. Your suggestion actually did the trick.

I think I was fooled by the fact the application is working because of the fallback to the id property. I know this because the app contains other multi-word models (like RegistrationToken) that - as I know now - are not properly declared and works just fine, including model binding.