[8.x] Warn if relation's foreign key is not in select list

[shaedrich]

Fix #2550

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.

[taylorotwell]

No plans to change this atm.

[shaedrich]

That's unfortunate. Because the behavior differs from how raw sql queries would behave. They would actually throw an error in that case. Just for the record: Why do you think, it's okay to have that unexpected behavoir?

[lk77]

throwing exceptions like that is not a good idea. It will break things that were previously working for no reason.

[shaedrich]

An empty collection is a wrong result. That's not what I expect.

Just because something throws no exception that doesn't mean that code works as expected. Not throwing exceptions because the code does something is misleading.

[lk77]

it's not enough for me to justify an exception, that will bring the whole thing down for no reason. Users will not understand that. I prefer to have missing data on some relationships, depending on the cases it might not be a big deal

[shaedrich]

Users don't necessarily understand why they get an empty collection either. And not understanding something and not being told anything is worse in my opinion. If you have an exception, you can google it and ask questions on stackoverflow.

[lk77]

there is a difference between notifying the developer about a potential issue, and breaking the whole page in production for all your users.

I don't see the point of throwing an exception in that case, just to warn the developer, an exception is not a warning, it's a handlable error, that will halt the execution of the page if not catched.

i would rather auto include the foreign key in the select than throwing an exception like that.

[shaedrich]

Okay, maybe a E_USER_NOTICE is better. How would you auto include the foreign key? Via addSelect()? If you auto select it, you change the behavior.

[shaedrich]

@lk77 Is #38215 what you'd do instead?

[lk77]

@shaedrich yeah totally something like that

for the implementation detail, i don't know, it seems to me that it only works with ->get([...columns...]) and not with ->select([...columns...)->get() ?

try to get confirmation from laravel team that it's something they want to do before spending time on this,
it's only my point of view. I think this can fit with the framework doctrine of making the life easier for developers, for me missing foreign key on the select is always a mistake that could be corrected. I don't think this will break any use case.

[shaedrich]

Yeah, not yet. Wanted to hear your opinion first.

Okay, added it and asked the Laravel team about it.