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

Add cascadingEmptyTrash method… #57

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

frandss
Copy link

@frandss frandss commented Sep 12, 2022

…and modify emptyTrash method to adapt to new changes, preserving backward compatibility. Add related info to readme.

…to new changes, preserving backward compatibility. Add related info to readme.
$bindingKey = (array)$association->getBindingKey();
$conditions = array_combine($foreignKey, $entity->extract($bindingKey));

foreach ($association->find('onlyTrashed')->where($conditions) as $related) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a record is being hard deleted then all it's dependent records should be hard deleted, not just the ones which are trashed, otherwise you could end up with orphaned records. So the withTrashed finder should be used here not onlyTrashed.

*/
public function cascadingEmptyTrash(?EntityInterface $entity = null)
{
$result = $this->emptyTrash($entity);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dependent records should be deleted before parents, otherwise you will get delete failures if foreign key constraint is set on the dependent table.

* @param \Cake\Datasource\EntityInterface|null $entity to delete.
* @return bool|\Cake\Datasource\EntityInterface|int
*/
public function emptyTrash(?EntityInterface $entity = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't mix deletion of single and multiple records, it makes the API ugly. Static analyzers won't be able to detect the proper return type.

@ADmad
Copy link
Member

ADmad commented Sep 14, 2022

Besides deleting of associated records the use of deleteAll() in empty trash also causes the afterDelete callback not being run for the primary record. For e.g. if the primary record has a uploaded behavior too which is supposed to delete a file when the record is deleted then that won't happen.

So a better way to solve all problems would be to just use Table::delete(), which would ensure delete callbacks are run. So we can either have a behavior config or an argument deleteWithCallbacks for emptyTrash(). If it's enabled then we first find all the trashed records and simply pass the resultset to Table::deleteMany().

@kyleweishaupt
Copy link

For e.g. if the primary record has a uploaded behavior too which is supposed to delete a file when the record is deleted then that won't happen.`.

Just ran into this same issue today. Need to delete files only when a record has been hard-deleted.

An option to use callbacks would be great or an event like Model.afterHardDelete.

@kolorafa
Copy link
Contributor

If I understand correctly the topic, I set ->setCascadeCallbacks(true); when defining relations and just use Table::delete(), because without it the hard delete is just doing generic DELETE on database in turn behaviors can't track deletions because the app doesn't actually know what is deleted.

It would be nice to be able to set cascadeCallbacks to true by default for all relations (to avoid accidentally forgetting) so any and all related deletions trigger delete events.

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.

4 participants