[5.4] Eloquent BelongsTo fails with incrementing key but using non-integer non-keys on the relation

[miklcct]

When a model belongsTo another model with an incrementing primary key using a non-primary-key as the other_key, the generated query when eager loading causes SQL data type error and/or possibly yielding wrong result if the foreign key column is null.

Consider the following database structure:

• table users

  • id integer incrementing key
  • role string nullable

• table user_roles

  • id integer incrementing key
  • ctf_role string

The following code fragment demonstrate the example, where I am referencing column users.role on user_roles.ctf_role with string type.

/**
 * @property string|null $role
 */
class User extends Model {
    public function userRole() : BelongsTo
    {
        return $this->belongsTo(UserRole::class, 'role', 'ctf_role');
    }
}

/**
 * @property integer $id
 * @property string $ctf_role
 */
class UserRole extends Model
{
    public function users() : HasMany {
        return $this->hasMany(User::class, 'role', 'ctf_role');
    }
}

User::whereRole(null)->first()->load(['userRole']);

If I put a UserRole with a non-numeric ctf_role (test in the following example), the following is the result:

Illuminate\Database\QueryException with message 'SQLSTATE[22018]: [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]Conversion failed when converting the nvarchar value
'test' to data type int. (SQL: select * from [user_roles] where [user_roles].[ctf_role] in (0))'

If I put a UserRole with ctf_role set to 0, the object will be erroneously loaded to the relation.

This pull request remove the bug by always return null in the key list no matter whether the key is incrementing or not.

[taylorotwell]

Won't this break some situations? If not, why was the code there in the first place?

[miklcct]

The relevant code was added due to #13378 to convert 0 to null, but it was an incomplete fix.

It has been further modified in commit c803376 to convert more instances of 0 to null, but it was still incomplete yet.

This pull request convert all remaining instances of 0 to null to eliminate the bug.

The mentioned possible BC break in #13378 for referring to a model with 0 as the key is also a bug, as demonstrated in the code above.

[taylorotwell]

We need an integration test for this probably.