[10.x] Fix nested join when not JoinClause instance

[joelharkes]

Fixes issue where Nested join would build wrong sql, when joining another Query builder on it.
it would mistakenly cut 'where ' of to 're ' expecting it to say 'on ' (which it doesn't because its not a joinClause but just a regular query builder class inside).

before:

select "users"."id" from "users" inner join "contacts" on "users"."id" = "contacts"."id" and (re "contacts"."id" = ?)

now:

select "users"."id" from "users" inner join "contacts" on "users"."id" = "contacts"."id" and ("contacts"."id" = ?)

[joelharkes]

currently have problems with this in my package development: https://github.com/joelharkes/laravel-model-joins

[taylorotwell]

Can you explain how you identified the fix and how it fixes the issue?

[joelharkes]

Can you explain how you identified the fix and how it fixes the issue?

@taylorotwell I found it when working on anther PR (that you closed without much explaining why, which really bumped me out. i understand not wanting to add the code, but i'd like a proper reason why). This fix was already in that PR, but not as cleanly: #46603

Found in usage:

Joining Eloquent Builder on another Eloquent builder

          $this->join($aliasToUse, fn (JoinClause $join) => $join
                ->on($modelToJoin->qualifyColumn($joinColumnName), '=', $baseModel->qualifyColumn($baseColumnName))
                ->addNestedWhereQuery($builderToJoin->getQuery()),  // Nested query builder inside JoinClause is where it fails
                type: $joinType
            );
            $this->applyScopesWith($builderToJoin->getScopes(), $modelToJoin);

Debugging

The addNestedWhereQuery part fails as it generates where (not on code because its not a JoinClause class, it fails here.
It checks the wrong variable. It checks $query, but $where['query'] is actually send downstream to generate the SQL.

protected function whereNested(Builder $query, $where)
    {
        // Before
        $offset = $sql instanceof JoinClause ? 3 : 6;
        // fixed solution
        $offset = $where['query'] instanceof JoinClause ? 3 : 6;
        return '('.substr($this->compileWheres($where['query']), $offset).')';
    }

Downstream `$where['query'] is the one being checked here:

  protected function concatenateWhereClauses($query, $sql)
    {
        $conjunction = $query instanceof JoinClause ? 'on' : 'where';

        return $conjunction.' '.$this->removeLeadingBoolean(implode(' ', $sql));
    }

Technically it only fails when:

• Using nested Builder in JoinClause => not very many people will see this happening but for my package this would shortcut soo much development. (not having to first wrap/cast it to a JoinClause)
• Or nested JoinClause in Builder (which i don't expect anybody doing).

[joelharkes]

@taylorotwell

It fixes the issue because it will no longer try to cut of "on " at the beginning of te string where it actually has "where " because its not a JoinClause

[taylorotwell]

Thanks.

Regarding closing the PR - I think the reason is pretty much there, although a bit terse. Every PR I merge I am now responsible for. The person who submitted it is often not around to fix any problems years down the road and we are on our own maintaining that code for the rest of my life maintaining Laravel. Therefore, my default instinct is to reject code, not accept code. I already maintain a lot of code, and there has to be a very compelling reason for me to accept new code to maintain.

When the PR is simple, it's obviously easier to merge, revert, etc.