issue with query select override

[wanghanlin]

I'm re-opening issue #21464 , I believe fix #21468 didn't fix the problem.

@themsaid the reason why I put clear binding into select method is because no matter where select method is called, the clean method won't affect the code, your fix only fix the problem with withCount, but user can still face same issue, if like your test case,

1. Third have global scope where status = 1 and using this query on second.
2. Second have protected $withCount = ['thirds'];

we just need call this

Second::select('id')->where('foo', 'bar')->get()

in this code, it will override select like how it does in withCount, if run

Second::select('id')->where('foo', 'bar')->toSql()
Second::select('id')->where('foo', 'bar')->getBindings()

it will show

select "id" from "seconds" where "foo" = ?

[
     1,
     "bar",
]

the final SQL will be wrong too, it will be select "id" from "seconds" where "foo" = 1

my conclusion is whatever case, as long as select() method being called, it's overriding original select columns, this method is not like addSelect(), all old bindings for select should be all cleared, so we should put a global binding clean code there.

For the simplest way to reproduce bug, check comment below.

Created PR #21486

[sisve]

A quick observation from someone that hasn't understood the problem; if there's a code commit that attempted to fix your problem, but didn't, then you totally failed to provide clear steps to reproduce the problem, and as such the coder couldn't reproduce your exact problem or verify the fix properly.

Hint hint.

Can you provide a test that currently fails, but should work?

[wanghanlin]

Hi @sisve the fix in #21468 fix a specific case where explained in #21116 and #21464 , but not the entire problem, the explanation is based on that test cases of that PR #21468 , so I agree it's not very friendly, thanks for your suggestion

Here is the simplest way to reproduce the problem, run this in tinker

$query = app('Illuminate\Database\Query\Builder')->selectRaw('(select count(*) from posts where status = ?)', [1])->from('users');
$query->toSql();
$query->getBindings();
$query = $query->select()->where('foo', 'bar');
$query->toSql();
$query->getBindings();

it will output correct SQL for first query.

>>> $query->toSql();
=> "select (select count(*) from posts where status = ?) from "users""
>>> $query->getBindings();
=> [
     1,
   ]

and it will output wrong SQL (Bindings) once columns have been override

>>> $query->toSql();
=> "select * from "users" where "foo" = ?"
>>> $query->getBindings();
=> [
     1,
     "bar",
   ]

so mysql will run query as select * from users where foo = 1 instead of select * from users where foo = bar, so here you can notice in second query, columns have been override by select() so the final SQL Bindings should be only ["bar"], and that's what I'm trying to fix in PR #21465

[brunogaspar]

Hey

Here's another way to reproduce

$stories = Story::published()
    ->whereHas('organisation.company', function ($query) use ($company) {
        $query->whereNotNull('published_at');
        $query->where('companies.id', '=', $company->id);
    })
    ->get()
;

When calling toSql():

select * from `stories` where `published_at` is not null and exists (select * from `organisations` where `stories`.`organisation_id` = `organisations`.`id` and exists (select * from `companies` where `organisations`.`id` = `companies`.`organisation_id` and `published_at` is not null and `companies`.`id` = ?)) and `stories`.`deleted_at` is null

When calling getBindings():

array:5 [
  0 => "companies"
  1 => "companies"
  2 => "companies"
  3 => "companies"
  4 => 549
]

@themsaid The PR https://github.com/laravel/framework/pull/21486 fixed the issue, so no idea why it was declined saying it's not a bug when it's clearly a way to use Eloquent and it's clearly a bug on the framework.

Edit:

Just to keep it in perspective, this only happens if we use the $withCount property on the models. If i don't use them, it works as expected.

[wanghanlin]

Hi @brunogaspar, I do agree this is clearly a bug, but @themsaid have a good point that this might break someone's code as it's not fully back-ward compatible. if someone are building the query himself by first set bindings and then assign select columns, his query won't work after my fix, so now I agree on him that we should push the fix to 5.6 instead of 5.5. Meanwhile, did you check if your code still have issue after fix #21468? if that's the case we can make another fix to fix your specific problem instead of change the select() function.

[brunogaspar]

I'm on 5.5.14 which was tagged yesterday and it doesn't work.

I tried before coming here and "complain" 😁

With your changes, it does work.

[wanghanlin]

@brunogaspar can you share your three model class? more specifically which model have withCount on which model and anything else related in these models so I can reproduce your error and find out a patch for that?

[brunogaspar]

Of course..

Organisation.php

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Model;

class Organisation extends Model
{
    protected $table = 'organisations';

    public function company()
    {
        return $this->hasOne(Company::class, 'organisation_id');
    }

    public function stories()
    {
        return $this->hasMany(Story::class, 'organisation_id');
    }
}

Company.php

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Model;

class Company extends Model
{
    protected $table = 'companies';

    protected $dates = ['published_at'];

    protected $withCount = [
        'categories',
        'countries',
        'contacts',
        'followers',
    ];

    public function categories()
    {
        return $this
            ->morphToMany(Category::class, 'entity', 'categories_morphed')
            ->withTimestamps()
        ;
    }

    public function countries()
    {
        return $this
            ->morphToMany(Country::class, 'entity', 'countries_morphed')
            ->withTimestamps()
        ;
    }

    public function contacts()
    {
       return $this
           ->morphToMany(User::class, 'entity', 'contacts_morphed')
           ->withTimestamps()
       ;
    }    

    public function followers()
    {
        return $this->morphMany(Follower::class, 'followable');
    }

    public function organisation()
    {
        return $this->belongsTo(Organisation::class, 'organisation_id');
    }
}

Story.php

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\SoftDeletes;

class Story extends Model
{
    useSoftDeletes;

    protected $dates = [
        'published_at',
        'deleted_at',
    ];

    public function organisation()
    {
        return $this->belongsTo(Organisation::class, 'organisation_id');
    }
}

I've simplified them for brevity.

[wanghanlin]

@brunogaspar if you add ->setBindings([], 'select') in line 340 of src/Illuminate/Database/Eloquent/Relations/HasOneOrMany.php, it should work, but I discovered there are more places using getRelationExistenceQuery without setBindings so I'm not sure either if that part of code is functional, or if it's good to add setBindings there.

More places are

• src/Illuminate/Database/Eloquent/Relations/MorphOneOrMany.php#106
• src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php#827
• src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php#848
• src/Illuminate/Database/Eloquent/Relations/MorphToMany.php#823

So it's better wait @themsaid 's suggestion here, for your reference i created a test file that will fail, and work if add setBindings on line 340 of HasOneOrMany.php. Link is here

[brunogaspar]

Thanks @shwhl i'll have a look in a bit and will let you know.

Again, thanks. Really appreciated!

[brunogaspar]

@shwhl Having that change on src/Illuminate/Database/Eloquent/Relations/HasOneOrMany.php fixes it for me.

[wanghanlin]

that's good!

[themsaid]

I'd mark this as a no-fix, there's no safe way I can think of where this can be fixed without breaking people apps. It's a rare edge case in my opinion.

[brunogaspar]

@themsaid Sorry to come back to this, but i don't really agree when you say "it's a rare edge case".

Basically, this is a feature that doesn't work as expected, and i have to assume here, because it was not implemented properly, which is fine, was implemented through a PR i believe.

However, when using Model::withCount('foo'), it works perfectly fine but when using the protected $withCount = ['foo']; property directly on the modal it doesn't. Shouldn't they behave the same way? If not, they should as the end goal is to achieve the exact same thing, but, what do i know.

But anyway.. if the no-fix is the way to go, at least a fix should be found for 5.6 if not done already (i haven't checked), that way there's no chance on causing a "breaking change".

✌️

[garygreen]

I'm having the same problem as well.

Basically this is my query - it gets the number of members within a specified distance.

Member::query()
    ->addSelect(\DB::raw("ROUND((3959 * acos( cos(radians($latitude)) * cos(radians( `members_data_members`.`data_latitude` )) * cos( radians( `members_data_members`.`data_longitude` ) - radians($longitude) ) + sin( radians($latitude) ) * sin( radians(`members_data_members`.`data_latitude`) ) ) ), 0) AS distance"))
    ->having('distance', '<=', $miles)
    ->count();

This fails as the select is overriden. Maybe this is an "edge case" but I don't believe so, either. Any folks who use use a dynamic expression and having will experience problems as well.