[5.2] Using ::has is creating a query with 6 bindings but a 7 key binding array

[bgarrison25]

Stack Overflow: http://stackoverflow.com/questions/34142183/laravel-5-1-has-many-relationship-returning-0-when-using-existence-query
Laravel IO: http://laravel.io/forum/12-08-2015-using-has-on-a-relationship-between-two-models-that-have-the-same-table?page=1#reply-29024

Setup:
php 7 / Laravel 5.1 / on homestead / redis cache (not being used in this instance)

Quick(ish) Explanation:

I have two models using the same table fm_vip_demographics. In the members model you have a trait that basically sets up a global scope to only return vip's with a vip_type_id of 3 (members). In the dependents model there is another trait to only return vip's with a vip_type_id of 2(dependents).

These models are then set up through a hasMany relationship. Now, when I try to do (new App\Models\Members)->has('dependents')->get(); it always returns nothing. I dumped out the query log and got this:

array:1 [
  0 => array:3 [
    "query" => "select * from `fm_vip_demographics` where `vip_type_id` = ? and `fm_vip_demographics`.`record_status` = ? and (select count(*) from `fm_vip_demographics` as `self_3005b65b2c6b3cccca2d6b3b119f3570` where `self_3005b65b2c6b3cccca2d6b3b119f3570`.`group_id` = `fm_vip_demographics`.`vip_id` and `vip_type_id` = ? and `fm_vip_demographics`.`record_status` = ? and `relationship_id` in (?, ?)) >= 1"
    "bindings" => array:7 [
      0 => 3
      1 => 1
      2 => 1
      3 => 2
      4 => 1
      5 => 3
      6 => 14
    ]
    "time" => 0.28
  ]
]

You will notice here that there are 6 question marks in the query but 7 bindings. Binding number 1 or 2 is incorrect as i only need 1 binding of 1 and this is causing the query to work incorrectly.

This is as far as I could go on debugging this issue. At this point I have determined its a bug because of the mismatch between binding array and query but I don't know what the source of the bug is. Let me know if you need any other specifics.

[GrahamCampbell]

We're open to PRs if you want to have a go at fixing it.

[bgarrison25]

Ill give it a look tomorrow and see if I can deduce the cause and get you guys a fix. no promises though as this would be my first foray into core development

[mikevrind]

I'm also experiencing binding problems since version 5.1.20 (no problems in 5.1.19).
This could make some sense since there where some changes to binding related methods within the framework in 5.1.20.

When using a whereHas on a relation which also contains a where statement I get an extra binding.

$getProducts = $this->whereHas( 'product.price', function ( $q ) use ( $userData )
        {
                    $q->where( 'products_prices.sale_price', '>', 0 );
        }
        } )
               // more code

// price relation

return $this->hasOne( 'ProductPrice', 'products_id' )->where('store_currencies_id', $countryData->store_currencies_id);

[bgarrison25]

So i've looked into it but I'll be honest it is way beyond my development skills. I just wanted to bring this particular issue to your attention. Sorry I can't be of more help

[acasar]

@bgarrison25 If you submit a failing unit test in a PR it's helpful too. And someone else can tackle the issue.

[bgarrison25]

I dug into this some more and I determined the general vicinity of the issue. In file Illuminate\Database\Eloquent\Builder.php in the method mergeWheresToHas on line 714 when I dd() the $hasQuery's bindings it has the erroneous binding. I checked and its a "where" binding. All the other bindings in that method are the appropriate ones. This is literally my first time looking at the core code so im not 100% sure what all of this means and am just trying to give you guys a point in the right direction.

If a little background is helpful: Both models work off the same table. One has a global Scope through "MemberTypeTrait" that basically says the vip_type_id is 1 where the relational model has a global scope through DependentTypeTrait that syas the vip_type_id is 2. They both have the RecordStatusActiveTrait which is my version of the soft delete. If you need any more info it is described a bit in my stackoverflow / laravel.io

To be honest @acasar I am not sure where I would start to create said Unit test either. If you have any questions about how my models are set up please let me know and ill answer as best i can.

[acasar]

@bgarrison25 Can you paste your global scope? I'm guessing you are not removing where clauses correctly.

If I am correct, your issue might already be resolved in Laravel 5.2.

[bgarrison25]

Which global scope? I have 2 on the main model and 2 (1 shared) on the relationship

[bgarrison25]

This is my RecordStatusActiveScope:

    <?php namespace App\Models\Eloquent\Scopes;

    use Illuminate\Database\Query\Builder as BaseBuilder;
    use Illuminate\Database\Eloquent\ScopeInterface;
    use Illuminate\Database\Eloquent\Builder;
    use Illuminate\Database\Eloquent\Model;

    class RecordStatusActiveScope implements ScopeInterface
    {

    /**
     * All of the extensions to be added to the builder.
     *
     * @var array
     */
    protected $extensions = ['Activate', 'Deactivate', 'WithInactives', 'OnlyInactives'];

    /**
     * Apply scope on the query.
     *
     * @param \Illuminate\Database\Eloquent\Builder  $builder
     * @param \Illuminate\Database\Eloquent\Model  $model
     * @return void
     */
    public function apply(Builder $builder, Model $model)
    {
        $column = $model->getQualifiedRecordStatusColumn();
        $builder->where($column, 1);
        $this->extend($builder);
    }

    /**
     * Remove scope from the query.
     *
     * @param \Illuminate\Database\Eloquent\Builder  $builder
     * @param \Illuminate\Database\Eloquent\Model  $model
     * @return void
     */
    public function remove(Builder $builder, Model $model)
    {

        $query = $builder->getQuery();

        $column = $model->getQualifiedRecordStatusColumn();

        $bindingKey = 0;

        // here you remove the where close to allow developer load
        // without your global scope condition

        foreach ((array) $query->wheres as $key => $where) {
            if ($this->isRecordStatusConstraint($where, $column)) {
                $this->removeWhere($query, $key);

                // Here SoftDeletingScope simply removes the where
                // but since we use Basic where (not Null type)
                // we need to get rid of the binding as well
                $this->removeBinding($query, $bindingKey);
            }

            if (!in_array($where['type'], ['Null', 'NotNull'])) {
                $bindingKey++;
            }
        }
    }

    /**
     * Extend the query builder with the needed functions.
     *
     * @param  \Illuminate\Database\Eloquent\Builder  $builder
     * @return void
     */
    public function extend(Builder $builder)
    {
        foreach ($this->extensions as $extension) {
            $this->{"add{$extension}"}($builder);
        }
    }

    /**
     * Remove scope constraint from the query.
     *
     * @param  \Illuminate\Database\Query\Builder  $query
     * @param  int  $key
     * @return void
     */
    protected function removeWhere(BaseBuilder $query, $key)
    {
        unset($query->wheres[$key]);

        $query->wheres = array_values($query->wheres);
    }

    /**
     * Remove scope constraint from the query.
     *
     * @param  \Illuminate\Database\Query\Builder  $query
     * @param  int  $key
     * @return void
     */
    protected function removeBinding(BaseBuilder $query, $key)
    {
        $bindings = $query->getRawBindings()['where'];

        unset($bindings[$key]);

        $query->setBindings($bindings);
    }

    /**
     * Check if given where is the scope constraint.
     *
     * @param  array   $where
     * @param  string  $column
     * @return boolean
     */
    protected function isRecordStatusConstraint(array $where, $column)
    {
        return ($where['type'] == 'Basic' && $where['column'] == $column && $where['value'] == 1);
    }

    /**
     * Extend Builder with custom method.
     *
     * @param \Illuminate\Database\Eloquent\Builder  $builder
     */
    protected function addWithInactives(Builder $builder)
    {
        $builder->macro('withInactives', function(Builder $builder) {
            $this->remove($builder, $builder->getModel());

            return $builder;
        });
    }

    /**
     * Extend Builder with custom method.
     *
     * @param \Illuminate\Database\Eloquent\Builder  $builder
     */
    protected function addOnlyInactives(Builder $builder)
    {
        $builder->macro('onlyInactives', function (Builder $builder) {
            $model = $builder->getModel();

            $this->remove($builder, $model);

            $builder->getQuery()->where($model->getRecordStatusColumn(), '<>', 1);

            return $builder;
        });
    }

    /**
     * Add the activate extension to the builder.
     *
     * @param  \Illuminate\Database\Eloquent\Builder  $builder
     * @return void
     */
    protected function addActivate(Builder $builder)
    {
        $builder->macro('activate', function (Builder $builder) {
            $builder->withInactives();

            return $builder->update([$builder->getModel()->getRecordStatusColumn() => 1]);
        });
    }

    /**
     * Add the activate extension to the builder.
     *
     * @param  \Illuminate\Database\Eloquent\Builder  $builder
     * @return void
     */
    protected function addDeactivate(Builder $builder)
    {
        $builder->macro('deactivate', function (Builder $builder) {
            $model = $builder->getModel();
            $inactiveId = ($model->{$model->primaryKey} == 0) ? 5 : 2;

            return $builder->update([$model->getRecordStatusColumn() => $inactiveId]);
        });
    }
}

This is my MemberTypeScope:

<?php namespace App\Models\Eloquent\Scopes;

use Illuminate\Database\Eloquent\ScopeInterface;
use Illuminate\Database\Eloquent\Builder;
use Illuminate\Database\Eloquent\Model;

class MemberTypeScope implements ScopeInterface
{

    public function apply(Builder $builder, Model $model)
    {
        $builder->where('vip_type_id', 3);
    }

    public function remove(Builder $builder, Model $model)
    {

        $query = $builder->getQuery();

        // here you remove the where close to allow developer load
        // without your global scope condition

        foreach ((array) $query->wheres as $key => $where) {

            if ($where['column'] == 'vip_type_id') {

                unset($query->wheres[$key]);

                $query->wheres = array_values($query->wheres);
            }
        }
    }
}

This is my dependentTypeScope:

<?php namespace App\Models\Eloquent\Scopes;

use Illuminate\Database\Eloquent\ScopeInterface;
use Illuminate\Database\Eloquent\Builder;
use Illuminate\Database\Eloquent\Model;

class DependentTypeScope implements ScopeInterface
{

    public function apply(Builder $builder, Model $model)
    {
        $builder->where('vip_type_id', 2);
    }

    public function remove(Builder $builder, Model $model)
    {

        $query = $builder->getQuery();

        // here you remove the where close to allow developer load
        // without your global scope condition

        foreach ((array) $query->wheres as $key => $where) {

            if ($where['column'] == 'vip_type_id') {

                unset($query->wheres[$key]);

                $query->wheres = array_values($query->wheres);
            }
        }
    }
}

[bgarrison25]

Also I just read up on the new implementation of global scopes in 5.2.....can you still use traits as we have been?

[acasar]

@bgarrison25 In DependentTypeScope and MemberTypeScope you are only removing where clauses but you forgot to remove the bindings. That's why you have an extra binding with has() queries (under the hood has() query removes global scopes, but since your implementation is faulty, you end up with extra bindings).

In 5.2 you can still use traits and removing global scopes is no longer an issue. So if you upgrade you can clean up your implementation considerably.

[bgarrison25]

I tried adding in this to my remove (pulled from recordStatusActive) but still no good:

                $bindings = $query->getRawBindings()['where'];

                unset($bindings[$key]);

                $query->setBindings($bindings);

[bgarrison25]

Do you use traits the same way though...by implementing the boot<scope name>Trait method?

[acasar]

Yes it's the same.

[acasar]

Removing bindings is tricky. If a certain where clause contains multiple bindings, you can't remove them by unset($bindings[$key]);...

[bgarrison25]

I will update to 5.2 today and see if this fixes the issue. I will keep you updated

[bgarrison25]

Having an issue where updating to version 5.2 means that my route group middleware is not being called....so itll be a bit before i can post results

[bgarrison25]

I know its off topic but the my issue in upgrading to 5.2 is here if your interested http://stackoverflow.com/questions/34440256/laravel-5-2-updated-and-route-group-middleware-now-is-no-longer-called

[acasar]

@bgarrison25 Maybe it would be better to listen for Illuminate\Routing\Events\RouteMatched event?

[bgarrison25]

So instead of using middleware, I would register a listener on that event? This sounds like a pretty sound idea...I will attempt it and get back to you on whether my original issue is fixed.

[bgarrison25]

Unfortunately it seems like this error occurs BEFORE this event is fired. In fact I put a dd() right before the event is fired in Router.php and the error still occurs so im sure of it. After more testing the error actually occurs in Route.php on line 263 in the signatureParameters` method. This is where the implicit model binding happens and is JUST before that event is fired. The issue is that in order for implicit model binding to happen it needs to access the listed class / method in 'uses'. This is obviously an issue since at this point {api-version} still hasn't been removed.

[bgarrison25]

So at this point i have a whole new issue...(as stated above). Should I put this in a new issue?

[acasar]

hmm... yeah, implicit route binding tries to create controller before that event is fired. Probably worth creating a separate issue for that.

[bgarrison25]

#11496 <-- for reference.

[bgarrison25]

Okay I resolved above issue by just hardcoding in my api version (for now) and in 5.2 when I try to run members = (new \App\Models\Member)->has('dependents')->get(); I now get the following error:

SQLSTATE[HY093]: Invalid parameter number (SQL: select * from `<member table>` where (select count(*) from `<member table>` as `self_3701b7d25ec60c89e5a6f0320391a251` where `self_3701b7d25ec60c89e5a6f0320391a251`.`group_id` = `<member table>`.`vip_id` and `vip_type_id` = 2 and `<member_table>`.`record_status` = 1 and `vip_type_id` = 3 and `<member table>`.`record_status` = 1) >= 1 and `vip_type_id` = ? and `<member table>`.`record_status` = ?)

Now it seems im MISSING 2 bindings at the end.......i just can't win.

Also I updated my global scopes to no longer have removes and to use removeGlobalScopes() correctly in my recordStatusActive Scope

[acasar]

@bgarrison25 Can you open Eloquent Builder and make a manual change on this line: https://github.com/laravel/framework/blob/5.2/src/Illuminate/Database/Eloquent/Builder.php#L658

Basically replace this:

return $this->addHasWhere(
    $query->applyScopes(), $relation, $operator, $count, $boolean
);

With this:

return $this->addHasWhere(
    $query, $relation, $operator, $count, $boolean
);

Does that fix your problem?

[acasar]

And the second thing, make sure you are no longer calling extend() in the apply method of the scope. It is now called automatically if it exists.

[acasar]

@bgarrison25 Disregard my two posts above. I managed to replicate your issue and sent a PR.

[acasar]

The PR was merged. @bgarrison25 Can you check if your problem is fixed?

[bgarrison25]

awesome.....i assume were in different time zones because your always more active as im going to sleep.....Thanks again for all your help. Updating my code and testing...

[bgarrison25]

So after doing extensive testing it looks like everything is working! I am getting correct bindings on every call. Its going reallly slow....but its working and after testing the query on my database manually i realize the slow down is there. I would officially call this Closed now. Thanks again!

[acasar]

Amsome :)