wintercms / winter

Free, open-source, self-hosted CMS platform based on the Laravel PHP Framework.
https://wintercms.com
MIT License
1.31k stars 188 forks source link

Inconsistent behavior between model.relation.afterAttach and model.relation.afterDetach #1144

Closed goldmont closed 4 weeks ago

goldmont commented 1 month ago

Winter CMS Build

1.2

PHP Version

8.1

Database engine

MySQL/MariaDB

Plugins installed

No response

Issue description

Hi,

I noticed a difference in how model.relation.afterAttach and model.relation.afterDetach events behave. It sounds more like a bug than something wanted.

I have a custom trait attached to my model and two listeners: one for afterAttach and another for afterDetach:

trait NotifyChanges
{
    public static function bootNotifyChanges()
    {
        static::extend(function ($model) {

            $model->bindEvent('model.relation.afterAttach', function (string $relationName, array $attachedIdList, array $insertData) use (&$model) {
                traceLog("New relation {$relationName} was created", $attachedIdList);
            });

            $model->bindEvent('model.relation.afterDetach', function (string $relationName, array $detachedIdList) use (&$model) {
                traceLog("Relation {$relationName} was removed", $detachedIdList);
            });

        });
    }
}

In my API I simply do the following:

$task->labels()->sync(array_unique($data['labels'], SORT_NUMERIC));

This is the output:

[2024-06-10 10:44:19] development.INFO: New relation labels was created  
[2024-06-10 10:44:19] development.INFO: Array
(
    [0] => 8
)

[2024-06-10 10:44:19] development.INFO: New relation labels was created  
[2024-06-10 10:44:19] development.INFO: Array
(
    [0] => 1
)

[2024-06-10 10:44:19] development.INFO: New relation labels was created  
[2024-06-10 10:44:19] development.INFO: Array
(
    [0] => 6
)

[2024-06-10 10:44:37] development.INFO: Relation labels was removed  
[2024-06-10 10:44:37] development.INFO: Array
(
    [0] => 1
    [1] => 6
    [2] => 8
)

The afterAttach listener is invoked for every new relation; instead, the afterDetach listener is invoked only once and it receives an array of ids as expected. I made some mistake in my configuration or is it a bug? Thank you for your efforts as always.

Steps to replicate

  1. Setup afterAttach and afterDetach listeners.
  2. Use sync method.

Workaround

No response

goldmont commented 1 month ago

Forgot to say... labels relation is a morphToMany

UPDATE: Reading the source of BelongsOrMorphsToMany, I saw the following:

If a custom pivotModel is being used the parameters will actually be string $relationName, mixed $id, array $attributes

I'm not using a custom pivot model because I haven't either specified it. This is the configuration of my relation:

public $morphToMany = [
    'labels' => [
        Label::class,
        'name' => 'labelable',
        'table' => 'company_management_labelables',
        'timestamps' => true
    ]
];

UPDATE 2: Apparently Morphs are using a default Pivot model if not specified. That's why I'm getting a single ID at a time instead of all IDs. I don't know how it can be improved at the moment (if it can be).

mjauvin commented 1 month ago

This is how Laravel implemented sync(), not a bug.

goldmont commented 1 month ago

How can I determine which labels have been removed and which ones have been added? My task involves notifying task assignees that certain labels have been removed while others have been added. The sync() method provides an array of detached IDs via afterDetach, but it only gives one ID at a time for attached ones. Should I collect them individually, or is there a more efficient method?

goldmont commented 4 weeks ago

Hi there,

I forgot to get back to you. Ultimately, I decided to manually diff the two collections and use attach / detach directly. For anyone who needs this:

$ids = $task->assignees()->pluck('id');
$newIds = collect(array_unique($data['assignees'], SORT_NUMERIC));

if (!empty($toDetach = $ids->diff($newIds)->all())) {
    $task->assignees()->detach($toDetach);
}
if (!empty($toAttach = $newIds->diff($ids)->all())) {
    $task->assignees()->attach($toAttach);
}