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

[9.x] Model::whereRelation ignores the callback function or does it wrong #42465

Closed
wants to merge 4 commits into from
Closed

Conversation

adideas
Copy link
Contributor

@adideas adideas commented May 20, 2022

Eloquent

$userSQL = User::whereRelation(
    "posts",
    function($query) {
        // THIS
        $query->selectRaw("id")->limit(1)->where("public", true);
    }
)->toSql();

Query

$dbSQL = DB::table("users")->whereExists(
    function ($builder) {
        // relations simulation
        $builder->from("posts")->addSelect(['*'])->whereColumn(
            'users.id', '=', 'posts.user_id'
        );
        // THIS
        $builder->selectRaw("id")->limit(1)->where("public", true);
    }
)->toSql();

Eloquent output

select * 
from "users" 
where exists (
   select * 
   from "posts" 
   where "users"."id" = "posts"."user_id" and ("public" = ?)
)

Query output

select * 
from "users" 
where exists (
   select *, id 
   from "posts" 
   where "users"."id" = "posts"."user_id" and "public" = ? limit 1
)

@adideas
Copy link
Contributor Author

adideas commented May 20, 2022

I agree I could have used a different method. For example "whereHas" instead of "whereRelation".
But I thought that it should work correctly.

I added tests for checks. I can improve them if needed in the next pull request.

@adideas
Copy link
Contributor Author

adideas commented May 20, 2022

As I understand it, the last one who changed this code was Italo DarkGhostHunter.
Pull Request [8.x] Adds a simple where helper for querying relations #38499
@DarkGhostHunter -> Adds simple where helper for relations. 23 Aug 2021

@DarkGhostHunter
Copy link
Contributor

(checking...)

@DarkGhostHunter
Copy link
Contributor

Well, it's expected:

Add a basic where clause to a relationship query.

I don't remember why I didn't do that in the first place, I think it was because the callback was already handled by Eloquent where() and it doesn't replaces whereHas(), which is a convenience method for has() with a callback where you have more control.

@adideas
Copy link
Contributor Author

adideas commented May 20, 2022

Well, it's expected:

Add a basic where clause to a relationship query.

I don't remember why I didn't do that in the first place, I think it was because the callback was already handled by Eloquent where() and it doesn't replaces whereHas(), which is a convenience method for has() with a callback where you have more control.

@DarkGhostHunter I'm sorry I commented on you. I thought you should see.

@GrahamCampbell GrahamCampbell changed the title Model::whereRelation ignores the callback function or does it wrong. [9.x] Model::whereRelation ignores the callback function or does it wrong May 21, 2022
@adideas
Copy link
Contributor Author

adideas commented May 22, 2022

@GrahamCampbell "Changed name" Thanks for correcting me!

@taylorotwell
Copy link
Member

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.

@taylorotwell
Copy link
Member

We don't document using these methods this way.

@adideas
Copy link
Contributor Author

adideas commented May 24, 2022

@taylorotwell I did an analogy.
where(callback)
...
has('relation', '>=', 1, "and", callback)
whereHas('relation', callback)
whereRelation() // no callback function
doesntHave('relation', operator, callback)
whereDoesntHave('relation', callback)
whereHasMorph('relation', [], callback)
whereDoesntHaveMorph('relation', [], callback)

I agree it's not in the documentation.
But as I understood by researching other methods, it is always possible to specify a callback function.
Also, not everything is described in the documentation.

Example 1 (method has)
There is no information that the 4th argument is a callback.
Example 2 (method doesntHave)
There is no information that the 3th argument is a callback.

In the last pull request you told me.
"All changes should follow the Laravel approach."
I believe that the current change does not violate the Laravel approach.

The developer who implemented this method said. DarkGhostHunter: I don't remember why I didn't do that in the first place, I think it was because the callback was already handled by Eloquent where() and it doesn't replaces whereHas(), which is a convenience method for has() with a callback where you have more control.

I have used this method. I needed to pass a callback function. I have read the documentation. But this was not in the documentation. I started looking for ways to do it differently. I found the answer not in the documentation. I looked at the source code. Found that this is a wrapper over whereHas. I thought. Why can't I pass a callback function properly?
Isn't that logical?

I don't know why you think the current addition is wrong.
I followed the Laravel approach.
This is not a critical addition.
You or I can add information to the documentation.

Tell me how it would be better. I'll fix it.

@adideas
Copy link
Contributor Author

adideas commented May 24, 2022

// One is enough for me. Which will reduce the burden.
User::whereRelation("posts", function($query) {
   $query->limit(1);
})

But I can't do that! Why?
I have to use "whereHas".
What's the point in "whereRelation"?
I don't know!

I can add documentation!

@adideas
Copy link
Contributor Author

adideas commented May 24, 2022

@taylorotwell I think we're talking about a supplement. I'm not sure if this is a mistake. But many expect a different result using this method.

@adideas adideas changed the title [9.x] Model::whereRelation ignores the callback function or does it wrong [9.x] Model::whereRelation add callback function May 24, 2022
@adideas
Copy link
Contributor Author

adideas commented May 24, 2022

please show this

@adideas adideas changed the title [9.x] Model::whereRelation add callback function [9.x] Model::whereRelation ignores the callback function or does it wrong May 24, 2022
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.

3 participants