awcodes / filament-curator

A media picker plugin for Filament Panels.
MIT License
343 stars 89 forks source link

Fix reordering for MorphMany relationships #513

Closed m7moudabdel7mid closed 4 months ago

m7moudabdel7mid commented 4 months ago

Fixed an issue where the query builder retained conditions from previous operations, causing reorder queries to malfunction for MorphMany relationships.

Prior to the fix, this line: $relationship->whereNotIn('media_id', $newIds)->where($typeColumn, $typeValue)->delete(); Excluded records being saved ($newIds) from deletion.

Then, on line 542: $relationship->where('media_id', $itemId)->update($data); The $relationship instance retained the exclusion condition, preventing new records from being updated.

Solution:

Cloned the $relationship instance before making updates to avoid condition retention issues.

Example Query:

Before: update `media_items` set `media_id` = 1, `order` = 4, `media_items`.`updated_at` = '2024-07-26 16:54:20' where `media_items`.`mediable_type` = 'user' and `media_items`.`mediable_id` = 1 and `media_items`.`mediable_id` is not null and `type` is null and `media_id` not in (1, 2 ,3) and `type` is null and `media_id` = 1 order by `order` asc

After: update `media_items` set `media_id` = 1, `order` = 4, `media_items`.`updated_at` = '2024-07-26 16:56:47' where `media_items`.`mediable_type` = 'user' and `media_items`.`mediable_id` = 1 and `media_items`.`mediable_id` is not null and `media_id` = 1 order by `order` asc

awcodes commented 4 months ago

Haven't done much with clones. Do they need to be cleaned up since this is a livewire request? Don't want them hanging out if they aren't destroyed at the end of the lifecycle.

m7moudabdel7mid commented 4 months ago

I'm not entirely certain, but it seems unlikely that it would persist, as it's not attached to a public property that gets hydrated. But replacing (clone $relationship) with a fresh instance from $component->getRelationship() should work similarly. What are your thoughts?

However, the commit does not fully resolve the bug. The $relationship used in the loop at line 543 also needs to be fresh to avoid retaining conditions from previous iterations.

I initially used cloning correctly in the loop but removed it, assuming it wouldn’t affect other lines since it was the final operation. I didn’t realize that this line was within a loop and didn’t test thoroughly. Apologies for that.

m7moudabdel7mid commented 4 months ago

Hi,

I've updated the commit, The bug should now be fixed. Could you please review the changes and merge the pull request if everything looks good?

Thank you!

awcodes commented 4 months ago

Thanks.