Drupal gets a trashbin, and other delete goodies!
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | duplicate |
ok folks,
attached is the undo/trashbin/modified confirm patch, which provides major enhancements to drupal's delete cycle, including:
- a trash can, into which deleted items are replaced for later recovery if desired.
- elimination of unnecessary confirm screens.
- conditional confirm screens which modules may request core to inject on any node type delete cycle, via a new 'delete pre' nodeapi op. on this confirm screen, modules may display custom messages, forms, control structures, and even prevent deletion of nodes if necessary.
couple of things about the trashcan feature:
- it's recursive, meaning that if you delete a node, all associated info is also place in the trash bin, and if the node is recovered, all of the related info is also recovered
- it can be used by non-node type modules as well
to test the patch, you'll need an up to date HEAD, and you'll need to run update.php and install the update from 2005-10-27. to try out the trash can, just create some stuff with the core modules and then delete it. you'll 'administer nodes' permission to access the trashbin at admin/trash
here's a list of all the core modules that now make use of this functionality:
locales.inc, block, book, comment, contact, filter, forum, locale, node, path, poll, profile, statistics, taxonomy, upload, user
and a list of core modules that have delete queries in them, but didn't seem like they would fit with converting them to this system.
i'll post doc within the next 24 hours detailing the use of the 'delete pre' hook, and how to convert a module to make use of the new features--it's quite easy... :)
i'd love to get feedback asap--my goal is to get this in before the 4.7 code freeze!
| Attachment | Size |
|---|---|
| undo.patch | 54.85 KB |
#1
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!
#2
because of the way node.module handles deletion, i don't see a way to put this functionality into contrib. it's core or bust... :)
#3
Ho ho - I did figure that once I'd read the patch! Will try and give this a go before the weekend.
#4
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.
#5
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.
fixed these. note that only someone w/ the 'administer nodes' perm can access the trashbin--otherwise there's all kinds of crazy perm stuff i'd have to do with the trashbin... :)
1. You need to add a pager query to the admin/trash page.
also fixed.
i'll get to the other stuff tonight!
#6
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!
#7
Any chance this will work with 4.6 or is this only for the lucky ones who will use 4.7?
#8
ramdak: sorry, it's a core patch. only for 4.7+
#9
ok, steven gave me lot's of homework on this patch, and i've completed it all... :)
upgrades in this version:
once again, to test, you'll need install the patch in a current version of HEAD, and you'll need to install the db update dated 2005-10-27. i made some changes to the db structure since the last patch, so it would be best to just wipe it clean...
any thoughts feedback would be welcome!
#10
Emailed Chad some minor nits, but otherwise it looks good in my testing.
#11
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.
#12
attached is an updated patch. the node delete rebuild broke the last one :)
i also noticed that the link for recovering an item right after trashing is not working properly due to the last round of updates that i made to the functionality. i'll need to abstract the recover code to get that one back.
last big task remaining: validating the data upon recovery. this will require some code that checks for dependent data in the tables and prevents recovery of data that is now stale. i'm hoping that this won't be too big of a deal--we'll see.
i'll get to that soon, and also address some of the other minor things folks have mentioned.
#13
just noting one more thing i thought of that needs added to the patch. the deleted table should be wiped clean anytime somebody does a database upgrade. this will be necessary because any data in the deleted table might become stale after such an upgrade. we can put a little warning message on the update page if necessary.
#14
just keeping up w/ some HEAD changes. i'm hoping to get some more work done on this in the next two weeks. in the meantime, this patch works w/ latest HEAD.
#15
another update to keep current w/ HEAD.
#16
and ANOTHER update--the core changes break my patch almost faster than i can fix it!! :)
incidentally, this patch was generated via my new bzr local repo system--if anybody has problems applying it, please let me know. it applied just fine for me to a clean cvs install.
#17
took care of the rest of the small items mentioned in previous comments, including:
also, cxj and i worked out a battle plan for the last major hurdle for this patch--a way to test the data integrity of recovered items. this is going to require a simple database dependency system--shouldn't be too big or hard to build.
#18
oop. missed one thing in that last patch. here's the right one :)
#19
$node-title(missing<)<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).$c++;inpoll_delete()$rows[] = array('<strong>'. t($key) . ':</strong> ', t($value));. Never translate user-supplied data.$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().I like the feature though ;).
#20
lots of stuff fixed in this version of the patch. most of steven's concerns above have been addressed. this version also takes care of the last remaining problem with the feature, which was the need to check table dependencies upon restoration of data, to prevent any bad data from being inserted.
todo items left:
let me know how it works!
#21
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).#22
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?
#23
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
#24
DriesK: good catch. i went through great pains to get the upload section to behave well, but i guess i missed that :) i would think renaming the file would be the best bet, but am open to suggestions. you obviously can't delete the attachment until it's been removed from the trash, so we have to figure out some way to preserve it.
yktdan@drupal.org: you couldn't get to users b/c i forgot to give the test account admin access :) i've fixed that--the test account now has full admin access--try again!
#25
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:
<?phpif (!variable_get('drupal_private_key', '')) {
variable_set('drupal_private_key', mt_rand());
}
?>
Simplenews (contrib) also uses this principle, but with a more secure key:
<?phpvariable_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 ofuniqidto TRUE even makes it more secure, I think secure enough for any application):<?phpfunction 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:
<?php$hash = md5($file_name . drupal_private_key());
?>
Just my $0.02.
#26
DriesK: this sounds like a great idea. fortunately, we don't need a new table to do any of this--i've already implemented a hook system for the trashbin that allows any module to store extra data upon deletion, and access it upon recovery, and i have a spot for them to put data... :D
i'll add this to my quickly shortening TO DO list :)
it should be a breeze to implement into the current structure.
#27
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.
#28
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 .#29
Heine:
pretty sure i fixed the error on multiple attempts at recovery (the array_keys error)--can you test it out on the test site?? the 'illegal choice' error is not related to my code, i don't think--that's a forms api validation issue. basically, if a choice is deleted by user, and another user tries to submit the original form w/o refreshing the page first, it will result in a validation error.
#30
for anyone interested in helping to test this patch, you can visit the test site, which is a clean drupal install updated w/ latest patch changes. i figure this will be more efficient than posting eight million patches to this thread... :)
if anybody wants to test on a local install, then lemme know any i'll toss up a current patch.
#31
DriesK:
ok, i implemented the automagical file renaming for attachments--works beautifully! i elected not to insert the drupal_public_key function, as i decided this was out of scope for the current patch. i can always amend the code to use the function if it ever gets added.
Steven:
i've taken this patch as far as i can without further feedback, and i'm feeling good about it. to address a few of the remaining issues you raised:
i've left all the calls to system_trash in for the moment. if we decide the trashbin feature is inappropriate for any particular situation, then it's easy to change back
the preview code works quite well structurally. if anybody else wants to step up and prettify the display, please do--my html/css sucks... :)
i disagree. i think they belong at the top just like the filters/buttons in admin/node and the project search form.
i can't find this anywhere--can you point it out specifically?
all of your other concerns above have been addressed. please let me know if there's anything else i can do to get this sucker ready, and check the test site if you want to play with the functionality.
#32
Steven:
also, attached is the latest patch for your code examining pleasure :)
#33
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.
#34
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?
#35
maybe it would be good if we could read at the trashbin who deleted which item
#36
maybe make the table sortable. (if more then just 20 items were deleted)
#37
not sure what you mean here--i'm using theme_table to generate the table. i'm not much of a themer, so i'll leave that to others. it is important to note, though, that the preview data isn't node data at all, but a prebaked summary that's generated at the time of deletion.
i like your other suggestions and have added them to my to do list :)
#38
for example if the deleted item is a page-node use all the (theme) functions which are used to show a page node
#39
but it's not a node anymore, or even remotely constructed like one. i don't think that would work.
#40
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).
#41
sepeck: there's already a 'Delete all' button, which will empty the trash--so you're not at a total loss for maintenance. but i like the cron idea, and i'll be adding it shortly...
#42
sepeck: ok, your trash auto-delete feature has been added :)
i put it under admin/settings in the 'Site maintenance' group. simple dropdown menu with about 7 different options, including 'Never'
you can check it out on the test site.
#43
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).
#44
jaza:
#1: i spoke w/ dries about where to put the functions, and he told me system.module--you'll have to take that up w/ him... ;) we can't put it into a module to be disabled, b/c the feature can't be turned on and off--it would be a tremendous pain in the ass to make the feature optional, and i don't see the point in it anyways, as it's superior in every way to the original approach. a .inc file would probably be fine, but i'll leave that to others to decide...
#2: the feature has already been slated to go in early in the next devel cycle--this was decided months ago.
#45
ok, added some new features, which are now on the test site and included with this patch:
this is actually a bit tricky to pull off, and i haven't figured out how to do it yet. i see no reliable way to obtain the relative url at the time of deletion. i do support return to nodes upon recovery, which is probably the most important thing...
#46
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.
#47
i did, and i don't think they will work. for example, when a node or a user is deleted, there's a redirect _before_ you get to the delete code, so the original url of the page is no longer available at that time. i'm really not excited about the idea of refactoring a bunch of code to support this feature, especially when the most important thing, nodes, is already pretty well supported
yes, it would certainly help if i updated my database tables on the test site after i add a column in the code :P it's working now.
#48
more exciting news!
i've set up the test site to demonstrate the 'conditional delete' feature in this patch. further information is under the update section of the main page there.
#49
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/."
$nids = isset($edit['nodes']) ? array_keys($edit['nodes'], !0) : array();It's equivalent to TRUE, it looks like this is what is intended.
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()?
// 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.
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.
'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.
#50
attached patch corrects all of the above issues. i think this baby is ready for the big show. :) test site is updated w/ latest changes.
#51
Remove drupal_goto from _submit functions. was commited
so I think you can remove some of your
drupal_goto()calls#52
good catch. attached patch corrects this.
#53
just keeping the patch up to date w/ HEAD
#54
just noticed i was missing curly braces in my db update table statements. this corrects the issue.
#55
(sigh) also missing the UTF8 stuff, so here's an updated patch which includes that
#56
changes in upload module broke the patch again. here's an updated one. i would love it if somebody could visit the test site and see how the trashbin is doing with handling files upon deletion/recovery.
#57
keeping up w/ HEAD
#58
broke by HEAD changes again. here's an update
#59
broken again. here's another update.
#60
aaaaaaaaannnnnd broken again. updated version attached.
#61
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...
#62
ah, nice catches. both of these are now fixed, and an updated patch is attached. thanks!
#63
whoops. i just realized that i was using access denied instead of page not found. attached patch corrects this.
#64
keeping up w/ HEAD. i think i'm about halfway to killes' record for patch revisions... :P
#65
broken again by HEAD changes. updated version attached.
#66
keeping up w/ HEAD.
#67
keeping up w/ HEAD. god, i hope we branch soon, 'cause this is gettin' reeeeeeaaaaaallll old... ;)
#68
upload module change broke it. here's an update
#69
busted again. here's an update...
#70
whew. broken good by forum, taxo, and other changes. took me awhile to fix it up! i need to add some new functionality to allow proper restoration of forums and also for restoration of user's relationships to nodes upon recovery. please test the patch and report any other bugs that you find.
thanks!
#71
broken by the database.mysql split. new patch attached.
#72
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.
#73
killes spotted a query error--it's fixed in this patch.
#74
keeping up w/ HEAD.
#75
what fun. broken again by whitespace changes. killes' revisions patch record is definitely in jeopardy here... :P
#76
undo has a spiffy new test site: http://undo.xcarnated.com/
please help test this patch out! it's slated for core consideration when the branch opens up for development again...
#77
broken by recent HEAD change. new patch attached.
#78
attached patch fixes up the trashbin feature for forum deletions, and also i've cleaned up the system_trash code quite a bit, which resulted in much cleaner code, and less of it :)
#79
broken by HEAD changes again. here's an update.
#80
reworked some of the doxygen stuff. no functionality changes.
#81
broken by HEAD. attached patch brings it up to date.
#82
keeping up w/ HEAD.
#83
+ $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.
#84
keeping up w/ HEAD. i'll address drumm's concerns shortly
#85
dries and i are trying to decide if this patch is worth putting in now, so i'm not going to do any development on it until i have more info. for the time being, i'll just keep it up to date w/ HEAD so people can try it out. here's the latest...
#86
keeping up w/ HEAD.
#87
keeping up w/ HEAD.
#88
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...
#89
well, i've got some small updates to take care of to get it current w/ HEAD again, and dries said he wanted to feel out the desire to have this feature in core. i'm waiting on more data to see if it will in fact happen...
#90
keeping up w/ HEAD. also fixed a bug where some bad queries were being run for the auto-empty feature.
#91
keeping up w/ HEAD. also converted the admin comment deletion feature to use the trashbin.
#92
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#93
hm, i'm getting that error even on a clean install of HEAD on that server, which is running PHP 5.1.4 iirc. so it's not b/c of the patch. i'll investigate further and see if i can figure out the problem. can you try installing the patch into one of your own drupal installations, and see if the error persists? you'll need to run update.php in order to load the trashbin tables...
#94
nevermind. problem was that the ctype extension is not enabled by default in php5/FreeBSD. i've installed the extension and the problem is resolved.
#95
broken by HEAD changes. attached patch is up-to-date...
#96
broken again by HEAD changes. updated patch attached.
#97
hello hunmonk, we are near the 100 comment mark :D
at update.inc you can remove
TYPE=MyISAMThanks Tobi
#98
tobias: thanks for the tip. fixed in the attached patch, as well as some conflicts w/ HEAD.
#99
attached patch fixes a bug in comment batch deletions, and adds support for query strings and fragments in the recovery goto feature.
#100
broken again by major HEAD changes. here's the update.
#101
50+ patch revisions
100+ comments
What's left to fix?
#102
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 updatesdb_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... :/
#103
yes it does. it's necessary because at the time update.php is run, core tables might be altered in a way that corrupts the data.
no it's not. the only place to put this, really, is on the main description page of update.php, i think. or possibly as a drupal set message on the run page. i'll consider putting it in, or, feel free to submit a patch that adds that and i'll have a look... :)
#104
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:
Code:
The following two concerns from february (#49) still apply:
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.#105
@Steven: since i'm in the process of refactoring this functionality, i'm going to hold off on any fixes until i see how that turns out.
in the meantime, attached patch keeps this feature current w/ HEAD.
#106
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.
#107
@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
#108
@pwolanin: this is an insufficient strategy, as there are other kinds of deletions besides nodes--not to mention all of the tables of data that are involved with a node via nodeapi operations. a system which addresses all tables is necessary.
#109
@Dries: couple of things:
just some further food for thought. as i said, i'm not opposed to the approach you suggested--but the cost/benefit of it should be weighed against an approach that's already shown to be workable, and is basically ready to go.
#110
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.
#111
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.
#112
massively broken by the admin patch--took awhile to recover from that... :)
updated patch attached.
test site: http://undo.xcarnated.com/
#113
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
#114
pwolanin: that's not an error--that's the dependency checking system of the trashbin working properly. you tried to recover a node that had dependent data on user/3, who had been deleted from both the active users and the trashbin. to prevent stale data from being reinserted into the database, the system checks that all data that the package depends on is present, and will not allow recovery if it isn't. for example, say you deleted a comment for a node, then deleted the node, then tried to recover the comment. that would be a bad idea, since there's no node to associate the comment with... :)
in this case, since user/3 had been permanently deleted from the system, this package would not be recoverable. such is life... :)
#115
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?
#116
it doesn't matter. somebody came along before you and permanently deleted the user, thus making that node unrecoverable. you've run into a rare edge case, and the dependency checking is meant to prevent stale data from being reinserted into the database in cases like these.
the "example textfield" form field may have been what threw you off. that's merely part of the example to illustrate that the conditional confirm screens can contain form data as well. if you read the main page of that test site, it explains all this pretty clearly.
yes, the preview is stored as an html string in the trash metadata, so previewing wouldn't be a problem. this doesn't mean that the data itself can be recovered safely.
by neccessity, the dependency checking is done when recovery is attempted, as other package deletions which would affect the dependencies of the package being recovered could happen after the package to be recovered is deleted.
#117
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.
#118
@pwolanin: hm, those messages are themed as standard drupal warnings--not sure how i would do it differently!
#119
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.
#120
broken by HEAD changes. updated patch attached. i've also updated the feature to work with the new user mass deletions feature added to core.
test site: http://undo.xcarnated.com/
#121
broken by the content types patch. updated patch attached.
test site: http://undo.xcarnated.com/
#122
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#123
gah. razzin' frazzin' HEAD changes... :) updated patch attached.
site site working again: http://undo.xcarnated.com
#124
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
#125
yeah, that's just a throw in module for illustrative purposes, but i fixed it anyways :)
#126
patch is currently broken b/c of the drupal_render change. leaving as review--you can still hit the test site to check out the functionality. i'll get it fixed up by the beginning of next week
test site: http://undo.xcarnated.com/
#127
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!
#128
hm, files shouldn't collide, as when they are trashed each is moved to a filename w/ a random string appended to it, and that new filename is stored with the deleted package for later recovery/permanent deletion. perhaps some changes to upload module have broken that--i haven't looked at that code in awhile. i'll check it out...
in the meantime. attached patch is updated for current HEAD.
test site: http://undo.xcarnated.com/
#129
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?
#130
broken by the css changes. updated patch attached.
#131
@pwolanin: i believe i've gotten that file bug worked out. please test with a brand new set of nodes and let me know how it turns out! updated patch attached.
test site: http://localhost/drupal/bzr/undo
#132
I can't access http://localhost/drupal/bzr/undo ;)
#133
{listen mode: on}
#134
ah-whoops...
test site: http://undo.xcarnated.com/
#135
latest patch is once again broken on current HEAD. i think i'm going to lay off trying to keep this current until the code freeze is over :) the test site is still working fine if anybody wants to check out the feature and submit bugs.
test site: http://undo.xcarnated.com/
#136
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?
#137
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]
#138
@borked: this patch is no longer being worked on. i left the test site up as a proof of concept. i may work on another version of this in the future, but i offer no guarantees--it's been met with some resistance for inclusion into core, and i can't rightly do it as a contrib.
fixed the test user as well, you are forgiven :)
test site: http://undo.xcarnated.com
#139
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!
#140
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?
#141
correct. but a proper deletion API is no walk in the park. :)
i have written some preliminary code for a new deletion API, but don't have time at the moment to go any further with it.
#142
#143
Any interest in this still? Would need to be rerolled at least...
#144
Susurrus: it was moved to here: http://drupal.org/node/147723