topclaudy / compoships

Multi-columns relationships for Laravel's Eloquent ORM
MIT License
1.12k stars 130 forks source link

Updating to 1.1.17 from 1.1.16 breaks #74

Closed SlyDave closed 4 years ago

SlyDave commented 4 years ago

Running 1.1.16 everything works fine.

Updating to 1.1.17 results in: ErrorException explode() expects parameter 2 to be string, array given

Which appears to be occurring @ vendor/laravel/framework/src/Illuminate/Database/Eloquent/Relations/Relation.php:319

It's as if the custom logic provided by compoships stops working and it reverts to using eloquent's implementation...

namespace App;

use Awobaz\Compoships\Compoships;

class MyModel extends Model
{
    use Compoships;

// ...
}
SlyDave commented 4 years ago

Code in our project that it's breaking on is a fairly complex chain of filtered with statements and some scopes, but nothing out of the ordinary.

All of the child models have the Compoships trait too.

    $assignments = UserVesselAssignment::with(
        [
            'user.latestHistory'      => function (HasOne $history) use ($date) {
                return $history->forMonth($date);
            },
            'latestUserVesselHistory' => function (HasOne $history) use ($date) {
                return $history->forMonth($date)->with('department', 'departmentPosition');
            },
        ]
    )->forVessel($vessel)->forMonth($date)->get();
SlyDave commented 4 years ago

It's the Laravel key optimisation not working with compound keys, it appears the changes in 1.1.17 are not working correctly? (at least for relationship conditions) from what I can tell...

The Laravel 7 key optimisation:

    protected function whereInMethod(Model $model, $key)
    {
        return $model->getKeyName() === last(explode('.', $key))
                    && in_array($model->getKeyType(), ['int', 'integer'])
                        ? 'whereIntegerInRaw'
                        : 'whereIn';
    }
SlyDave commented 4 years ago

The removal of addEagerConstraints() from src/Database/Eloquent/Relations/HasOneOrMany.php is how the breaking is occurring.

As without that method is calls the parent version, which in turn calls whereInMethod which has the last(explode('/', $key)) as noted above.

Unlike the BelongsTo, which is working because it has a addEagerConstraints() override

topclaudy commented 4 years ago

Hi @SlyDave Can you PR the fix?

SlyDave commented 4 years ago

I've added a version of whereInMethod() to HasOneOrMany that works with compound keys, delegating to the parent if key isn't an array. See PR for details.

topclaudy commented 4 years ago

Please check 1.1.18. Thanks for the contribution.