filamentphp / filament

A collection of beautiful full-stack components for Laravel. The perfect starting point for your next app. Using Livewire, Alpine.js and Tailwind CSS.
https://filamentphp.com
MIT License
16.23k stars 2.58k forks source link

TypeError with Closure Parameter in `ClassAction` URL Definition with `assertTableActionHasUrl` Assertion #12321

Closed ousid closed 3 months ago

ousid commented 3 months ago

Package

filament/tables

Package Version

v3.2.66

Laravel Version

^10.0.0 || ^11.0.0

Livewire Version

v3.4.10

PHP Version

PHP 8.2.17

Problem description

When attempting to assert the URL of an edit action in a Livewire test using the assertTableActionHasUrl method, a TypeError is encountered.

The error message indicates that the closure parameter, which should receive a Model instance, is instead receiving null.

This occurs despite providing a hard-coded URL within the closure.

The issue appears to be related to how the closure parameter is handled within the ClassAction::make()->url() method.

Expected behavior

When defining the URL for a ClassAction that extends the Filament\Tables\Actions\Action using the ClassAction::make()->url() method with a closure, the closure should receive an instance of a Model class, as its argument.

This model instance represents the record being edited (in my case) and allows for dynamic URL generation based on the record's attributes.

In this specific scenario, the closure should receive the appropriate Model instance, allowing the hard-coded URL to be correctly evaluated and returned for the action.

Steps to reproduce

->actions([
...
    Tables\Actions\EditAction::make()
        ->url(
            fn (Model $record) => 'https://filamentphp.com'
        ),
...
]),
...
Livewire::test(ListUsers::class)
    ->assertTableActionHasUrl(
        'edit',
        'https://filamentphp.com'
    );
...

Note:

I have provided this scenario out of the box under action-url-assertion-issue branch.

Reproduction repository

https://github.com/ousid/filament-issues/tree/action-url-assertion-issue

Relevant log output

`
TypeError: App\Filament\Resources\UserResource::App\Filament\Resources\{closure}(): Argument #1 ($record) must be of type Illuminate\Database\Eloquent\Model, null given, called in /path/to/project/vendor/filament/support/src/Concerns/EvaluatesClosures.php on line 35
`
dmitry-udod commented 3 months ago

@ousid Hi! Yes, the bug is indeed present, but only in tests. Seems it's happen because you use $record variable name and this is like a reserved word here

But you can avoid this bug to change code from:

->actions([
...
    Tables\Actions\EditAction::make()
        ->url(
            fn (Model $record) => 'https://filamentphp.com'
        ),
...
]),

To

->actions([
...
    Tables\Actions\EditAction::make()
        ->url(
            fn (User $user) => 'https://filamentphp.com'
        ),
...
]),

https://github.com/filamentphp/filament/assets/4639175/3ebc2538-e793-42e8-a935-28bcafca8294

saade commented 3 months ago

But you can avoid this bug to change code from: ...

Changing Model $record to User $user would break the application if the closure needs the record. $record is a named evaluation parameter, if renamed, it would never be resolved, thats why the error is gone.

@ousid, you need to pass the $record as the third parameter to assertTableActionHasUrl()

ousid commented 3 months ago

@ousid Hi! Yes, the bug is indeed present, but only in tests. Seems it's happen because you use $record variable name and this is like a reserved word here

But you can avoid this bug to change code from:

->actions([
...
    Tables\Actions\EditAction::make()
        ->url(
            fn (Model $record) => 'https://filamentphp.com'
        ),
...
]),

To

->actions([
...
    Tables\Actions\EditAction::make()
        ->url(
            fn (User $user) => 'https://filamentphp.com'
        ),
...
]),

Screencast.2024-04-18.14.31.03.mp4

Yes, the error is gone, but the example here, is just for demonstrating the shown error, in the real app, I'm developing, I need to pass the record var, as it is, to get the actual data. It's like something like this:

EditAction::make()
    ->color('success')
    ->url(
        fn (?Article $record) => route('articles.edit', $record?->getSlug())
    ),

PS: I'm using the tables package outside the panel.

ousid commented 3 months ago

@ousid, you need to pass the $record as the third parameter to assertTableActionHasUrl()

You cannot pass the $record instance as a third parameter, because it does not exist

https://github.com/filamentphp/filament/blob/3.x/packages/tables/.stubs.php#L100

saade commented 3 months ago

wtf, i've acidentally edited your comment instead of adding a new one. sorry.

You cannot pass the $record instance as a third parameter, because it does not exist

@ousid I think the function definition is outdated? https://github.com/filamentphp/filament/blob/3.x/packages/tables/src/Testing/TestsActions.php#L575

I may be wrong, i'm not too much into tests, i'm source diving rn to help you. Just wanted to correct the comment from @dmitry-udod

ousid commented 3 months ago

@saade

wtf, i've acidentally edited your comment instead of adding a new one. sorry.

No worries, It happens

@ousid I think the function definition is outdated?

Not sure, will try it out, and let you know. I'm using the latest release, tho.

Update

Yes, you are right. I passed the model, and the test passes.

The only things remain, is why the .stub didn't get updated? Or I'm missing something?

dmitry-udod commented 3 months ago

wtf, i've acidentally edited your comment instead of adding a new one. sorry.

You cannot pass the $record instance as a third parameter, because it does not exist

@ousid I think the function definition is outdated? https://github.com/filamentphp/filament/blob/3.x/packages/tables/src/Testing/TestsActions.php#L575

I may be wrong, i'm not too much into tests, i'm source diving rn to help you. Just wanted to correct the comment from @dmitry-udod

Oh. Thanks for catching that!

saade commented 3 months ago

The only things remain, is why the .stub didn't get updated? Or I'm missing something?

It can happen, maybe a conflicted merge result or something.

ousid commented 3 months ago

The only things remain, is why the .stub didn't get updated? Or I'm missing something?

It can happen, maybe a conflicted merge result or something.

I was thinking about closing this issue (since it's not an issue anymore) But I'll keep it open for this reason, till the maintainers decided how to resolve this one.

I'll be happy to open a PR to update the .stubs file, if you want me to!

Note: Even the docs, missing this part (https://filamentphp.com/docs/3.x/tables/testing#url)

saade commented 3 months ago

Go for it!

danharrin commented 3 months ago

Please open a PR to make the stubs consistent if you wish