topclaudy / compoships

Multi-columns relationships for Laravel's Eloquent ORM
MIT License
1.1k stars 130 forks source link

With() is not working #123

Closed miqoo1996 closed 2 years ago

miqoo1996 commented 3 years ago

When the trait "use Compoships" is used in the model the with() function isn't working at all. So, why does the trait stop it? I opened the codes inside your package and noticed that you used newBelongsTo() each time when Laravel's default belongs to is called, I don't think that this can be a good solution at all. I have a page where is pagination with 500 items per page, So it's an extremely heavy query that has more than 15 relationships.

From my point of view, this's a serious issue and meantime imperceptible for developer, it can cause problems to any project till this's not fixed.

Please take a look at the problem and fix it.

miqoo1996 commented 3 years ago

The problem is that the package ignores the with() function and it's the same if I don't use the with(). It does select * for every row for any relation instead of doing join query, or if you use whereIn, it does SQL selects for every condition.

topclaudy commented 3 years ago

@miqoo1996 Can you please PR the fix?

miqoo1996 commented 3 years ago

@topclaudy did you mean open a PR? So I tried to open the PR but it seemed that I didn't have access to it.

topclaudy commented 3 years ago

Implement the fix and submit a pull request

miqoo1996 commented 3 years ago

@topclaudy maybe I fix it in my project and submit it here to help people to avoid the issue, but why should this be done by me, I don't understand to be honest. I thought this is your package and you try to maintain this, provide a quality and functional code.

topclaudy commented 3 years ago

@miqoo1996 It's open source and contributions are welcomed. If you have the solution to a problem just submit a PR like other contributors and I will gladly merge it when I have a chance. I can't work on that for the moment (I'm actually replying to you from my phone). Maintainers are not full time invested in their packages, especially on weekends.

erikn69 commented 3 years ago

So I tried to open the PR but it seemed that I didn't have access to it.

You have to make a fork, then make your fix on that fork, and later do the PR, don't forget test with PHPUnit

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND

@miqoo1996 read https://opensource.org/licenses/MIT

but why should this be done by me

Because open source grows with the help of the community and that's why we can all do PR, you are not obliged to collaborate but it would be selfish if you know how to fix something for everyone

so, that's why it's free of charge, the author cannot spend 24/7 troubleshooting third-party problems

I appreciate and thanks @topclaudy & collaborators for maintaining support of this code

keironlowe-edriving commented 2 years ago

I'm experiencing the same issue, in the query that's executed, it checks that the columns are NULL instead of their correct values, my code below...

CoachingRecommendation::with(['driver' => function ($query) use ($user, $driverStatus) {
    $query->withinUserLocations($user);
    $query->where('status', $driverStatus ?? 'Active');
}])

Which creates the query

SELECT *
FROM `drivers`
WHERE (
    (
      `drivers`.`company` IS NULL
      and `drivers`.`employee_number` IS NULL
    )
  )
  and (`location1` in ('UK'))
  and (
    `status` = 'Active'
  )

I'll try to look into it and open a PR, although my knowledge with the internals of Eloquent is limited so I'm not sure I'll be able to solve it.

erikn69 commented 2 years ago

@keironlowe-edriving it does not seem to me that it is the same problem, it seems that the model has the relation fields as null, for some reason it is not locating them or it is not bringing them from the db

keironlowe-edriving commented 2 years ago

You're right it's not the same issue, but there is an issue with using select. My code example was a reduced version, in the proper one I was also using selecting a raw SQL count, and it seems when using select, you have to include the primary key columns, otherwise they'll be null.

In my case, it turns out I didn't need with, so I don't have the issue anymore.

erikn69 commented 2 years ago

you have to include the primary key columns

yes, always, also inside the with, without the keys, laravel cannot bind the relation

PaolaRuby commented 2 years ago

@miqoo1996 any news?

miqoo1996 commented 2 years ago

Issue is closed!

miqoo1996 commented 2 years ago

@PaolaRuby I closed the issue for now. I'll make a PR when I have time. Now it'd be better to keep this closed.

Thanks!