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

Conditional (when helper) gives back Eloquent Builder instance inside the closure instead of the relationship instance #53292

Open
christian-nielsen opened this issue Oct 24, 2024 · 11 comments

Comments

@christian-nielsen
Copy link

christian-nielsen commented Oct 24, 2024

Laravel Version

11.28.1

PHP Version

8.3

Database Driver & Version

No response

Description

Inside the when closure you will not get a relationship instance, so any specific relationship function will not be available.

Expected:
$query to be BelongsToMany instance

Actual:
$query is a Illuminate\Database\Eloquent\Builder instance

Ex. BelongsToMany relationships, here you will not be able to call the wherePivot*-methods.

return MyModel::find(2)
->someRelationship()
->when(true, function($query) {
        logger($query::class); // Illuminate\Database\Eloquent\Builder

        $query->wherePivotBetween('updated_at', ['2000-05-05', '2024-05-05']); 
    return $query;
})->get();

Steps To Reproduce

Create a model any kind of relationship and and fire a query that uses the relationship. Like in the example above.

@rodrigopedra
Copy link
Contributor

rodrigopedra commented Oct 25, 2024

As a workaround, the Eloquent Builder returned is the one being built by the Relation instance, not the parent's query builder (as the Illuminate\Database\Eloquent\Relations\Relation class uses the Illuminate\Support\Traits\ForwardsCalls trait).

So you can use this call in the meanwhile:

return MyModel::find(2)
    ->someRelationship()
    // replace "pivot" by the pivot table's name, 
    // in case you are using a custom name
    ->when(true, fn ($query) => $query->whereBetween('pivot.updated_at', ['2000-05-05', '2024-05-05']))
    ->get();

You can try sending a PR, adding the Illuminate\Support\Traits\Conditionable trait to the Illuminate\Database\Eloquent\Relations\Relation class.

@ghost
Copy link

ghost commented Oct 25, 2024

@rodrigopedra then that trait will be included in relation twice. Once in the relation and once in the Eloquent Builder...

@rodrigopedra
Copy link
Contributor

@marius-mcp how come?

The Relation class does not extend the Builder class.

And the Illuminate\Support\Traits\ForwardsCalls trait would only forward to the Builder class calls to methods not found in the Relation class.

Adding the trait to Relation won't forward when calls to the Builder instance. And as it is not a subclass, there is also no conflict.

@ghost
Copy link

ghost commented Oct 25, 2024

@rodrigopedra you are right.

Copy link

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

@crynobone
Copy link
Member

While this may seem like a bug, developers may already use the method and the current behavior. Please send it to master branch

@ghost
Copy link

ghost commented Oct 28, 2024

@rodrigopedra I think also https://github.com/laravel/framework/blob/11.x/src/Illuminate/Database/Query/Builder.php the query builder should have that trait. So, Relation, Eloquent Builder and Query Builder because of the forwards call to. The reason is that on callbacks the instance of the query can be Relation, Eloquent Builder and Query Builder.

@Sajid-al-islam
Copy link

I have sent the PR to master branch

@ghost
Copy link

ghost commented Nov 18, 2024

@Sajid-al-islam why didn't you put the trait also on the Query Builder?

@Sajid-al-islam
Copy link

I forgot, I am adding it

@mhshagor
Copy link

mhshagor commented Feb 6, 2025

It seems you're encountering an issue with the when closure in Laravel when working with BelongsToMany relationships. Specifically, the problem arises because inside the when closure, the $query variable is an instance of Illuminate\Database\Eloquent\Builder, not the expected BelongsToMany instance. This means that you cannot access the wherePivot methods.

Explanation
In Laravel, when you define a relationship method (like someRelationship()), it returns a relationship instance (e.g., BelongsToMany). However, when you chain methods, Laravel returns a query builder instance, which does not have access to relationship-specific methods.

Solution
To work around this issue, you can use the getQuery() method on the relationship instance before applying the when closure. Here’s how you can modify your code:

return MyModel::find(2)
    ->someRelationship()
    ->getQuery() // Get the underlying query builder for the relationship
    ->when(true, function($query) {
        logger($query::class); // This will now be the BelongsToMany instance

        // Now you can use wherePivot methods
        $query->wherePivotBetween('updated_at', ['2000-05-05', '2024-05-05']); 
        return $query;
    })->get();

Summary of Changes
Use getQuery(): By calling getQuery() on the relationship, you ensure that you are working with the correct query builder instance that allows you to use the wherePivot methods.
Steps to Reproduce

  1. Create a model with a BelongsToMany relationship.
  2. Fire a query that uses the relationship, similar to the example provided.
  3. Implement the suggested change to ensure the correct instance is being used in the when closure.

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

No branches or pull requests

6 participants
@crynobone @rodrigopedra @Sajid-al-islam @mhshagor @christian-nielsen and others