Framework: [5.1] [5.2] addBinding adds bindings in wrong order

[ikari7789]

I'm not sure if this is really an issue or due to the complexity of the query, but the query builder no longer works as expected (for our use case) as of 5.1.20. It was working in 5.1.16. It now produces an invalid number of binding variables and throws an Illuminate\Database\QueryException. I realize this is due to the ->addBinding() calls after the complex joins near the end. They were added as the complex join was not properly adding the bindings before, but now the complex join adds the bindings into the wrong location. The first two queries merge their bindings into the 'where' portion of the bindings, but the following two queries merge their bindings into the 'join' portion, putting them before the two earlier bindings.

$inList1 = ['value1', 'value2', 'value3'];
$inList2 = ['value4', 'value5', 'value6'];
$inList3 = ['value7'];

$query1 = DB::table('table2')
    ->select('table2.id')
    ->leftJoin('table4', 'table2.table4_id', '=', 'table4.id')
    ->whereNull('table2.deleted_at')
    ->whereNull('table4.deleted_at')
    ->whereIn('table4.key', $inList1);

$query2 = DB::table('table3')
    ->select('table3.id', 'table3.table5_id')
    ->leftJoin('table4', 'table3.table4_id', '=', 'table4.id')
    ->whereNull('table3.deleted_at')
    ->whereNull('table4.deleted_at')
    ->whereIn('table4.key', $inList2);

DB::table('table1')
    ->select(
        'column'
    )
    ->join(
        DB::raw('(' . $query1->toSql() . ') table_alias1'),
        'table1.table2_id',
        '=',
        'table2.id'
    )
    ->mergeBindings($query1)
    ->join(
        DB::raw('(' . $query2->toSql() . ') table_alias2'),
        'table1.table3_id',
        '=',
        'table3.id'
    )
    ->mergeBindings($query2)
    ->join(
        DB::raw('"table4" as "table_alias3"'),
        function (JoinClause $join) use ($inList) {
            $join->on('table1.table4_id', '=', 'table_alias3.id')
                ->whereIn('table_alias3.key', $inList3);
        }
    )
    ->addBinding($inList3)
    ->join(
        DB::raw('"table4" as "table_alias4"'),
        function (JoinClause $join) use ($inList) {
            $join->on('table1.table4_id', '=', 'table_aliase4.id')
                ->whereIn('table_aliase4.key', $inList3);
        }
    )
    ->addBinding($inList3)
    ->latest('table1.created_at');

By merging inside of the join, it seems to have fixed the problem.

$inList1 = ['value1', 'value2', 'value3'];
$inList2 = ['value4', 'value5', 'value6'];
$inList3 = ['value7'];

$query1 = DB::table('table2')
    ->select('table2.id')
    ->leftJoin('table4', 'table2.table4_id', '=', 'table4.id')
    ->whereNull('table2.deleted_at')
    ->whereNull('table4.deleted_at')
    ->whereIn('table4.key', $inList1);

$query2 = DB::table('table3')
    ->select('table3.id', 'table3.table5_id')
    ->leftJoin('table4', 'table3.table4_id', '=', 'table4.id')
    ->whereNull('table3.deleted_at')
    ->whereNull('table4.deleted_at')
    ->whereIn('table4.key', $inList2);

DB::table('table1')
    ->select(
        'column'
    )
    ->join(
        DB::raw('(' . $query1->toSql() . ') table_alias1'),
        function (JoinClause $join) use ($query1) {
            $join->on('table1.table2_id', '=', 'table2.id');
            $join->bindings = array_merge($join->bindings, $query1->getBindings());
        }
    )
    ->join(
        DB::raw('(' . $query2->toSql() . ') table_alias2'),
        function (JoinClause $join) use ($query2) {
            $join->on('table1.table3_id', '=', 'table3.id');
            $join->bindings = array_merge($join->bindings, $query2->getBindings());
        }
    )
    ->join(
        DB::raw('"table4" as "table_alias3"'),
        function (JoinClause $join) use ($inList) {
            $join->on('table1.table4_id', '=', 'table_alias3.id')
                ->whereIn('table_alias3.key', $inList3);
        }
    )
    ->join(
        DB::raw('"table4" as "table_alias4"'),
        function (JoinClause $join) use ($inList) {
            $join->on('table1.table4_id', '=', 'table_aliase4.id')
                ->whereIn('table_aliase4.key', $inList3);
        }
    )
    ->latest('table1.created_at');

[GrahamCampbell]

Ping @taylorotwell.

[acasar]

@ikari7789 I was the author of the PR that fixed the join binding issue. I looked at your query and it seems to me that what you had before was just a workaround for a bug that is now fixed. AddBinding calls are no longer needed since the joins now add bindings correctly.

However, you need to merge your bindings of the join sub-queries in the join bindings themselves. And mergeBindings() doesn't really do that - it merges bindings by type. And the way you were doing it before is problematic, since it would fail if you had any "order by", "group by" or "having" statements in the subqueries.

I suggest you try this:

DB::table('table1')
    ->select('column')
    ->join(
        DB::raw('(' . $query1->toSql() . ') table_alias1'), 'table1.table2_id', '=', 'table2.id'
    )
    ->addBinding($query1->getBindings(), 'join')
    ->join(
        DB::raw('(' . $query2->toSql() . ') table_alias2'), 'table1.table3_id', '=', 'table3.id'
    )
    ->addBinding($query2->getBindings(), 'join')
    ->join(DB::raw('"table4" as "table_alias3"'),
        function (JoinClause $join) use ($inList3) {
            $join->on('table1.table4_id', '=', 'table_alias3.id')
                ->whereIn('table_alias3.key', $inList3);
        }
    )
    ->join(
        DB::raw('"table4" as "table_alias4"'),
        function (JoinClause $join) use ($inList3) {
            $join->on('table1.table4_id', '=', 'table_aliase4.id')
                ->whereIn('table_aliase4.key', $inList3);
        }
    )
    ->latest('table1.created_at');

Again, I'm sorry about your problem but I'm not sure if this really counts as a BC break. You had a workaround for a bug that existed for a long time and now you can remove it. But of course I'll let Taylor decide on this.

[ikari7789]

@acasar Thanks for the input. I also agree that I'm not sure if it counts as a BC break or not.

Hadn't thought about changing the mergeBindings() calls to addBinding() calls. It seems that would also work. I am actually preferring my suggested method though only because it removes the need to add the bindings outside of the initial join clause and encapsulates the entire query + bindings in a single join(), but the JoinClause object doesn't have any methods for adding or merging in bindings, so I could only resort to using array_merge.

Would sure be nice if there was a way to alias items other than using DB::raw().

[acasar]

@GrahamCampbell This can be closed, the issue was resolved.

[crossmaya]

same my question. thank u @acasar