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

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

Closed
khwadj opened this issue Sep 17, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@khwadj
Copy link
Contributor

khwadj commented Sep 17, 2024

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:

  • Use a custom User class (that does not extend Illuminate\Foundation\Auth\User, in this example it's App\Models\Utilisateur) , using the AuthServiceProvider
  • Create a livewire-datables component with at least one column
  • On the page showing your livewire-datables component, show or hide any number columns

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:

  • 3.4.17
  • 3.4.20

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

@khwadj khwadj added the bug Something isn't working label Sep 17, 2024
@khwadj khwadj changed the title [Bug]: Using a User class that does not extends Illuminate\Foundation\Auth\User crashes when a Rappasoft\LaravelLivewireTables\Events\LaravelLivewireTablesEvent is triggered [Bug]: Using a User class that does not extend Illuminate\Foundation\Auth\User crashes when a Rappasoft\LaravelLivewireTables\Events\LaravelLivewireTablesEvent is triggered Sep 17, 2024
@lrljoe
Copy link
Collaborator

lrljoe commented Sep 17, 2024

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
Copy link
Contributor Author

khwadj commented Sep 18, 2024

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
Copy link
Collaborator

lrljoe commented Sep 20, 2024

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.

@lrljoe
Copy link
Collaborator

lrljoe commented Sep 25, 2024

Released in 3.4.21

@lrljoe lrljoe closed this as completed Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants