[8.x] Add foreignKey and localKey to query if they are missing

[shaedrich]

… to ensure, eager loading can assign the results to the right parent/child

Issue	Fix #2550
Based on	#37496
Suggested in	#37496 (comment) by @lk77

If I have something like this

class Post extends Model
{
    public function comments(): HasMany
    {
        return $this->hasMany(Comment::class)->select('body');
    }
}

class Comment extends Model
{
    public function attachments(): HasMany
    {
        return $this->hasMany(Attachment::class);
    }
}

Comment just has an empty attachments array. I know, why this happens and that I can solve this, by adding 'id' to the 'select()` method call, but wouldn't it be good if Eloquent knew that it needs this key? Since there's no error message, the mistake is everything but obvious due to the abstraction Laravel and Eloquent have.

[shaedrich]

I'm currently not sure if the keys should be removed from the collection if the user didn't specify them in the first place (reversing the added keys from before executing the query

[shaedrich]

@laravel What do you think of adding this change?

[lk77]

this looks clean to me,
there is one test failing because it expect id not be present, which is not the case with this new behaviour.

[driesvints]

@shaedrich prs in draft won't be reviewed and closed after a few days. Prs with failing tests will also be closed. Please make sure to have passing tests and a thorough description when submitting.

[shaedrich]

Are you sure, the error is caused by my changes? Cause I didn't change anything route-related

There was 1 error:

1. Illuminate\Tests\Integration\Routing\UrlSigningTest::testSigningUrlWithCustomRouteSlug
   ErrorException: Undefined property: Illuminate\Tests\Integration\Routing\RoutableInterfaceStub::$routable

D:\a\framework\framework\tests\Integration\Routing\UrlSigningTest.php:204
D:\a\framework\framework\src\Illuminate\Routing\UrlGenerator.php:520
D:\a\framework\framework\src\Illuminate\Routing\UrlGenerator.php:322
D:\a\framework\framework\src\Illuminate\Support\Facades\Facade.php:261
D:\a\framework\framework\tests\Integration\Routing\UrlSigningTest.php:35

[lk77]

i was talking about this one :

There was 1 failure:

1) Illuminate\Tests\Integration\Database\EloquentCollectionLoadMissingTest\EloquentCollectionLoadMissingTest::testLoadMissing
Failed asserting that an array does not have the key 'id'.

[shaedrich]

Oh, okay. I'll check that one!

it expect id not be present, which is not the case with this new behaviour.

That means, we make the test assert the opposite?

[lk77]

you should rebase and force push your branch, you are not up to date.

[lk77]

i think this will fix tests issues

edit: it does not seems to be fixed, i don't know why

[donnysim]

I don't think this would be enough, in some cases it would end up being ->select('comments.body') to accommodate for joins, which I'm not sure if existing tests cover that. Also this covers HasOne, HasMany but I'm not sure about if it'll work with MorphOne MorphMany which also extend HasOneOrMany as they use more than just foreign key. Also what about other relationships like BelongsTo, BelongsToMany and such? Kind of seems like it needs tests.

I'd personally rather have an exception to let me know I forgot something as a query exception would rather than something magically correct the query for me with a probability to throw a wrench in some use case where there should be none.

[driesvints]

8.x was failing. Try rebasing again.

[shaedrich]

@donnysim Exception was my first attempt. Got outvoted.

[driesvints]

@shaedrich try merging once more. Should be fixed now.

[taylorotwell]

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

[lk77]

I still think it's something to think about it, perhaps not now, but worth writing it down somewhere for later use.
when i see the amount of hours we loose every year because of missing foreign key in a select, it's always painfull to debug,
i don't see any use case where the foreign key should be missing on purpose when calling a relationship.