[10.x] Add HasOneOrMany::shouldAddForeignKeyCheck()

[cosmastech]

#46909

HasOneOrMany adds a where foreign_key is not null to all queries, though in most cases it's not needed.

This issue comes up a lot: it makes it hard to debug queries, it's unnecessary in most (all?) cases, and it leads to confusion from developers trying to figure out WHY it was added (if they did something wrong, where it's being added within the framework, etc).

Here is a solution that allows developers to turn it off entirely, or per relationship

class AppServiceProvider extends ServiceProvider
{
    public function boot()
    {
        HasOneOrMany::shouldAddForeignKeyCheck(false);
    }
}

Voila. Now instead of:

'select * from "posts" where "posts"."user_id" = ? and "posts"."user_id" is not null'

you get

'select * from "posts" where "posts"."user_id" = ?

Don't want it turned off globally? Cool, nothing is different.

Want it turned on globally but want it turned off for some relationship? Have at it.

$posts = HasOneOrMany::withoutForeignKeyCheck($user->posts()->get());

In a future release, this static property could be set to false by default, and then maybe in a subsequent release, removed entirely.

final thought

I like how the DB migration traits within test uses a state class with static properties. Would be nice to see this bound to the container so it could be easily swapped within tests. But I don't see this pattern used elsewhere within the framework, so I assumed something so far out of the usual stood less of a chance of being accepted (especially for an issue that has been a long-standing).

[bert-w]

Assume we have some User parent model. If for some reason the parentKey (id) is null, then the $query->where() will eventually call it as $query->whereNull($this->foreignKey) (see Query Builder where()), including all related models with a null foreign-key (which is wrong).

Shouldn't the solution be more something like this?

if (! is_null($this->getParentKey()) {
    // Value available, apply it as the where clause:
    $query->where(...);
} else {
    // Essentially a query-aborter here since we won't get any results (better would be to not query at all):
    $query->where(1, '=', 0);
}

I couldn't find a testcase that covers this behaviour and this addition wasn't explained in #37451 either.

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