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

Cascading deletion doesn't work with cascadeCallbacks if you first trash related elements. #68

Open
kolorafa opened this issue Jul 22, 2024 · 4 comments

Comments

@kolorafa
Copy link
Contributor

If you have cascadeCallbacks enabled.

The 'purge'=>true only works if the related (hasMany) elements are not trashed.

Steps to reproduce:

  1. Have 2 tables, both with Trash.
  2. First table hasMany secondTable with Dependent true and setCascadeCallbacks true
  3. delete() entity from firstTable
  4. delete(['purge' => true]) the same entity

Expected result:

Have both entity from firstTable and related entities from secondTable hard deleted (purged)

Actual result:

Related entities from secondTable are left as soft deleted (or you get constraint violation if set).

It's because the DependentDeleteHelper.php doesn't see those related elements because by default the Trash Behavior hides deleted items.

foreach ($association->find()->where($conditions)->all()->toList() as $related) {

https://github.com/cakephp/cakephp/blob/323997781a608191192066b883201a12c3c5b2f2/src/ORM/Association/DependentDeleteHelper.php#L56

@ADmad
Copy link
Member

ADmad commented Jul 22, 2024

The only solution I see for this problem is to loop through all dependent associations and disable the trash behavior (or set some flag so that TrashBehavior::beforeFind() doesn't add the condition, and then revert back the setting after the deletion is done.

You are welcome to submit a patch for that (or any other solution).

@kolorafa
Copy link
Contributor Author

kolorafa commented Jul 22, 2024

Thanks for checking it. I did also check multiple places.

I could also loop over associations and use setFinder that would set options to include trash and revert it back after delete.

But I think manually handling deletion of trashed items in Model.beforeDelete would be a cleaner solution (unless it result in some crazy event loop, but I don't think so). But best would be to somehow override cascadeDelete in DependentDeleteHelper or HasMany/HasOne

Current dirty work-around I have found is to have (separate second) associations that is only used for delete with setFinder('WithTrashed')

@ADmad
Copy link
Member

ADmad commented Jul 22, 2024

I could also loop over associations and use setFinder that would set options to include trash and revert it back after delete

Yeah that would be inline with the approach I suggested above.

But best would be to somehow override cascadeDelete in DependentDeleteHelper or HasMany/HasOne

You could extend HasMany/HasOne::cacacadeDelete() and override Table::hasMany/hasOne() to use your custom association classes, but that's an app level solution.

@kolorafa
Copy link
Contributor Author

kolorafa commented Sep 3, 2024

@ADmad
I don't think it can be fixed on the plugin level without changes in CakePHP, but correct me if I'm wrong.

  • loop over associations and use setFinder
  • loop through all dependent associations and disable the trash behavior (or set some flag so that TrashBehavior::beforeFind() doesn't add the condition

Both cases are not possible, because if delete fail for any reason, you can't undone those changes, and those changes affect all next ORM queries on that table.

So If your app have some handling for failed deletions (even printing some page with error message) - then you are f... as you could accidentally also fetch trashed entities.

You can't use Model.beforeDelete to modify Associations because Model.afterDelete might not fire and there is no other way to catch delete failures.
So the only place you can modify assosiations by overriding the delete() function with a try/catch/return value check, but that can only be done on app level, not possible from Behavior.

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

No branches or pull requests

2 participants