topclaudy / compoships

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

Allow nullable columns #80

Closed ChristopheB closed 4 years ago

ChristopheB commented 4 years ago

I stumbled upon some unexpected results while working on a table where a column is nullable. If I were to use the example in the tests folder, pretending vehicle_id could be null, we would have:

$allocation = new Allocation();
$allocation->booking_id = 1;
$allocation->vehicle_id = null;
$allocation->save();

$allocation->trackingTasks()->save(new TrackingTask());

And the result is fairly unexpected:

// EMPTY
$allocation->trackingTasks;
$allocation->trackingTasks()->get():
Allocation::first()->trackingTasks;

// OK, not empty
$allocation->load('trackingTasks')->trackingTasks;
Allocation::first()->load('trackingTasks')->trackingTasks;
Allocation::with('trackingTasks')->first()->trackingTasks;

After some investigations, it turns out that this line is causing this issue.

$this->query->whereNotNull($fullKey);

The reason of the presence of this line in the Laravel repository is detailed here (PR here, and an interesting note here).

I think it could be possible to drop this whereNotNull condition when not all of the values are null, something like this:

if (is_array($this->foreignKey)) {
    $allParentKeyValuesKeysAreNull = array_unique($parentKeyValue) === [null];

    foreach ($this->foreignKey as $index => $key) {
        list(, $key) = explode('.', $key);
        $fullKey = $this->getRelated()
                ->getTable().'.'.$key;
        $this->query->where($fullKey, '=', $parentKeyValue[$index]);

        if ($allParentKeyValuesKeysAreNull) {
            $this->query->whereNotNull($fullKey);
        }
    }
} 

I could make a PR for this if that new behaviour sounds good to you.

Thanks for your time, and thanks for Compoships.

topclaudy commented 4 years ago

@ChristopheB Thanks for the report. You can make a PR. I think this will introduce some breaking behaviours in some integrations. I will bump the version to 2.0

ChristopheB commented 4 years ago

Just a side note on the presence of $this->query->whereNotNull($fullKey);, as I struggled around it a bit: it is needed only when a local auto incremented primary key is used in the relationship.

In other words, it ensures the collection on a new instance without an id set is the same as the collection on the saved object with its definitive id.