Skip to content
This repository was archived by the owner on Feb 16, 2022. It is now read-only.

FIX trash multiple attribute types in one save #101

Merged

Conversation

michal-kusy
Copy link
Contributor

Fixes issue when trashing/replacing multiple attribute types leads to data integrity issues (deleting IDs in different/first found model) and duplicating data (especially for replace in collections).

Fixes issue when trashing/replacing multiple attribute types leads to data integrity issues (deleting IDs in different/first found model) and duplicating data (especially for replace in collections).
@michal-kusy michal-kusy changed the base branch from master to develop October 28, 2019 11:55
@Omranic
Copy link
Member

Omranic commented Sep 28, 2021

Thank you for the valuable contribution. I did a complete rewrite for the whole package and changed how this feature works. In the new refactor, it actually uses normal relationships, and should be easy and straightforward like default Laravel relationships. Although, that refactor is incomplete.

I'm still not happy with the overall performance (I believe we can reduce number of executed queries), if you want to check it out, see https://github.com/rinvex/laravel-attributes/tree/refactor-to-native-laravel-relationships

Currently no plans to merge that rewrite, but hopefully sometime I can get it to a stable state, improve performance and release it. Any help with that branch would be much appreciated! 🙂

@Omranic Omranic closed this Sep 28, 2021
@drrcknlsn
Copy link

@Omranic If there is no plan to merge your rewrite, would you reconsider opening this PR back up and merging the fix? The affected code is deleting arbitrary unrelated data, which is a pretty serious issue.

@Omranic Omranic reopened this Jan 28, 2022
@Omranic Omranic merged commit 58dd930 into rinvex:develop Jan 28, 2022
@Omranic
Copy link
Member

Omranic commented Jan 28, 2022

Done! ✅ @drrcknlsn @michal-kusy

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

Successfully merging this pull request may close these issues.

3 participants