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

Delete orphaned media in clean command #3419

Merged
merged 9 commits into from
Oct 25, 2023

Conversation

mbardelmeijer
Copy link
Contributor

@mbardelmeijer mbardelmeijer commented Oct 23, 2023

The use-case here is to automatically delete orphaned media objects, which also deletes them from the disk.

In database structure, it's common to use cascading deletes via foreign keys. When deleting database records like that, the Media object isn't automatically deleted due to its morphed relation. This delete action should be implemented by the developer to delete the related media objects in the deleting event, but this can be easily missed. No direct effects are visible on application level, but deleted storage objects remain indefinitely in the medias table, as well as on storage, which might infringe on GDPR requirements, as well as unnessesary storage used.

This PR adds logic to automatically delete orphaned models in the clean command. I've added it as an opt-out, but if a safer opt-in option (--delete-orphaned instead of --skip-orphaned) I've added it as an opt-in option, where --delete-orphaned needs to be included to delete orphaned models.

Happy to chat about other approaches if preferred :)

@freekmurze
Copy link
Member

Could you add a test for this?

@freekmurze
Copy link
Member

Could you also rebase with main. That should make the tests pass.

@mbardelmeijer
Copy link
Contributor Author

The tests have been added. I've changed it to an opt-in function to ensure in complex cases we aren't deleting media objects where not wanted (i.e. complex global scopes, etc).

@freekmurze freekmurze merged commit fde9246 into spatie:main Oct 25, 2023
15 checks passed
@freekmurze
Copy link
Member

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