[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.

I think, this behavior differs from how raw sql queries would behave. They would actually throw an error in that case. An empty collection is a wrong result. That's not what I expect. Not throwing exceptions because the code does something is misleading. 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.

Some might argue, that adding an exception might be a breaking change. In that case, one might want to go for an E_USER_NOTICE instead.

Others would rather automatically add the missing keys to the query but this might be unintuitive in my opinion.

Therefore, two changes have to be made:

src/Illuminate/Database/Eloquent/Relations/HasOneOrMany.php

namespace Illuminate\Database\Eloquent\Relations;

use Illuminate\Database\Eloquent\Builder;
use Illuminate\Database\Eloquent\Collection;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\Concerns\InteractsWithDictionary;
use InvalidArgumentException;

abstract class HasOneOrMany extends Relation
{
    // ...

    /**
     * Build model dictionary keyed by the relation's foreign key.
     *
     * @param  \Illuminate\Database\Eloquent\Collection  $results
     * @throws \InvalidArgumentException
     * @return array
     */
    protected function buildDictionary(Collection $results)
    {
        $foreign = $this->getForeignKeyName();

        return $results->mapToDictionary(function ($result) use ($foreign) {
            if (! isset($result->{$foreign})) {
                throw new InvalidArgumentException('Relation needs foreign key "'.$result->qualifyColumn($foreign).'" to be fetched');
            }

            return [$this->getDictionaryKey($result->{$foreign}) => $result];
        })->all();
    }

    // ...
}

src/Illuminate/Database/Eloquent/Relations/Relation.php

namespace Illuminate\Database\Eloquent\Relations;

use Closure;
use Illuminate\Database\Eloquent\Builder;
use Illuminate\Database\Eloquent\Collection;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\ModelNotFoundException;
use Illuminate\Database\MultipleRecordsFoundException;
use Illuminate\Database\Query\Expression;
use Illuminate\Support\Arr;
use Illuminate\Support\Traits\ForwardsCalls;
use Illuminate\Support\Traits\Macroable;
use InvalidArgumentException;

/**
 * @mixin \Illuminate\Database\Eloquent\Builder
 */
abstract class Relation
{
    // ...

    /**
     *
     * @param  array  $models
     * @param  string|null  $key
     * @throws \InvalidArgumentException
     * @return array
     */
    protected function getKeys(array $models, $key = null)
    {
        return collect($models)->map(function ($value) use ($key) {
            if (! isset($value->{$key})) {
                throw new InvalidArgumentException('Relation needs parent key "'.$value->qualifyColumn($key).'" to be fetched');
            }

            return $key ? $value->getAttribute($key) : $value->getKey();
        })->values()->unique(null, true)->sort()->all();
    }

    // ...
}

[tomadmiraal]

Isn't it posible to detect what key is missing and add it temperarly to the select list?

The key is set in the relation that has been created in the modal and thereby known.

In that case there will be never a missing key thats needed by a relation. afterwards its posible to return only the fields that are set in the select method?

[shaedrich]

#38215 tried to achieve that but was rejected.

[tomadmiraal]

Thnx for your answer.

For what I see, the solution is not rejected because it is a stuppid idea but because he doesn't want to make to many changes in Laravel 8.x anymore.

Maybe a new request for Laravel 9.x? this feature makes life in laravel so much easier!

[shaedrich]

He didn't suggest this. I'm currently a bit cross with Taylor because it seemed like he didn't make any effort to include this change despite my repeated tries.

But yes, I'd love to see that change included as much as you do. Especially, since I'm strongly convinced that the current behavior is wrong.