rappasoft / laravel-livewire-tables

A dynamic table component for Laravel Livewire
https://rappasoft.com/docs/laravel-livewire-tables/v2/introduction
MIT License
1.75k stars 328 forks source link

[Bug]: Using a User class that does not extend Illuminate\Foundation\Auth\User crashes when a Rappasoft\LaravelLivewireTables\Events\LaravelLivewireTablesEvent is triggered #1949

Open khwadj opened 5 days ago

khwadj commented 5 days ago

What happened?

When your application uses a User class that does not extend Illuminate\Foundation\Auth\User, livewire-datables crashes if you try to show/hide columns.

Not tested but can be implied from the code base: This also happens for any Rappasoft\LaravelLivewireTables\Events\LaravelLivewireTablesEvent

How to reproduce the bug

Setup:

Result: the component crashes with the following error message

Capture d’écran 2024-09-17 à 09 55 51

Expected behavior: Triggering any kind of Event works fine regardless of the User class used by the application

Tested version:

Package Version

3.4.20

PHP Version

8.3.x

Laravel Version

10.48.12

Alpine Version

3.14

Theme

None

Notes

The issue arises from the definition of the LaravelLivewireTablesEvent's $user property.

<?php

namespace Rappasoft\LaravelLivewireTables\Events;

use Illuminate\Foundation\Auth\User;
(...)

class LaravelLivewireTablesEvent
{
    (...)
    public ?User $user;
    (...)

    public function setUserForEvent(): self
    {
        if (auth()->user()) {
            $this->user = auth()->user();
        }

        return $this;
    }

The type of the property may not match the actual answer from auth()->user().

Removing the type of the $user attribute is enough to solve the issue.

On top of that, at the moment of writing, I cannot find any use of the LaravelLivewireTablesEvent::$user property. I may not be fully aware of the Laravel event system, and I Imagine I either didn't look well enough, or it's been done for a future use.

As to why a Laravel application would not use a default User class that extends the default laravel class, I would say this is fairly irrelevant, as each developper is free to accomodate their own needs in the way they desire. Laravel works just fine by entirely replacing their User classes, and so should any laravel library.

Error Message

No response

lrljoe commented 5 days ago

Does your custom User model utilise the "Illuminate\Contracts\Auth\Authenticatable" interface?

It's probably an oversight that Illuminate\Foundation\Auth\User is being used rather than the Authenticatable Interface.

Otherwise, I'll probably need to add in some customisations to swap in ability to use custom Events.

What I want to avoid is a non-typed property as default.

khwadj commented 4 days ago

Thanks for answering, and answering quickly.

Our model indeed implements de Illuminate\Contracts\Auth\Authenticatable Interface. However, the way typing work means the result of auth()->user() must be of the provided class or one of its direct descendent, which is not the case here.

I'm totally on board with typing everything. In this case defining the user property as Illuminate\Contracts\Auth\Authenticatable seems to me to be the better solution. I should have mentioned it in my bug report, I'm sorry I was kind of in a rush to address this issue on our servers. I have tested this solution and can confirm it solves the issue.

If you want, I can submit a pull request for this change.

lrljoe commented 3 days ago

Author

Thanks for answering, and answering quickly.

Our model indeed implements de Illuminate\Contracts\Auth\Authenticatable Interface. However, the way typing work means the result of auth()->user() must be of the provided class or one of its direct descendent, which is not the case here.

I'm totally on board with typing everything. In this case defining the user property as Illuminate\Contracts\Auth\Authenticatable seems to me to be the better solution. I should have mentioned it in my bug report, I'm sorry I was kind of in a rush to address this issue on our servers. I have tested this solution and can confirm it solves the issue.

If you want, I can submit a pull request for this change.

@khwadj

Ah that's not painful at all in that case! Please do feel free to issue a PR, I 100% encourage it, otherwise you'll probably have a wait for 2-3 minor releases (approx 3-4 weeks) before I can release a fix for this particular issue.

Please do feel free to reach out to me on the official Discord as well if you stumble upon anything that you're not sure of, Discord is absolutely a faster way to get hold of me than GitHub.