search

Askedio / laravel-soft-cascade

Cascade Delete & Restore when using Laravel SoftDeletes
https://medium.com/asked-io/cascading-softdeletes-with-laravel-5-a1a9335a5b4d
MIT License
705 stars 63 forks source link

Already deleted items should not be in the affectedRows when #98

Closed tkempf closed 5 years ago

tkempf commented 5 years ago

softcascade should not change deleted_at timestamps of already deleted childmodels, becaus this could break unique Indexes on pivot tables. E.g. i had a parent model wich contains n child models at position 0..n. to ensure each position is only user once for every parent i created a unique index over the columns parent.id,position and deleted_at in the pivot table. when deleteing the parent Model softcascade tries to change all of the related childmodels to the same deleted_at timestamp, which will not work with the unique index. As i can't see no use in changing all the timestamp of already deleted items, i made this PR Kind Regards Tom

maguilar92 commented 5 years ago

@tkempf could you create / update tests for this scenario?

tkempf commented 5 years ago

I'll see what i can do, but i will be on holidays for the next 3 weeks now. BTW. i ran your tests and testDeleteQueryBuilder fails here ... There was 1 failure:

1) Askedio\Tests\IntegrationTest::testDeleteQueryBuilder Failed asserting that a row in the table [profiles] does not match the attributes { "deleted_at": null }.

Found: [ { "id": "1", "user_id": "1", "phone": "1231231234", "created_at": "2019-08-07 05:52:46", "updated_at": "2019-08-07 05:52:46", "deleted_at": null } ].

maguilar92 commented 5 years ago

@tkempf I will review it and merge on laravel 6.

maguilar92 commented 5 years ago

@tkempf It will be accepted today, sorry for delay.

tkempf commented 5 years ago

no Problem. I'm not in a hurry ;-) btw. i think this pr should solve issue #81, or am i wrong ?

maguilar92 commented 5 years ago

This PR does not solve problem #81 because the restore will restore all deleted items instead of items deleted by the item being restored.

tkempf commented 5 years ago

ah, i missed that one. should read better next time ...

maguilar92 commented 5 years ago

No problem, thanks!

maguilar92 commented 5 years ago

@tkempf that change is available in version 6.0.1.