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

Allows to use Relation instead of Builder to generate data #279

Merged
merged 2 commits into from
May 14, 2021
Merged

Allows to use Relation instead of Builder to generate data #279

merged 2 commits into from
May 14, 2021

Conversation

fabio-ivona
Copy link
Contributor

@fabio-ivona fabio-ivona commented May 10, 2021

During my development using this awesome package, I needed to display a table taking data from a many to many relationship:

Course <=> Subscription <=> Student

in order to render a table to display all students of a specific course, I used laravel-livewire-tables and tried to take data this way

class StudentstTable extends DataTableComponent
{
   //.....

   public function query(): Builder
    {
        return $this->course->students()->getQuery();
    }

  //.....
}

I thought everything worked until I tried to take one of the row's ID and found that it was not the ID of the Student, but the ID of the pivot table (Subscription)

that's because getQuery() of a many to many relationship mixes columns of both tables

an elegant solution would be to return an Illuminate\Database\Eloquent\Relations\Relation instead of the Builder:

class StudentstTable extends DataTableComponent
{
   //.....

   public function query(): Builder|Relation
    {
        return $this->course->students();
    }

  //.....
}

but this is not possible because of the strict return type checking of the query() method's signature

this PR slightly changes the code in order to loose the strict type checking, allowing to return a Relation.

every test runs smoothly without any change and this is a non breaking change: removing typechecking from parent method allows to still use typechecking on the classes extending it.

I could not use union types because of the php7.4 requirement, maybe this would be a nice additional PR in a future version as the php7.4 requirement will be dropped

@rappasoft sorry for my english, I'm not a native speaker. I hope to have well explained my goal with this pull request

and thanks for you awesome work! You saved me a lot of code!

edit: added tests

@rappasoft
Copy link
Owner

I don't see a problem with this. The test suite is not that powerful, so I'll try to put it through some manual tests to make sure nothing breaks.

@fabio-ivona
Copy link
Contributor Author

@rappasoft may I help by adding some test for a specific aspect that worries you?

@rappasoft
Copy link
Owner

No this is fine I just have nothing else to release with it right now. I'll add a patch tomorrow if nothing else comes through.

@rappasoft rappasoft added the Awaiting Next Release Currently merged into development awaiting a release to master label May 13, 2021
@fabio-ivona
Copy link
Contributor Author

great!

again! thanks for your awesome and valuable work!

@rappasoft rappasoft mentioned this pull request May 13, 2021
@rappasoft rappasoft merged commit d3d0bab into rappasoft:master May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Next Release Currently merged into development awaiting a release to master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants