Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix reordering for MorphMany relationships #513

Merged

Conversation

m7moudabdel7mid
Copy link
Contributor

@m7moudabdel7mid m7moudabdel7mid commented Jul 26, 2024

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
Copy link
Owner

awcodes commented Jul 26, 2024

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
Copy link
Contributor Author

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.

Fix reordering for MorphMany relationships
@m7moudabdel7mid m7moudabdel7mid force-pushed the fix-reordering-for-morphmany-relations branch from fc8d6c4 to b6136df Compare July 28, 2024 21:42
@m7moudabdel7mid
Copy link
Contributor Author

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 awcodes merged commit 41c6fe7 into awcodes:3.x Jul 29, 2024
1 check passed
@awcodes
Copy link
Owner

awcodes commented Jul 29, 2024

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants