I had a very good conversation with dopry and hunmonk on IRC today trying to figure out how to cleanly add files to the deletion API.

The motivation boils down to: there are only two things you can really delete on a webserver, files and rows in a database. Right now the API only supports the former. Moving the files into the deletion API provides a uniform way for deletions to be managed and will improve the work we're doing on the file API on #142995.

The attached patch adds a new function drupal_delete_add_file() which lets you add a file to a deletion package. When _drupal_delete_execute() is called it deletes the files using file_delete().

Hunmonk had an idea to add a new "condition" type of object which sounded good but a bit too complex for me. I'm going to post this patch to get his feedback, and then see if his system works out better.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hunmonk’s picture

posting the following IRC convo between drewish and i, as i think it accurately describes how i feel about this patch:

hunmonk: at this point, what is the big difference between between the post deletion callback and the files patch you posted?

drewish_again: the problem with that is that it's hard to see what else is being changed and it will make a mess of our patch to add hook_file. when file_delete deletes the rows from the database it's going to be a problem. ideally both work work together. dopry's feeling we just screw it and try to get hook_file committed. i wanted to try to have files and the delete api working together.

hunmonk: ok, here's what i say -- i looked at your patch. it's fairly straightforward. it makes a certain amount of sense that files and database deletions are the two basic deletion types. i'm not seeing how it's vastly superior to what i've already done, and what's already available, but i also don't understand the file stuff like you do. so, i won't hold that patch back. we can let the higher ups decide, or at least go back and forth a bit on the issue so they have enough information to decide if it's worth it to put in

drewish_again: i agree that it can be done within the current framework. my feeling is that it's a hack though. i can understand the fear of a slippery slope of all sorts of delete special cases but i really can think of anything deleteable other than files and db rows. you're right that at this point it's not a big improvement, but it's a good ground work.

drewish’s picture

re-reading that transcript i realize i had a few typos. that last sentence should have read:

...i really can't think of anything deletable other than files and db rows...

i'd love to get a couple of reviews, it's an easy patch to test: just enable the upload module, attach some files, to a few nodes with different revisions, delete them and make sure the files and rows get deleted...

moshe weitzman’s picture

Seems reasonable to me. Files are now first class objects.

hunmonk’s picture

FileSize
4.31 KB

had more time to review this patch, and i'm now in full support of it. tested and works perfectly. attached patch makes a minor doc change, and more importantly, adds a user/watchdog messages if file deletion fails.

drewish’s picture

Status: Needs review » Reviewed & tested by the community

retested hunmonk's patch and it's good to go.

hunmonk’s picture

FileSize
4.33 KB

one tiny correction -- watchdog doesn't use t() anymore, so i fixed that up. still ready to rock!

drewish’s picture

Priority: Normal » Critical

I'm reluctant to mark this as critical but it's blocking the work on #154604 and the code freeze is swiftly approaching.

drewish’s picture

i'm an idiot, i meant to say that this is blocking #142995.

Frando’s picture

The approach is sounding really good. Tested, and files are correctly deleted when a node is deleted. RTBC +1.

webchick’s picture

Status: Reviewed & tested by the community » Closed (won't fix)

The deletion API was rolled back.