Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BelongsTo relation with nullable relations #42493

Closed
pionl opened this issue May 24, 2022 · 2 comments
Closed

BelongsTo relation with nullable relations #42493

pionl opened this issue May 24, 2022 · 2 comments

Comments

@pionl
Copy link
Contributor

pionl commented May 24, 2022

  • Laravel Version: 9.13.0
  • PHP Version: 8.1.2

Description:

  • While using belongsTo relations (with/load) there can be situations when there are no relations (nullable relations) to be loaded and query is "invalid". -> select `id`, `name`, `public_name` from `options` where 0 = 1 .
  • This similar issue can be potentially found on other relations
  • This bug is not noticeable if the results contains at least 1 value in the relation (if you have pagination, some pages can contain this error).

Steps To Reproduce:

  1. Create a table with optional belongsTo relations
  2. Build a query to get the results (all results should contain null)
  3. Here a screenshot of a real queries

Snímek obrazovky 2022-05-24 v 14 22 56

Fix

  1. In Illuminate\Database\Eloquent\Relations\Relation add property protected bool $hasValidConstrains = true;
  2. Change the implementation of Illuminate\Database\Eloquent\Relations\Relation::getEager
 public function getEager()
    {
        if ($this->hasValidConstrains) {
            return $this->get();
        }
        return new Collection();
    }
  1. Change the implementation of Illuminate\Database\Eloquent\Relations\BelongsTo::addEagerConstraints
public function addEagerConstraints(array $models)
    {
        // We'll grab the primary key name of the related models since it could be set to
        // a non-standard name and not "id". We will then construct the constraint for
        // our eagerly loading query so it returns the proper models from execution.
        $key = $this->related->getTable().'.'.$this->ownerKey;

        $whereIn = $this->whereInMethod($this->related, $this->ownerKey);

        $eagerModelKeys = $this->getEagerModelKeys($models);

        if ($eagerModelKeys === []) {
            $this->hasValidConstrains = false;
            return;
        }

        $this->query->{$whereIn}($key, $eagerModelKeys);
    }

Result

Snímek obrazovky 2022-05-24 v 14 26 39

I can make PR if you consider this a bug (which I believe it is) and don't have the time to make the fix.

Thank you for your time.
Martin.

@driesvints
Copy link
Member

I personally don't see a big issue here but you're free to attempt that PR if you like 👍

@pionl
Copy link
Contributor Author

pionl commented May 26, 2022

Hey @driesvints what do you mean? This triggers a query on database that is not needed. I see it as a bug right?

I'll make the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants