Drupal gets a trashbin, and other delete goodies!

Wow, some nice ideas there, and a whopper of a patch. Not had time to review or test it myself, and I'm guessing other more active contributors than I have got an awful lot on their plates leading up to the code freeze for 4.7, currently aimed at Sun 30th Oct.

Is there any possibility this could be done in a module (via nodeapi or somesuch)? Forgive me if the answer is blindingly obvious had I taken a look at the code, I'm just not sure how many peeps are going to be able to test/review this one before Sunday!

Ho ho - I did figure that once I'd read the patch! Will try and give this a go before the weekend.

To give people a quick how-to to test this patch:

1. Apply the patch file (duh)
2. Go to http://www.example.org/updates.php and run the very last update (10-27-2005). This will create a new table 'deleted' where deleted content is stored.
3. Create some content
4. Delete some content
5. Notice that there is no prompt "are you sure you want to delete this?" it just displays a message that the content has been deleted and a link to recover it.
6. Go to administer >> trash. Here you can see the content waiting for deletion.
7. Click "recover" to recover the node back to where it was. Sweet!

Now some comments on this patch (you know this already cos we talked on IRC):

Overall it works great, and I love the functionality. +1!

There are just a couple minor things I noticed:

1. database.mysql/database.pgsql need to be updated to reflect the new deleted table
2. I would like an extra link after I've deleted content to take me straight to the trash bin, so I could preview the content before just recoviering it blindly, or delete it straight away, etc.
3. Some method of dealing with content with "sub" content. For example, if I delete a forum reply with a sub-reply, the trashbin only shows me the content of the first reply. The whole subthread is recovered (which is great!), but say some thread had a really offensive reply on one of the sub-replies so I delete the parent thread to nip it in the bud (for example). One of my mods sees the first post and is like, "Oh, that must've been deleted on accident, there's nothing wrong with that" and recovers it and now my BUY V1AGARA!!one!one" post is back. ;) Not sure if this would only be true of comments (the nested thing), maybe in preview mode display the comment + all of its sub-comments, same as the forum view does?

You also mentioned these:

1. You need to add a pager query to the admin/trash page.
2. You want to reorder the messages displayed when content is deleted.

Installed the patch last night to my test site and it's looking good. The issues others have noted do apply, but in general it's a stellar piece of work. Great stuff!

Any chance this will work with 4.6 or is this only for the lucky ones who will use 4.7?

Emailed Chad some minor nits, but otherwise it looks good in my testing.

Here's some opinions:

I think I've found one near-bug, but otherwise it looks good.

The near-bug is that when I logout and I'm the anonymous user (uid == 0), the only item on my navigation menu is "trash". That seems kind of silly. It does not seem like anonymous users should have trash access. Or at least, only if the anonymous user has been given "edit own story" access, which is itself also silly, but if some admin wants to do that. :-/ But that doesn't help much for other node types, since they will have different permissions. So it might be better to just not allow anon user access to trash at all. It works just fine with the anon user; it just looks very weird to have a menu with only trash on it. Although if I turn on anon user node/story creation, then the "create" item shows up.

Other stuff:

* You might want to make the Recover button disappear when the trash list is empty, since there is nothing that can be recovered.

* Same with the Delete Selected and Delete All buttons in the Admin screen when the list is empty.

* It seems like the preview display could be improved so that it was somewhat less geeky (words like "type: node" and "data:") but I don't have any good suggestions.

Works well. I like it. No real bugs.

• Looking over the code, there are several situations where a deletion affects more than just the data being deleted. For example, when you delete a filter format, any nodes/comments/blocks that use the format are changed to use the default format. This change is not undone when you undelete the format. Perhaps in cases like this we should avoid using the trash, and simple have a confirm screen which tells the user the action is irreversible?
• The previews needs to be properly filtered: right now they are being output as-is. This is very very very very very very very very bad. Given that the original data is destroyed and that the trash system is data-agnostic, you'll have to filter when you're assembling the preview data, and store a 'ready made' preview in the trash table. This also applies to simple stuff like e.g. roll names: they need to be check_plain()ed. See http://drupal.org/node/28984.
• If a module cancels node deletion, there is no feedback to the user about this?
• Isn't the phrasing "foo has been moved to trash" odd? "to the trash" sounds better to me.
• I'm not sure I like the way previews are displayed. "file1:" "subject:" "description4:" etc ... Wouldn't one marked-up chunk per item be better? Oh and using a blank row for spacing = evil :P.
• The link to recover a node immediately after deletion doesn't work.
• Neither 'delete selected' or 'delete all' work for me.
• The action buttons on admin/trash belong below the table IMO.
• A module should be able to provide a friendly type name IMO. Type 'node' is useless to most users; type 'blog entry' is nicer. The column name for this (root_row?) is odd too.
• The preview page needs a nicer title. It would be best to store the item's title specifically in the trash, so you can display it later. "Item title - item type (trashed)" would make a good title.
• Code ickies:
  • 'Click here' is very bad. Better would be something like "You can recover the item from the trash."
  • Missing dbprefix braces in update.php
  • $node-title (missing <)
  • Call to confirm_form() in node_multiple_delete_confirm() whose return value is not used. What is it for?
  • Usage of non-semantic <b> tags across an entire list. Set a class on the list and add some drupal.css instead (there is no reason to use <strong> in this case because the entire list is strong).
  • Missing theme('placeholder') in some delete messages (note, this also invokes check_plain, so it is essential!). I saw one for contact category deletion, possibly more.
  • Useless $c++; in poll_delete()
  • Use of uppercase tags in trash preview (not valid XHTML)
  • Incorrect usage of t() on $value: $rows[] = array('<strong>'. t($key) . ':</strong> ', t($value));. Never translate user-supplied data.
  • The indentation is off is several places.
  • $item = (count($dids) == 1) ? 'this item' : 'these items'; = bad. Use format_plural() on the entire sentence.
  • t('The %item has been recovered', array('%item' => l(t('item'), "node/$nid"))) = bad usage of t(). You should keep all literal words in a sentence together and only put the %url of the link as an argument, wrapped in url().
  • Use more whitespace to separate out logical sections inside a function.
• Code commenting:
  • I would suggest you go over the patch directly and try to identify areas which can do with more commenting. It is a complicated feature. E.g. function node_multiple_delete_confirm() has several phases, none of which are separated with whitespace or clearly described. It took me a couple of reads to figure this function out.
  • No doxygen for the system_functions?

I like the feature though ;).

Wow, what a great job you did! I gave it a try at your test site, and I _really_ like this feature.

From what I could see during a quick tryout, there is one important issue that needs to be addressed IMO (at least, I find it important). When a node with an attachment is moved to the trash, the attachment itself remains accessible through its direct url (http://www.computermercenary.com/bzr/undo/files/attach.txt). I think this should not be possible from a conceptual point of view, and from a practical point of view (for example, I often e-mail this type of urls to people, and you never know how long a site admin will wait before emptying the trash).

I'm not sure about the best way to solve this. Temporarily renaming the attachment to something difficult to guess (md5($file_name . $private_key)), and saving filename and hash in the db could be an option. Copying the file to a directory with a secret name could be another, but I don't think that's a good solution since you can't be sure that that directory is going to be allowed to be automatically created (same problem as with the 'files' directory itself).

I tried testing user deletion by adding a dummy user but then could not get to the user pages from the test admin id. The obey comments imply that something has been done in user but not much about what. There was a discussion on the email about possible meanings of deleting a user and how much should go away. What if anything of that discussion is supported by the code?

I've been testing this for hunmonk. I started with the 7 patch, now i'm starting tonight with patch 8. I noticed a lot of the same things Ste... had mentioned. I agreed with him though the "click here" wasn't a terrible thing. Formatting of the page gives info about the deleted item isn't too bad either. Just make sure previewing works properly. This is definetely something that would be a great asset to 4.7 - I find it to be immensly useful and i'm sure users of our sites would agree. It's a brilliant patch and i really like the dedication put into it so far. Using crontab it might even be possible to set a number of undo's to happen all the same time or for the trash to be emptied on a cronned basis no?

Some organizational ideas might be useful for organizing deleted items based on types (or is it already doing that?). Using merlins views we could probably organize items based on heirarchy, node-type, and various other interesting attributes.

I'm not adding +1 though until I can further test. Good work

Temporarily renaming the file also seems the best option to me. Both the original filename and the new name should be stored in the db, and the file should than be renamed again when the node is removed from the trash. Maybe we can store this info in the db as a serialized array, since a node can have multiple attachments, or provide a separate table for that (nid - filename - hash)? I didn't take a thourough look at the db structure of the trash though.

In any case, you'll need a private key to hash the filename. Form.inc already implements a private key (in drupal_get_form) like this:

if (!variable_get('drupal_private_key', '')) {
  variable_set('drupal_private_key', mt_rand());
}

Simplenews (contrib) also uses this principle, but with a more secure key:

variable_set('simplenews_private_key', md5(uniqid(rand())));

If now also the trash would use a private key, I would suggest to insert a new function into common.inc: drupal_private_key(). This way, all core and contrib modules that need this functionality, can use it. This function might look like this (setting the second argument of uniqid to TRUE even makes it more secure, I think secure enough for any application):

function drupal_private_key() {
  $key = variable_get('drupal_private_key', FALSE);
  if (!$key) {
    $key = md5(uniqid(rand(), TRUE));
    variable_set('drupal_private_key', $key);
  }
  return $key;
}

Which, for the trash, would mean:
$hash = md5($file_name . drupal_private_key());

Just my $0.02.

While working on the testsite I encountered an error @ 2006-01-30 17:04 when deleting two blocks. An empty element of type block showed up in the trash. When undeleting it I got

warning: array_keys(): The first argument should be an array in /home/apartme/public_html/bzr/undo/modules/system.module on line 1461.
warning: Invalid argument supplied for foreach() in /home/apartme/public_html/bzr/undo/modules/system.module on line 1462.

Not sure if it's because what I did or a race condition. I will try to replicate the error and provide a more detailed report.

If the browser doesn't refresh properly and you click the same 'delete' link twice*, this is what you get. Deletion of the empty item is no problem. Recovery results in the error above.

*or when someone else deletes the item after you view and before you click delete. A similar race-condition exists when recovering items: the error is then:

Illegal choice in .

hunmonk: the recover twice error is fixed.

I've been playing with blocks and I noticed that when you delete a block, then create a new one with the same description and then trying to recover the old one results in the error:

user warning: Duplicate entry 'dummy block 1' for key 2 query: INSERT INTO boxes (bid, title, body, info, format) VALUES(6, 'dummy block 1', 'This is the body', 'dummy block 1', 1) in /home/apartme/public_html/bzr/undo/includes/database.mysql.inc on line 124.

The old block is then effectively deleted.

I deleted an user and then pressed "recover" on the message.
the user was recovered but I landed at the mainpage.
it would be nice if I would be on the same screen again.

preview of deleted items:
isnt it possible to use the same functions which I use to theme, for example a node or the user profile, to theme the preview?

maybe it would be good if we could read at the trashbin who deleted which item

maybe make the table sortable. (if more then just 20 items were deleted)

for example if the deleted item is a page-node use all the (theme) functions which are used to show a page node

Speaking with hunmonk in IRC, there is no automation to clean out the undeleted items on a scheduled basis. So if you have users using the delete function a lot then you have a table growing in your database that you have to backup, etc. Also, many people abuse similier features in other programs to 'store' things that they might otherwise have been deleted or were supposed to be deleted.

So my request is to add some automation and the ability to remove deleted data after a certain user definable timeframe (or never).

This may be outside the scope for this patch but I feel it is important to consider the effects. (and no, manually running a SQL query to accomplish this is not a solution).

I really like this functionality, and I'm all for it being committed to core. BUT, a few criticisms...

1. A big -1 to the trash functions being plonked into system.module, which is already (IMO) too bloated, and is already too much of a "miscellaneous core stuff" module. This all needs to be moved: either to a new included file such as trash.inc (easy to do - maybe it can be called the trash API?); or to a new core module such as trash.module (a bit harder to do, but better IMO - then users can disable the whole thing).

2. I know this has been under review and has been refined a lot in the past few months, and that when the first patch was made, HEAD was still open to new features. But seriously, this is a major new feature: let's leave it for 4.8, and let 4.7 finally go into a proper code freeze. We have to draw the line somewhere - we can't keep sneaking more and more features into 4.7 (this has been discussed recently on the mailing lists).

take a look at drupal_goto() and drupal_get_destination()

I tried the test page and got a lot of error messages as I deleted a node:

    * user warning: Column count doesn't match value count at row 1 query: INSERT INTO deleted VALUES(83, 2, '15', 'test', 'node', 'poll', 'dummy poll 2','', 'a:1:{s:5:\"files\";N;}', 1139052679) in /home/apartme/public_html/bzr/undo/includes/database.mysql.inc on line 118.
    * user warning: Column count doesn't match value count at row 1 query: INSERT INTO deleted VALUES(83, 2, '15', 'test', 'node', 'poll', 'dummy poll 2','', 'a:1:{s:5:\"files\";N;}', 1139052679) in /home/apartme/public_html/bzr/undo/includes/database.mysql.inc on line 118.
    * user warning: Column count doesn't match value count at row 1 query: INSERT INTO deleted VALUES(83, 2, '15', 'test', 'node', 'poll', 'dummy poll 2','', 'a:1:{s:5:\"files\";N;}', 1139052679) in /home/apartme/public_html/bzr/undo/includes/database.mysql.inc on line 118.
    * user warning: Column count doesn't match value count at row 1 query: INSERT INTO deleted VALUES(83, 2, '15', 'test', 'node', 'poll', 'dummy poll 2','', 'a:1:{s:5:\"files\";N;}', 1139052679) in /home/apartme/public_html/bzr/undo/includes/database.mysql.inc on line 118.
    * user warning: Column count doesn't match value count at row 1 query: INSERT INTO deleted VALUES(83, 2, '15', 'test', 'node', 'poll', 'dummy poll 2','a:1:{i:0;a:2:{s:7:\"choice1\";s:1:\"5\";s:7:\"choice2\";s:1:\"4\";}}', 'a:1:{s:5:\"files\";N;}', 1139052679) in /home/apartme/public_html/bzr/undo/includes/database.mysql.inc on line 118.
    * user warning: Column count doesn't match value count at row 1 query: INSERT INTO deleted VALUES(83, 2, '15', 'test', 'node', 'poll', 'dummy poll 2','a:1:{i:0;a:2:{s:7:\"choice1\";s:1:\"5\";s:7:\"choice2\";s:1:\"4\";}}', 'a:1:{s:5:\"files\";N;}', 1139052679) in /home/apartme/public_html/bzr/undo/includes/database.mysql.inc on line 118.
    * user warning: Column count doesn't match value count at row 1 query: INSERT INTO deleted VALUES(83, 2, '15', 'test', 'node', 'poll', 'dummy poll 2','a:1:{i:0;a:2:{s:7:\"choice1\";s:1:\"5\";s:7:\"choice2\";s:1:\"4\";}}', 'a:1:{s:5:\"files\";N;}', 1139052679) in /home/apartme/public_html/bzr/undo/includes/database.mysql.inc on line 118.
    * user warning: Column count doesn't match value count at row 1 query: INSERT INTO deleted VALUES(83, 2, '15', 'test', 'node', 'poll', 'dummy poll 2','a:1:{i:0;a:2:{s:7:\"choice1\";s:1:\"5\";s:7:\"choice2\";s:1:\"4\";}}', 'a:1:{s:5:\"files\";N;}', 1139052679) in /home/apartme/public_html/bzr/undo/includes/database.mysql.inc on line 118.
    * user warning: Column count doesn't match value count at row 1 query: INSERT INTO deleted VALUES(83, 2, '15', 'test', 'node', 'poll', 'dummy poll 2','a:1:{i:0;a:2:{s:7:\"choice1\";s:1:\"5\";s:7:\"choice2\";s:1:\"4\";}}', 'a:1:{s:5:\"files\";N;}', 1139052679) in /home/apartme/public_html/bzr/undo/includes/database.mysql.inc on line 118.
    * user warning: Column count doesn't match value count at row 1 query: INSERT INTO deleted VALUES(83, 2, '15', 'test', 'node', 'poll', 'dummy poll 2','a:1:{i:0;a:2:{s:7:\"choice1\";s:1:\"5\";s:7:\"choice2\";s:1:\"4\";}}', 'a:1:{s:5:\"files\";N;}', 1139052679) in /home/apartme/public_html/bzr/undo/includes/database.mysql.inc on line 118.

• When deleting items from the trashbin, their title(s) should be shown. Sort of like node mass delete used to do.
• When doing a mass delete, the resulting messages are a bit weird:
  • Item 2 moved to the trash.
  • You may recover the item from the trash.
  • Item 1 moved to the trash.
  • You may recover the item from the trash.

  It's not immediately clear which recovery link relates to which item. IMO either the recover/trash message should be part of the "/item/ has been moved to trash", or there should simply be a single link "You may recover any of these items from the /trash/."

• The preview is still a bit icky. IMO for things such as nodes and comments we should just have a single pre-rendered chunk, i.e. the output of node_view or something.
• Also filter-wise, while it is certainly safe to call filter_xss(), it is not ideal: it assumes the content was written in HTML. If the content was written in BBCode for example, the tags would show up literally ([b]foo[/b]) and HTML-like stuff (e.g. a comparison 'a < b') would get mangled. There is no real reason not to convert the data when you trash it, so it would already be safe to output when it comes out of the database. The watchdog also works this way for example for the error messages.
• Weird bit of code (!0) in node_multiple_delete_confirm():
  

  $nids = isset($edit['nodes']) ? array_keys($edit['nodes'], !0) : array();
  

  It's equivalent to TRUE, it looks like this is what is intended.

• The messages for recovery seem inconsistent. Sometimes you have "The item has been recovered", sometimes it is "The item has been recovered". This is especially weird if you recover a node, because you end up on the node view, with the message linking to the current page.

  It also seems weird to have a bunch of node specific logic in there. Wouldn't it be possible for example to pass an (optional) URL to the item's view when you call system_trash()?

• SQL injection! The DIDs are passed straight from the menu callback's arguments (i.e. the url) into the SQL query with:
  

    // Grab all data for all the specified packages, compose into a master array
    $all_dids = implode(', ', $dids);
    $result = db_query("SELECT * FROM {deleted_data} WHERE did IN(%s)", $all_dids);
  

  The IN(%s) construct is suspicious. The good approach is to make a %d,%d,%d,%d string in the query. Unfortunately it can be annoying to construct it due to a lack of an array_repeat() function. There are other cases where IN(%s) is used and it isn't unsafe, but it would still be better to change that.

• Another tricky SQL query:
  INSERT INTO {%s} (%s) ...

  The prefixing is applied before the %s values are filled in, so tablespecific prefixing won't work. In this case you can safely insert the literal value: it is a table name that was supplied by the deleting module.

• The preview title is a bit odd:
  'Preview: %title, Type: %type -- (trashed)'
  Perhaps better:
  'In the trash: %title (%type)'

  There is also a problem with the type (description column): it needs to be translatable and human-friendly. But, it is stored on delete, so we can't just call t() then: it would be stored in the langauge of the person who deleted it. More annoyingly is that most strings (e.g. friendly node type name such as 'blog entry') are only returned in translated form by the functions responsible.

  I think as long as we can ensure 'description' only contains a fixed set of possible values, we can call t() on output and store the english type name in the database.

Remove drupal_goto from _submit functions. was commited
so I think you can remove some of your drupal_goto() calls

hello hunmonk,

I'm on admin/block of your testsite and deleted a block.
the block was moved to the trash - as it should be, but I stay on admin/block/delete/8, see the message "The block dummy block 1 has been moved to the trash. You may recover the item." and under the message there is "admin/block" written...

another small error
when someone goes to a trash-page which does not exist (like trash/preview/334545) you can read "Trash preview: ()..." but you should get a 404 filne not found...

I've tried the patch and after fixing a small omission it seems to work as advertised. I was able to "delete" and restore a node. All settings seems to be restored.

I have also had a (short) look at the code, and not everything I see there is to my liking.

Especially the fact that this patch is not very modular puts me off. there would be no way to disable this functionality (which is nice to have but which I didn't need over more than four years of Drupal). I think this patch could make better use of our hook system. if you had a trashbin module, gave it a low eight, it would be run before all other modules and could intercept all delete calls from all hooks. We could add hooks for blocks and other items that don't have them yet.

+ $message = t('The block %name has been moved to the %trash.', array('%name' => theme('placeholder', $box['info']), '%trash' => l(t('trash'), 'admin/trash')));

Use of placeholder isn't needed as this is a static string ('trash'). And all the words (including 'trash') need to be in the first t() call, as documented at http://api.drupal.org/api/HEAD/function/t.

On language, lets keep "trashbin" out of outputted strings. Instead lets decide on "trash bin" or simply "trash."

Trash is a page under admin/ which all logged in users have access to. I'm not sure if this is a good place, but I can't think of anything better.

The word "Trashed" seems a bit weird. Maybe the column headers could be 'Date deleted' and 'Deleted by'.

The delete package API concept is a bit confusing to me. When is a package completed and the operation done? Why would I want to pass only some arguments at a time? What are the best examples of the changed code?

It looks like all of the confirm pages are being taken out, which is good.

I just wanted to know how things are going with integrating the trashbin patch to Drupal, sorry for my impatience, but I'm just curious...

Looks like a great idea- just trying out the test site. Trying to delete a comment produced this error:

Fatal error: Call to undefined function ctype_digit() in /usr/home/hunmonk/bzr/undo/modules/comment.module on line 925

hello hunmonk, we are near the 100 comment mark :D

at update.inc you can remove TYPE=MyISAM

Thanks Tobi

50+ patch revisions
100+ comments
What's left to fix?

I have not tested your patch on my local system
but I did a look at your change at update_finished_page()

// The deleted table is cleared here to prevent stale data from being reinserted between db updates
db_query('DELETE FROM {deleted}');

does this mean every time I run update.php all my datas in trash will be deleted?
Is this mentioned somewhere so that the user can decide if he really wants do make the update, now?

I hope I did not understand something the false way... :/

Not a comprehensive review, but some comments:

• drupal_set_message(t('The item was not recovered'), 'error');
  This is a rather unhelpful error message. As far as I can tell, the only time this message appears is after this:

  +        drupal_set_message(t('%title cannot be recovered due to dependency errors. Check the errors and recover any data this
  +          package depends on before attempting recovery.', array('%title' => theme('placeholder', $title))), 'error');
  

• system_trash_recover(): the hack for $node->files is rather ugly

Code:

• The update.php delete wipe should be same location as cache wipe.
• 'The language %locale has been moved to the %trash.' You're breaking up the sentence near the end with %trash. Same comment as "the %item has been moved" before.
• system_trash_recover(): due to $all_data = array(), the isset($all_data) if will always evaluate to true.
• Some lines are wrapped in code or strings.

The following two concerns from february (#49) still apply:

• The preview is still a bit icky. IMO for things such as nodes and comments we should just have a single pre-rendered chunk, i.e. the output of node_view or something. I'd even extend this to everything: let the calling module create a nice pre-assembled preview. Key => value pairs are for programmers, not for users.
• Also filter-wise, while it is certainly safe to call filter_xss(), it is not ideal: it assumes the content was written in HTML. If the content was written in BBCode for example, the tags would show up literally ([b]foo[/b]) and HTML-like stuff (e.g. a comparison 'a < b') would get mangled. There is no real reason not to pass the data through the filter system when you trash it, so it would already be safe to output when it comes out of the database. The watchdog also works this way for example for the error messages.

My problem with the trashbin patch is the way it works. I'm not convinced that it should transfer deleted records to a dedicated table. Right now, the trashbin patch stores deleted records in a separate table in serialized format. This is a little bit awkward as it has to circumvent the auto increment IDs or the sequence table when re-injecting the data. It's both ugly and clever, but at the end of the day, it doesn't feel right to me.

Why don't we add a status-field with two states -- STATUS_ACTIVE and STATUS_DELETED -- to each of the database tables with records that want to take advantage of the trashbin functionality? Then, the trashbin patch might be a lot easier to grok -- and from an architectural point of view, less awkward. It wouldn't be particularly clever but that is OK: it is super-easy so there is no point being clever to begin with. I'd like to see us explore this path instead.

Plus, this has a number of advantages. Most of all, we'd still able to query deleted data. For example, the filter form on the administer content page (?q=admin/content) would allow us to access delete nodes and we'd be able to use advanced query methods like 'show all deleted nodes from user Joe with the taxonomy term 'Apple'. That is, we get to reuse a lot of the existing UIs and modules.

@Dries- in this case would there be a difference between "deleted" and "unpublished"? Or, why not make greater use of the node status field? E.g. :

status == -1 == STATUS_DELETED
status == 0 == STATUS_UNPUBLISHED
status == 1 == STATUS_PUBLISHED

or, alternately:

status == 1 == STATUS_INMODERATION
status == 2 == STATUS_PUBLISHED

Sorry to add nothing constructive at all to this technical discussion, but just want to give my two thumbs up for anything that will make the Drupal deletion workflow more user friendly and less destructive.

I agree with both Dries and Hunmonk; the current solution doesn't "feel right" but doing it the other may complicate things. On the other hand; it would probably be better to do it "Dries way" with the future in mind.

looks very, very nice, but I managed to generate an error on the test site by deleting a node w/ a signup:

* Dependency error: value '3' for key 'uid' missing in table users
* story cannot be recovered due to dependency errors. Check the errors and recover any data this package depends on before
attempting recovery.
* Recovery of node #17 (story) unsuccessful

I'm not sure about that explanation. I did not delete any users, only the node with the signup.

There was an intermediate confirm imposed by the signup module, but the message it presented was a little unclear. So, maybe some data was permanantly lost there during the trashbin procedure?

What's odd is that I could preview the recovered node with no problem. Is this dependency checking done when the node is put is the trash or only when you attempt the recovery?

OK- sounds like I got lucky! One suggestion- I initally thought the error message was a PHP/SQL error because of the styling and color. My only other suggestion would be to make the message look more different from these.

I don't remeber whether there was a heading above the error messages- maybe just making is clear that it's an error from the trashbin would help.

on your test site at http://undo.xcarnated.com/admin/content/trash

Fatal error: Call to undefined function: node_get_name() in /usr/home/hunmonk/bzr/undo/modules/system/system.module on line 1582

Continues to look good- should certainly be included in HEAD.

minor bug (not this patch):

Call to undefined function: node_get_name() in /usr/home/hunmonk/bzr/undo/sites/all/signup/signup.module on line 261

Ok, found waht I think is a bug related to filename collision in attached files.

to reproduce:

create a node, and attach a file. Delete this node (-> trashbin)

create another node, attach a file with the same name but different content.

recover the first node from the trashbin- the content of the linked attachment is the content of the file attached to the second node.

The upload module has a mechasim for avoiding name collisions, though i'm not sure whether that relies on a check of the database or actually checking the directory.

Hmm, after doing this the test site is only giving 403 and 404. Hope that's you working on it!

I think the problem happened at the recovery stage- the 2nd node was never deleted, but had an attachment with a name identical to the attachment on the deleted node. The 2nd node was created after the 1st had been trashed.

Check out the following:

http://undo.xcarnated.com/node/91
http://undo.xcarnated.com/node/92

On node/91 the file I uploaded contained the string "the original file". You can see now that it links to a file with the string "the changed file". Also, the listed file size is now wrong on /node/91.

Looks like the checking for name collisions during uploads is done in the function file_copy.
It uses the PHP function file_exists(). so, maybe a zero-length file (or a file containing a string like "placeholder for a file in the trash bin") could be left in place of the one moved to the trash bin? I'm not sure, but maybe this placeholder file could be chmod 000 or 222 so that file_exists() would return TRUE(?), but the file would not be accessible?

I can't access http://localhost/drupal/bzr/undo ;)

{listen mode: on}

In my very naive opinion, this is sorely missed in the core.... I just came across it and would love to implement it, but I can't as it's broken....
Any ETA on a delivery in the core?

Oh, and the stupid dipsht that I am, I didn't read the first page and was wondering what would happen if I deleted the test user on the test site.

I'll offer my left kidney, if you'll forgive me....
[sorry]

That's a real shame - it was just great!
Without it, administering my community-written book is going to be a nightmare!

Anyway, congratulations on your effort - I read all the posts here, and it seems like it was a nightmare!

Let's at least hope this will be looked at again when the code opens up for 6.x. Am I remembering right that one discussion was improving the deletion APIs so that this (or alternative) modules could be added as contrib?

Any interest in this still? Would need to be rerolled at least...

Susurrus: it was moved to here: http://drupal.org/node/147723

Add related node to sidebar to prevent needing to scroll to bottom to see what this was marked as a duplicate for.