tighten / ziggy

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

Ziggy generating wrong params for resources with param url #764

Closed CapitaineToinon closed 1 month ago

CapitaineToinon commented 1 month ago

Ziggy version

2.2.1

Laravel version

11.13.0

Description

It seems like the ziggy generated files asks for wrong param types for my resource routes. I have one "Activity" resources can be of variable type ("Webinar", "Seminar", etc) and I mapped my resource controller to a custom path to automatially filter my activities. The pages work just fine from the Laravel side of thing but the JS client can't generate proper urls because it asks me for wrong params.

Ziggy call and context

console.log(
    route('activities.show', {
        id: 1,
        type: 'webinars',
    }),
)

Ziggy configuration

{
    url: 'http://localhost',
    port: null,
    defaults: {},
    routes: {
        'filament.exports.download': {
            uri: 'filament/exports/{export}/download',
            methods: ['GET', 'HEAD'],
            parameters: ['export'],
            bindings: { export: 'id' },
        },
        'filament.imports.failed-rows.download': {
            uri: 'filament/imports/{import}/failed-rows/download',
            methods: ['GET', 'HEAD'],
            parameters: ['import'],
            bindings: { import: 'id' },
        },
        'filament.admin.auth.login': {
            uri: 'admin/login',
            methods: ['GET', 'HEAD'],
        },
        'filament.admin.auth.logout': {
            uri: 'admin/logout',
            methods: ['POST'],
        },
        'filament.admin.pages.dashboard': {
            uri: 'admin',
            methods: ['GET', 'HEAD'],
        },
        'filament.admin.resources.activities.index': {
            uri: 'admin/activities',
            methods: ['GET', 'HEAD'],
        },
        'filament.admin.resources.activities.create': {
            uri: 'admin/activities/create',
            methods: ['GET', 'HEAD'],
        },
        'filament.admin.resources.activities.edit': {
            uri: 'admin/activities/{record}/edit',
            methods: ['GET', 'HEAD'],
            parameters: ['record'],
        },
        'sanctum.csrf-cookie': {
            uri: 'sanctum/csrf-cookie',
            methods: ['GET', 'HEAD'],
        },
        'livewire.update': { uri: 'livewire/update', methods: ['POST'] },
        'livewire.upload-file': {
            uri: 'livewire/upload-file',
            methods: ['POST'],
        },
        'livewire.preview-file': {
            uri: 'livewire/preview-file/{filename}',
            methods: ['GET', 'HEAD'],
            parameters: ['filename'],
        },
        home: { uri: '/', methods: ['GET', 'HEAD'] },
        activities: { uri: 'activities', methods: ['GET', 'HEAD'] },
        'contact-us.index': { uri: 'contact-us', methods: ['GET', 'HEAD'] },
        'activities.index': {
            uri: '{type}',
            methods: ['GET', 'HEAD'],
            parameters: ['type'],
        },
        'activities.show': {
            uri: '{type}/{{type}}',
            methods: ['GET', 'HEAD'],
            parameters: ['type', '{type'],
        },
    },
}

Route definition

Route::resource('/{type}', ActivityController::class)
    ->only('index', 'show')
    ->names([
        'index' => 'activities.index',
        'show' => 'activities.show',
    ]);
bakerkretzmar commented 1 month ago

What are the method signatures of the index and show methods in that controller?

Is that route definition wrapped inside anything? Or is there anything in your route service provider that could be prefixing/wrapping it?

CapitaineToinon commented 1 month ago

It wasn't wrapped in anything. It seems like the problem is caused by Route::resource which tries to singularize the last word of the path to generate the param names, Which is quite odd with dynamic routes like I have I think.

https://laravel.com/docs/11.x/controllers#restful-naming-resource-route-parameters

My show method uses an enum to cast to the param and automatically throw a 404 when the path isn't correct, it's quite neat:

    public function show(ActivityTypeParam $type, Activity $activity)
    {
        return Inertia::render('activities/show', [
            'type' => $type,
            'activity' => $activity,
        ]);
    }

So yeah I fixed it by renaming the param using the parameters method on the Route class but it feels like a hack honestly.

Route::prefix('/activities')->name('activities.')->group(function () {
    // extra route that just lists all the activities regardless of the type
    Route::get('/', [ActivityController::class, 'all'])
        ->name('all');

    Route::resource('/{type}', ActivityController::class)
        ->only('index', 'show')
        ->parameters([
            // this is the fix 👍
            '{type}' => 'activity',
        ])
        ->names([
            'index' => 'index',
            'show' => 'show',
        ]);
});

And now my routes are correct and work with ziggy as well:

  GET|HEAD   activities ................................ activities.all › ActivityController@all
  GET|HEAD   activities/{type} ..................... activities.index › ActivityController@index
  GET|HEAD   activities/{type}/{activity} ............ activities.show › ActivityController@show

ziggy.js

        'activities.index': {
            uri: 'activities/{type}',
            methods: ['GET', 'HEAD'],
            parameters: ['type'],
        },
        'activities.show': {
            uri: 'activities/{type}/{activity}',
            methods: ['GET', 'HEAD'],
            parameters: ['type', 'activity'],
            bindings: { activity: 'id' },
        },

However I still believe ziggy generates something wrong in the example I provided originally. Although to be fair, Laravel doesn't scream at you at your routes are incorrect until you try to access the page. However maybe ziggy could just fail to generate when the routes are invalid or something like that.

Not sure, up to you what you wanna do.

CapitaineToinon commented 1 month ago

So to be more precise, the old code generate routes like these in Laravel:

  GET|HEAD   / ........................................................ home › LandingController
  GET|HEAD   activities/{type} ......................... {type}.index › ActivityController@index
  GET|HEAD   activities/{type}/{{type}} .................. {type}.show › ActivityController@show

And then when you access something like /activities/webinars/1, Laravel just throws because:

image

So yeah, nice edge case.

bakerkretzmar commented 1 month ago

Interesting, thanks for those details. Since this doesn't work in Laravel either I don't think Ziggy should necessarily do anything, although I can see how it's annoying that one might not immediately realize something is broken.

That being said, your resource route definition is still invalid—the first parameter to Route::resource() is not a path with parameters like when you define individual routes, it's a resource name that the paths, route names, and parameters will be generated from, so you can't put parameters like {type} in it:

https://github.com/laravel/framework/blob/0e9053da9d709c544200260cc0be5e223ea8ff93/src/Illuminate/Routing/Router.php#L326-L345

The ->parameters(['{type}' => 'activity']) workaround is hacky you're right, {type} is still not actually a route parameter, you've just created a resource with the literal name {type} and then renamed it to activity.

What Laravel wants you to do is this:

Route::prefix('/activities')->name('activities.')->group(function () {
    Route::prefix('{type}')->group(function () {
        Route::resource('activities', ActivityController::class)
            ->only('index', 'show');
    });
});

And that generates these routes (which I realize is not what you're going for):

/activities/{type}/activities
/activities/{type}/activities/{activity}

What you want, which is a resource route that doesn't prefix the generated paths with the name of the resource, doesn't appear to be supported by Laravel as far as I can tell.

If I were you I would define these two routes individually.

One other workaround for this that I tried, which is even weirder but I think makes it a little clearer what's actually happening:

Route::prefix('/activities')->name('activities.')->group(function () {
    Route::prefix('{type}')->group(function () {
        Route::resource('/', ActivityController::class)
            ->only('index', 'show')
            ->parameter('', 'activity');
    });
});

This basically creates an unnamed resource route, or one where the name is an empty string, and then renames the empty string to activity. This might be an even worse idea and break in other ways, I haven't tested it.

CapitaineToinon commented 1 month ago

Oh okay that makes much more sense. To be honest, I'm quite surprised my example even worked in the first place. I'm only using the index and show methods in my controller since I'm using filament to edit the data anyway so I ended up just writting the routes explicitly.

Route::name('activities.')->group(function () {
    Route::get('/activities', [ActivityController::class, 'all'])->name('all');
    Route::get('/{type}', [ActivityController::class, 'index'])->name('index');
    Route::get('/{type}/{activity}', [ActivityController::class, 'show'])->name('show');
});

And now it works great with ziggy too. Thanks for the help ! 👍