Skip to content

[11.x] Where doesnt have nullable morph #54363

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

Merged
merged 11 commits into from
Feb 6, 2025
Merged

[11.x] Where doesnt have nullable morph #54363

merged 11 commits into from
Feb 6, 2025

Conversation

liamduckett
Copy link
Contributor

Hey, this PR aims to resolve this issue.

The problem currently (as described in the issue) is that QueriesRelationships:: excludes Models with a null morph_type value.

This is due to the logic in QueriesRelationships::hasMorph revolving around types (which exludes null):

// src/Illuminate/Database/Eloquent/Concerns/QueriesRelationships.php (231)
if ($types === ['*']) {
    $types = $this->model->newModelQuery()->distinct()->pluck($relation->getMorphType())
        ->filter()
        ->map(fn ($item) => enum_value($item))
        ->all();
}

My proposed solution adds a conditional clause to the query, to also include models with a null morph relation. I made the decision to widen the scope to include other operators - as this felt more complete of a solution than just solving for default whereDoesntHave. I understand this comes with it's own drawbacks, and am happy to partly undo if desired.

This only applies if the chosen type is wildcard, as I understand choosing types currently means any results must have that type of morph relation (and naturally, null cannot acheieve this). Truthfully I'm not particularily confident on this bit - would appreciate any checking over!

I've only added the test case from the issue, as it feels like the majority use case.

Please see my logic for deciding which operators to include below.

operators;
1. >=
  a) positive: doesnt need nulls as is inclusionary
  b) zero:     will always be true...
  c) negative: will always be true...
2. >
  a) positive: doesnt need nulls as is inclusionary
  b) zero:     doesnt need nulls as is inclusionary
  c) negative: will always be true...
3. <=
  a) positive: needs nulls as is exclusionary
  b) zero:     needs nulls as is exclusionary
  c) negative: can never be true...
4. <
  a) positive: needs nulls as is exclusionary
  b) zero:      can never be true...
  c) negative: can never be true...
5. =
  a) positive: doesnt need nulls as is inclusionary
  b) zero:     needs nulls as is exclusionary
  c) negative: can never be true...
6. !=
  a) positive: needs nulls as is exclusionary
  b) zero:     doesnt need nulls as is inclusionary
  c) negative: will always be true...

@crynobone crynobone self-requested a review February 6, 2025 00:43
@liamduckett
Copy link
Contributor Author

Test failures look unrelated to the PR - do they need rerunning?

@taylorotwell taylorotwell merged commit b29fa77 into laravel:11.x Feb 6, 2025
34 of 38 checks passed
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

Successfully merging this pull request may close these issues.

whereDoesntHave not produce correct query when use with nullableMorphs
3 participants