Deletion API for core

I talked with Nedjo and he asked what does this patch enable us? Abort the deletion -- if for some reason something is not a good idea to be deleted then the code can stop that. And also, acting on deletions. (And when the data API lands, there will be an API cleansing and it's like that this API will need to be changed significantly or scrapped altogether but that's not now, let's not cloud this issue with future concerns about possible code).

I see this as a very nice API which should go on.

I like the functionality this patch provides and agree that it is needed.

I spoke with chx as he noted. I'm satisfied with ideas that (a) it's too early to expect new APIs to anticipate schema API features that don't even exist yet and (b) a data API will necessitate enough rewriting as it is that introducing this patch now won't add appreciably to that task, (c) neither schema nor data API considerations should hold back a valuable addition.

Since I already tested the functionality of this patch before, here are some comments on the code:

1) The PHPDoc is extensive (which is good!) but needs to conform to coding standards. See http://cvs.drupal.org/viewcvs/drupal/contributions/docs/developer/hooks/...

2) The drupal_delete function is a gigantic monster with 4,000 different argument possibilities. We shouldn't need that much PHPDoc for any function; if we do, it's a clear sign that it's trying to do too much. I would suggest splitting that into a few different API functions, such as:

- drupal_delete_api: It's the mother control function that does all the other 'high-level' APi stuff, and holds a 'state' variable or something to track what state in the deletion API we're in. Somewhat analogous to drupal_bootstrap().
- drupal_delete: handles ONLY telling what you want to delete (the query op)
- drupal_delete_package: handles the package operations.

3) This looks like a lot of typing:

+  drupal_delete('comment', 'query', 'DELETE FROM {comments} WHERE cid = %d', array('cid' => $comment->cid));

It seems to me that with schema API, we should be able to simplify that syntax-wise to just:

drupal_delete('comment', $comment->cid);

The comment.schema file will already indicate which field is the primary key; no sense in a developer having to specify it and type out the SQL by hand.

oh. On the plus side, I should point out that I do think it's very needed functionality for modules to be able to interact with the deletion cycle. And i've functionality-tested this patch on a number of occasions and it's been solid.

I agree with Angie's suggestions for this patch.

IMO, this patch is not mainly about new functionality. We could probably find ways e.g. to provide feedback on or abort or respond to delete operations in D5 by form_altering delete confirmation forms (e.g., register #validate or #submit callbacks that e.g. test conditions and selectively invoke a goto).

The reason I support this patch is rather that it brings a consistent API. For the sake of that API, the patch could be strengthened and simplified by taking advantage of the schema API where feasible and separating out data handling (the welcome proto- data API part of this patch) from the complex UI and workflow handling, some of which seems to be unique to delete as opposed to insert or update operations.

@nedjo, that was my thought as well, but hunmonk pointed out that if you wish to remove the confirm form altogether (as you would if you wanted to change Drupal to use a "Recycle bin" approach to deletion... e.g. just delete it, don't make me confirm), you're out of luck because the form is forced upon you. And if the form is gone, you no longer have the ability to use form_alter to change deletion events.

@webchick, thanks for the clarification.

It seems to me that with schema API, we should be able to simplify that syntax-wise

I thought so too when I read your comment, but I think we're still missing a way to connect e.g. 'comment' the object type with 'comments' its primary table (that awaits http://drupal.org/node/140860, consistent table names). So we can do this sort of improvement as a follow-up patch after the deletion API lands and after other schema-related ones do as well.

I just spent some time hashing over the details of the Delete API with hunmonk.

I'd like to chime in with the others to note that this is a sound 'first stab' at a working deletion API, and we're better off with it than we are without it. Additional functionality can be layered onto it, but trying to cram everything into version 1 will result in nothing but another Drupal version without this functionality.

I do agree that the drupal_delete() function, with it plethora of ops, needs to be broken into bite-sized chunks. hunmonk is, from what we discussed, re-rolling the patch with helper functions implemented that allow commands like drupal_delete_add_form() and so on for the addition of confirmation forms, and so on.

My only thought is that we're building a cleanly structured array here, not unlike what we do with form_state in FormAPI. Would it make more sense to avoid the plethora of helper functions, and implement hook_delete_alter(), giving modules a chance to add additional queries to a delete operation, add snippets of form data, etc? All of the information is being centrally managed in just such an array already. It might be easier than putting in a new cloud of helper functions.

That's a philosophical question, though. As it stands, the API provides a solid expansion point that was previously impossible. Regardless of what the answer to my above question is, I think the patch has merit and should be committed once the $op clumsiness is elimatined. Nice work, hunmonk.

-The trivial:
I think function drupal_delete() should be re-named _drupal_delete(), since it is now only supposed to be used via the wrapper functions.
I would also suggest cutting much of the doxygen from this function, since it's redundant with the wrapper function comments.

-A more serious question:
right now I'm working on this menu module patch: http://drupal.org/node/151583
which has a hook_nodeapi implementation that deletes the menu links corresponding to a node when it is deleted. However, these deletions are done via a call to menu_link_delete(), which is a function that can handle any sort of link (external, node, callback, comment, etc). Thus, there is no link deletion via a direct query. So, it seems that except for noting this in 'metadata', there is no way to associate these deletion events with the node deletion?

ok, so at the least the actual invocations need to be in a callback.

Overall. this looks good, but it obviously requires a big shift in mentality.

the lines like this:

+    // This is a placeholder -- preserves proper ordering in the confirm form.
+    $form["node_$node->nid"] = array();

for node and comment modules seem unnecessary. The ordering seems to be the same without it, and in any case is by Primary key (I think) not by something meaningful like title.

The only bug I see is that both the multi-node and multi-comment delete don't have their own menu handler, hence causing help text and tabs to appear on the confirmation screen.

But, this appears to be a problem even before this patch. Chad, can you comment on this?

Also, the doxygen for drupal_delete_add_query() still uses the old syntax, and the documentation for $id is really confusing. From the description it sounds almost the same as $type in drupal_delete_initiate().

What is $id supposed to identify? A particular query in a package? A set of queries in a package?

From the code, it would seem it identifies a query, and even more that there are forbidden id's you cannot use:

+      unset($package_info['callback'], $package_info['metadata'], $package_info['form']);

This is generally bad form and should be avoided if possible. I don't see why you can't put all queries in a 'queries' array. It would clean up the check for metadata-only packages too. If not, it needs to be documented at least.

Finally, there is still mention of drupal_delete() in the examples, and some tabs have snuck in.

Here's an issue for the node and comment modules: http://drupal.org/node/153425 - perhaps should be extended to be a clean up all uses of raw $_POST when not necessary?

Fixed indentation on some of the confirm form arrays and added deletionapi doxygen group for the public methods.

Couldn't find any other issues.

Committed to HEAD.

Upgrade docs need to be added to the handbook.

Docs were added.

FYI: http://drupal.org/node/154046 -- We should clean up the ambiguous keys for the array you pass to drupal_delete_confirm().

1. On IRC I clearly mentioned to Chad that we would not be committing the Deletion API.

2. Why was this issue forked several times? Please, don't create new issues as it breaks the history. In one of the previous issues I clearly mentioned not to commit this patch.

3. I'm considering to rollback this patch as I don't see the advantages it brings. Personally, I'm not convinced that this is a step forward.

I agree with the rational for the patch that Steven's posted to the development list:

Part of the reason I committed this patch is because, while it
definitely solves some annoyances and adds useful hooks for contrib,
it is also subtly the beginning of a transaction API. I think it will
be interesting to see the kind of things modules do with it, and like
Form API, it will possibly get refactored (Form API 2/3) as well as
branch out to other areas (Node/profile rendering arrays).

I'm a bit dismayed that there'd be talk of rolling this patch back this close to the code freeze. hunmonk has put an amazing amount of work into this, and has been soliciting feedback for over three months. I hope that there's just been some confusion between a previous trashbin patch and this one.

Here as elsewhere, whatever the outcome, Dries' "sober second look" provides a valuable role.

Implementing the API will require a lot of rewriting of core and contrib. Is this worthwhile? Does it take us in directions we want to go?

I've hesitated to say too much on this issue as my comments were described as sidetracking and the issue forked accordingly, and I got some strong offlist feedback on my comments.

Reviewing the issue, I'm not convinced the concerns raised with this patch were fully addressed before it was applied. There are two main outstanding questions.

1. In this current thread, the main issue I saw raised was a perceived need to break the API into distinct pieces, providing in particular a clean separation of the data handling from the UI concerns.

   Helper functions were introduced, but it's not clear that they achieved the goal of simplifying and normalizing the approach. What's changed is that developers are shielded from the need to call the complex API function directly, but the helper functions all still feed back into the original complex function, which still mixes data operations with UI and state. The surface is reworked, but some of the underlying problems remain.

2. Steven's reasoning for applying the patch, this is a step toward a unified data API

   The problem is that this issue was forked precisely to leave behind data API questions as irrelevant. See my comments and questions here: http://drupal.org/node/126300#comment-251477. They were the last before the issue was moved. So if progress toward a data API is a major reason to take this direction, the comments weren't sidetracking--they were exactly what we needed to discuss, but didn't.

this issue was not forked several times -- it was forked once, at the suggestion of another dev, because the old issue was going seriously off track.

Please don't fork issues. It only complicates matters.

Let's continue to talk about this patch, and then we can decide whether it should be rolled-back or not. I'm still leaning towards rolling it back.

My major gripes are:

1. Rather than creating one uniform system to manipulate forms (i.e. the node API) we now built a layer on top of the forms API. This is a step backwards.
2. Rather than just writing a submit handler that executes a number of callbacks, we now register delete queries. This harms the readability of the code, especially because sometimes, these callbacks are registered at different places in the code. The code become less transparent, and usually, it takes more code as well.
3. The argument that it provides support for transactions seems moot. This patch doesn't provide a uniform or scalable mechanism to solve this problem.

I don't understand what this patch adds that couldn't already be done using the form API in Drupal 6. Maybe you can elaborate on that a bit more?

I don't understand what this patch adds that couldn't already be done using the form API in Drupal 6. Maybe you can elaborate on that a bit more?

I had that question as well. From #9:

hunmonk pointed out that if you wish to remove the confirm form altogether (as you would if you wanted to change Drupal to use a "Recycle bin" approach to deletion... e.g. just delete it, don't make me confirm), you're out of luck because the form is forced upon you. And if the form is gone, you no longer have the ability to use form_alter to change deletion events.

I'm not the resident expert on this patch, but my understanding is that it is not a 'layer on top of FormAPI' so much as it is a standard way of 'wrapping' destructive operations such that other modules can participate in the operation (aborting it to enforce data integrity, adding other elements to be deleted, adding a confirmation form where there was none, notifying the user of what 'cascade' effect will take place, etc.) FormAPI is about data presentation and workflow. Deletion handling is its own beast; it is often initiated by a form but is definitely a process of its own.

While other concerns may remain, I think any modules implementing the same level of functionality would have had to reinvent a similarly hefty API internally.

Dries, I agree with some the comments that _drupal_delete() could use some refactoring (maybe using global variables rather than statics?) but I think this provides a much cleaner way of handling deletion in general for all types of data.

dopry and I were arguing about how to allow modules to have a say in file deletion as part of the files patch we've been working on. He wanted modules to have a chance to cancel a file deletion and I was against it because it's not the "Drupal way". All of our APIs provide only notice of deletion, short of hacking every form that might call deletion, they don't allow modules a way to weight in until after the deletion has taken place. In the middle of this argument the Deletion API patch hit and after resolving the conflicts and spending a little time to understand how it works, I was able to work with hunmonk and come up with an (as yet uncommitted) patch to add files to the deletion api. The result will allow the flexibility Dorpy wanted, modules can monitor the deletion of files and abort the entire delete package or remove a specific file, and keep a clean hook_file() API that jives with the rest of Drupal.

Thanks for the carefully written reply. That's much appreciated. However, I'm still not convinced.

It's funny to see people support this patch without making _real_ arguments.

before this patch, how would a module intelligently prevent a deletion that was about to happen? it had no way to examine what was going to happen before it happened. in the case of a single node deletion, i suppose you could reload the node in a form_alter, examine it, and do a brute force drupal_goto() -- but isn't that a pretty bad hack?

Actually, you form_alter an extra validate hook into form, and you check the node in your validate hook. You don't have to use drupal_goto(). This is what the 2000 other Drupal forms use. If that is considered a hack, it is a hack for all forms, not just for the delete forms. Fixing it for the delete scenario only, is bad practice and prevents further form API improvements.

have you taken a look at any of the conversion patches in the queue?

Yes, I have, and they are harder to read. In fact, it are these conversion patches that make me reconsider this patch.

The following:

function user_something() {
  db_query('DELETE FROM {role} WHERE rid = %d', $form_state['values']['rid']);
  db_query('DELETE FROM {permission} WHERE rid = %d', $form_state['values']['rid']);
  db_query('DELETE FROM {users_roles} WHERE rid = %d', $form_state['values']['rid']);-
  drupal_set_message(t('The role has been deleted.'));
}

is a lot easier to read and test than the following:

function user_something() {
  drupal_delete_initiate('role', $rid);
  drupal_delete_add_callback(array('user_admin_role_delete_confirm_post' => array($name)));
  drupal_delete_add_query('DELETE FROM {role} WHERE rid = %d', $rid);
  drupal_delete_add_query('DELETE FROM {permission} WHERE rid = %d', $rid);
  drupal_delete_add_query('DELETE FROM {users_roles} WHERE rid = %d', $rid);
}

function user_admin_role_delete_confirm_post($name) { 
  drupal_set_message(t('The %name role has been deleted.', array('%name' => $name)));
}

The latter requires me to learn a new API and is harder to debug or test. The former lets me use my existing expertise about the form API. In other words, it lowers the barrier, it is easier to read, and its more transparent.

another major win is that finally, not only can we implement an undo functionality, but we can do it in contrib.

Yes, but we would only be able to implement undo functionality in a hacked-up way.
Your solution would physically remove the rows from the table, keep a copy of the data in serialized format, figure out the table layout. Upon undo, you have to construct an INSERT query and then force the data back in at the write place with the old row ID -- effectively by-passing things like auto_increment. That's quite a hack.

The *real* solution is to do it in core, and to use status-fields for all tables that want to support undo functionality. When you delete something, you set the status column to deleted. The "delete cycle" becomes a lot easier to coop with and hack-free.

Furthermore, in some environments it is not allowed to _ever_ delete something. I don't think the hacked-up-trashbin solution would accomodate that very well.

I can't believe people like drewish think the former is more clean than the latter.

i of course am totally open to making any neccessary adjustments to improve the API. there are already several patches in the queue that address some issues that others have raised. that being said, i sincerely believe that what's in there now is a quality 'next step' towards unified data management structures in drupal.

I would propose we start by deleting all the things that the form API lets you do. So far, the most compelling argument for the delete API was the ability to prevent something from being deleted. Unfortunately, that is something the form API can do.

If the form API doesn't let you do it in an easy way, we might consider keeping it, or better yet, we consider to improve the form API. Deleting something isn't any different from manipulating data. Maybe I want to prevent that someone manipulates a certain node field? Maybe someone can change his bio but not his salary?

The point? If the main motivation for the delete API are shortcomings or inconveniences in the form API, then that is a good reason to rollback this patch. We then need to focus our efforts on the form API, not the delete API.

All in all, I think I'm going to rollback this patch.

Moving away from the previous issue made some important comments harder to find, but I do share the critics brought up by nedjo at http://drupal.org/node/126300#comment-251477 and expressed by Dries above. This deletion API reinvents the wheel for some things which we already had more generic solutions for (like form altering becomes drupal_delete_add_form_elements(), SQL commands are added into packages and so on).

There were other kinds of "encapsulate actions to execute later into 'packages'" type of patches even in the localization front for Drupal 6, and I see the same pattern echoing back here. These type of packages limit the functionality you can use in the workflow, and also enforce you to use highly custom functions to accomplish basic data manipulation tasks.

Actually, you form_alter an extra validate hook into form...

Well, to be fair, you form_alter an extra validate hook into every form anywhere in Drupal that could delete nodes, for example. The multiple-deletion screen, the administrative screen, the node edit screen, Akismet module's custom admin page, and so on. And you hope that you caught them all.

This treats the deletion operation as something unique that can be started by any code, altered by any code, etc.

That may not be compelling, but the difference is important. Just wanted to chime in. :)

I just wanted to mention that I was equally reluctant to see this as a good thing initially. However, having tried the demo modules, and seen how, for example, I could use this to cleanly and easily add a message to both the single and multiple node delete confirm forms, I was won over. It certainly requires a shift in thinking, but I now agree firmly that this is a positive step forward.

dries, for me it boils down to this. as a module author, how do i prevent another module from deleting one of my nodes in a cron hook? there's no form to alter... with hunmonk's api i can affect that operation.

I just wanted to emphasize the distinction that others have made above. Drupal has a forms layer and a data layer. We can't mix the two until the mythical Data API hits (even though drupal_execute() is a step in that direction). That is, we cannot assume that all data manipulation happens through forms.

Note: this is about usability. Confirm forms are very bad usability wise because eventually you will have a habit of just pressing yes (ask Angie how she deleted her user from gdo). Instead, the computer should do what you ask and not chicken out but lend a hand to reverse the damage. This is the crux of all.

Let' see what we have here...

The *real* solution is to do it in core, and to use status-fields for all tables that want to support undo functionality.

I disagree. If a node or 10000 is deleted I want my node table to shrink. If there was a catastrophe like a spambot or something else (/me fondly remembers a night around Brussels DrupalCon when Michael and I were cleaning up 10 000+ bogus images) then I expect those to be gone from the table I query on every single page load. In case something goes wrong with the cleanup, they are still there in an archive table, ready to be brought back -- but not burdening the poor database every second.

Furthermore, in some environments it is not allowed to _ever_ delete something.

That's awesome, deletion API let's kill all deletion in one sweep, no matter how they happen either via forms or not.

The following... s a lot easier to read and test than the following... highly custom functions to accomplish basic data manipulation tasks.

Well, while data API is not an extension of form API as others mentioned and I confrim, I will still use form API but only as an example. You might want to say that Drupal 4.6 forms were simpler. There were less functions to be written. We had no separate validate or submit callbacks. You could throw in a piece of form wherever and whenever you wanted. Nowadays you call indirectly a form definition function. Why this burden is accepted? Among other things becuse form API let's you expand and alter forms in ways we never had before. Remember the pain of adding tokens against CSRF to 4.6. We already have two ideas -- trashbin and cancels -- which will be alters of the deletion API and we have not even started.

About data API, schema with relationships -- I would rather eat a pie today than a cake a year from now. And I am standing firm behind the "forking" the issue, our concern today is Drupal 6 and not Drupal 7 or 8. We can throw this API out when there will be a better one. Or we can augment it from the looks of it. I am for judging a patch on its own merits not against future, unwritten code.

I think the deletion API is a useful addition to core - and I would vote for leaving it in for now. Having it possible to control the response to deletions (from keeping them, to trashbins, to other yet to be discovered approaches) is a *very* beneficial addition to core.

I've not spent nearly as much time with this code as most, but - while there are some complaints - I think the good feature it provides outweigh them. It is a new API... it's a new approach... it's not perfect yet, but neither was FormAPI the first time around.

I'm with chx when he says "We can throw this API out when there will be a better one." ... let's leave it be and refine & improve on the months and months of hard work that Chad has put into this.

I also vote to keep this API in core. Whatever happened to "code is gold"? This patch represents almost 2 years of thinking, researching, trying, failing, redesigning, developing, testing, and refining. Many hundreds of hours of work. Is it perfect? Of course not. Will we ever improve upon it? Of course. Is this a step backwards for core development? No. Forms are different than data (though they're obviously related). I think hunmonk, chx, jvandyk and drewish (#49) have done an excellent job presenting this, so I won't repeat them.

What's already been committed obviously exists, and it works. I'm glad Steven took the initiative to drive the patch home and give use some much needed progress in the D6 issue queue. We've already begun to improve the API. We could have spent the time and energy we've had to put into defending the decision to commit into yet more progress. We have fixed a lot of the existing bugs in D6 in our collective efforts to port core to the new API before the freeze (I say "collective", but again, hunmonk has done the lion's share of the work). Let us not waste this immense effort because someday something else might supercede it. As with the book.module patch, we are comparing existing, functioning code that improves core against some imaginary future code that no one has committed to writing. The more often that golden code such as this is rejected in search of illusive platnum code we've been dreaming of that doesn't exist, the less likely developers will be willing to work on these issues in the first place.

It has taken 3 major iterations of FAPI and there are probably still some more improvements to be made. Certainly, we're not going to get everything 100% right the first time we do it, so we have to take action, then assess it, learn from the experince, and improve it in the future. However, we can't gain any experience or learn from the Deletion API if we take it back out of core, and our design discussions about a future Data API will remain abstract and generate more silver talk than gold code.

Whether you think the delete API is useful or not, does _not_ matter. I'm not arguing with the functionality, I'm arguing with the _implementation_ and the _design_ of the patch. If you comment on this issue, talk _technical_ not meta. It's disappointing to see the last 4 comments completely ignore the real crux of the argument.

I like hunmonk, and yes, he spent a lot of time working on this. At the end of the day, I only care about the design and implementation, not about who wrote the patch, or how long he spent working on it. Our job is to review patches based on their technical merit, not to feel sorry with hunmonk regardless of how much we like hunmonk. (I obviously like hunmonk.)

For example, I simply can't accept this patch as it would not allow me to implement a proper trashbin implementation. The design and implementation of the proposed trashbin solution is completely orthogonal with the design of a proper trashbin solution, and will lead to a hackup-up trashbin. This is a step backwards and it will cause us more work later on. What point is there in having to undo all the delete API changes that people will have to make to their contributed modules and core? That's just silly and would cause more damage and frustration compared to rolling back this patch now.

Yes, the first forms API wasn't perfect either, but at least, there were no obvious flaws that we knew we had to rollback.

I'm willing to commit a patch that allows the delete cycle to be interrupted more easily or that allow messages to be injected in the form more easily. However, drupal_delete_add_query(), drupal_delete_initiate() and everything else related to SQL query manipulating will have to go. That should be a good compromise, as that are the only features that people have been supporting.

I'm going to discuss this some more with Steven and Gabor, and then make a final decision.

There is no SQL query manipulation. It's just that you call a function with the same queries, the same syntax -- nothing new there. And while it seems I am talking meta, I can't even imagine how can one build a proper deletion API without collecting somehow the necessary queries.

Also, you call something a 'proper trashbin solution' which seems to be tables with a deletion flag and I am saying no, that's not the proper trashbin solution and if it's meta, be it, but I got no answers. I believe I honestly answered more or less all your concerns and they are pushed as meta.

chx: you were arguing that we need to be able to permanently delete rows from the database and you stated that using a status field would not allow us to. That's a false assumption, and hence, your reply missed the point. You just shared a use case, and no technical arguments.

The point is that we need a two-stage delete, and the question at hand is how to best implement a two-stage delete. You seem to be under the impression that we might go with a solution that is a half-assed one-stage delete.

So let me elaborate so it's hopefully a bit easier to understand:

1. When you delete something, permanently delete the data.
2. When you delete something, make it possible to undo the delete operation.
3. When you delete something, _never_ delete the actual data but just hide it. Depending on your country and the business you are in, there are government regulations that mandate this.

Currently, Drupal does (1) and we're trying to figure out how to implement (2) and (3). We all want to support (2) and (3). To get (2) and (3) we need a two-stage delete, and the question at hand is how to best implement a two-stage delete.

hunmonk's patch allows us to do (1) and (2). From a technical point of view, it implements (2) in a hacked-up way. It will actually delete the rows from the database table and store them in serialized format in a trashbin database table. Then, when you want to undo the delete operation, we need to reconstruct the INSERT query based on the unserialized data (requires meta-data and database table introspection). Furthermore, we need to re-INSERT the data at the exact same rows (with the exact same auto_increment IDs) of the tables that we deleted the data from. This is quite a hack, and it doesn't particularly scale well if you want to accomplish (3). hunmonk's patch sorta works for (2), but really fails short when we want to take (3) serious.

My suggestion is not to delete the data from the tables, but to mark the data as 'deleted' using a status code -- based on a simple convention. Then, both the first-stage delete and its undo operation are easy, fast and elegant: just flip the status field. Now, if you want to permanently delete the data from the database (the second stage delete), you simply delete the data as we always used to do. This allows us to implement (1), (2) and (3) in a straightforward way.

In other words, I'm not advocating that we cannot permanently delete data. I'm just proposing a better/cleaner technical solution for temporary deleting data. This means that the patch that went in needs to rolled back so we can work on a proper solution and so we can prevent hacked-up solutions from becoming the de facto standard in the contributions repository.

So, whether or not you had to delete 10,000 nodes on NowPublic is not really relevant and misses the technical arguments that I was looking for. Hopefully, this clarified the situation, and hopefully, we'll be able to discuss this further based on technical merits.

Hmm, when you're talking about temporary deletion, that just sounds like unpublish for nodes, a variation of "block" for users etc. - very different from this patch, which seems more like a safeguard against accidental permanent deletion.

I see two perspectives being argued here.

The first is about functionality. Roughly, "This patch provides needed functionality (and someone put a lot of work into it). The details or implications of the implementation - where it might be leading us - are not so important, since we can always rewrite it later. Let's just get this in."

The second is about design. Roughly, "If it works and brings new functionality, great, that's important. Now let's look at the design, and ensure it's not only done but done right. If we're introducing a major new API, we need to be sure it's taking us in a direction we want to go. It should stand up as a model for how this sort of problem should be solved."

I think both are valid positions. Personally, I'm firmly of the second opinion.

I am disappointed with the process in this issue. Among the various respected developers who expressed support for the patch, I haven't really seen any significant analysis of the approach. The general message I've got - particularly from the action of shutting down the issue and calling my attempts to provide feedback sidetracking - was, don't get in the way of valuable functionality by raising issues. Or, maybe, "shut up and get in line". This isn't the atmosphere I want to promote in the community. I don't think it leads to good decisions.

This major new direction for Drupal went through many revisions, but didn't have many developers' hands in it. It's probably a significant symptom that it was the patch author himself who set this to RTBC. No one - none of the developers now protesting the fact that Dries is reexamining the issue - spent the time to give this the level of review that would have allowed them to set it themselves to RTBC, or really to say much of anything about the patch, other than "I want the functionality".

Let me restate my personal concerns about this patch.

A major issue we have in our data management in Drupal is that our methods are relatively encumbered by UI and user interaction considerations. When we set about to build e.g. a SOAP application to directly load and manipulate Drupal data, we run into interference from UI-centred methods. A Forms API is great, but it's not a substitute for form-free data handling methods. The direction that's been taken with programmatic form submission - having to pretend we're users in order to trick Drupal into doing basic data operations - is one of the clearest symptoms of how over-reliant we are on the user input model.

What does the delete API design do in terms of the separation of data handling from UI and forms?

To try to understand this, I looked at node_delete() before and after the patch.

Before the patch, node_delete() was a relatively straightforward and clean data handler. It did what it said it did, delete, and it did so directly.

After the patch, node_delete() doesn't delete. Rather, it feeds an array of SQL statements (indirectly) into a very complex function _drupal_delete(), that mixes up the actual delete operations with a massive set of UI states, form elements, feedback messages, etc.

IMO, this is a major step backwards in terms of separation of data handling from UI.

A lot of the logic of the delete API seems premised on the assumption that delete operations have unique needs (for intervention and user feedback between queuing) that justify a custom data handling approach distinct from other operations. But consider update. Might we wish to prevent or modify or provide feedback on an update operation, based on certain conditions? I don't see why not. If so, should we also have an update API modelled on the delete API? And if we do, should update and delete be completely different from insert and load?

Which brings us back to the general question. Should this be in core, lumps and all? Does the functionality justify inclusion, regardless of the design implications? Should we commit it now and worry about rewriting it later? I see the argument in favour, but ultimately I don't agree. I'm more concerned with integrity of design than I am with new functionality. I appreciate the hard work that went into this patch. I agree that this is needed functionality. I love the demos. But I see this ultimately as the right functionality but an unconvincing underlying implementation. I think we could live for another cycle without this. I suspect this could be rewritten for contrib and live there--a lesser solution, but a place to work through the design issues. In contrib, I suspect the best approach would be a schema-based one, perhaps dependent on Schema module (assuming we don't get a patch into core that adds foreign key data, as needed to infer delete implications).

I don't think this patch was ready to be committed. The patch didn't get a lot of quality review. Significant issues that had been raised were not successfully addressed. Unless there is new work to address these issues, I would support rolling this patch back.

This is quite a hack, and it doesn't particularly scale well if you want to accomplish (3). hunmonk's patch sorta works for (2), but really fails short when we want to take (3) serious.

I guess I might want to elaborate on that. There are various reasons for this, but let me stick to the 2 most important ones (as far as I can see):

1. If you are required to keep all data, the serialized data in the trashbin table might have thousands of records. It becomes difficult to manage that data or to partially delete data that you are actually allowed to delete. If you're allowed to delete data after 2 years, there is no easy way to do that. You can't run useful queries on that data/table.
2. When you have hundreds/thousands of records in your trashbin table, and you upgrade from Drupal 6 to Drupal 7, you might not be able to undo your delete operations. It's practically impossible to provide an upgrade path for data in the trashbin.

@Dries - any such trashbin is a theoretical possibility. So, I find it odd that you're judging this patch based on it. Could not a trashbin be a separate MySQL DB with a full set of tables? Or a SQLite DB? etc.

I think with this API, you could intervene to make all node deletion operations turn into unpublish operations, so that no content is ever deleted. Without this API, I assume one must hack node_delete. So I guess I clearly fall more on the side of wanting functionality now, while agreeing that there will be an ongoing process of improving the API and/or merging it with a more complete data API. This is not something that can live in contrib.

@Dries - any such trashbin is a theoretical possibility. So, I find it odd that you're judging this patch based on it. Could not a trashbin be a separate MySQL DB with a full set of tables? Or a SQLite DB? etc.

It could, but it would be unwanted. Are you seriously proposing that we duplicate many (or all) tables? You might end up with 25+ tables with a _delete postfix.

I think with this API, you could intervene to make all node deletion operations turn into unpublish operations, so that no content is ever deleted.

Yes, but it would still be less elegant. It might be possible to do so for nodes, but how are you going to do this for comments, users, watchdog entries, or whatever other non-node content type? Even if you were to do it for nodes, it would be more complex and uglier than simply switching a status field and properly supporting a two-stage delete based on status-field convention.

Again, it's all about how to do it properly. As you can see from your own suggestions, this patch pushes us in the wrong direction. Thanks for re-enforcing my point. ;)

This patch deserves to go in for several reasons.

I have for many moons thought that Dries supported unifying the way things are done. We have over the past releases, with forms api, the theme system, drupal_render, and the menu system. Now I am proved wrong. We are letting every delete function delete in their own way, with no way for a developer to intercept the deletion. Even if that was the case, each time a delete function called oh say hook_delete_intercept, things would be a bit different. Some developers would be to lazy to call this function. That is why this patch deserves to get it. It seems Steven supports unifying the apis, and Gabor as well. (Neil has no say in this because he can only commit bug fixes) Chad has put some really hard work and long hours into unifying the way we do things in core. And now Dries is pulling it out.

dmitrig01: http://drupal.org/node/147723#comment-266927 -- Gabor is actually against this patch.

hunmonk: I'll try to allocate another hour to illustrate my point more clearly, I guess. I already spent 2-3 hours on this today, and I'd like to make some progress with other patches. Also, at the end of the day, you'll have to convince me -- and not the other way around.

For now, I'll re-iterate my point. We want to support two-stage commits and this patch doesn't get us there. However, it does a number of things that only seem to make sense in the context of a trashbin module. It looks to me as if we're in a fairly undefined state right now -- we might have to refactor the entire delete API if we want to support two-stage commits properly and it looks like we might have to undo large amounts of this patch if we want to fix this problem in a more generic way so that it is also useful for update screens, rather than just delete screens. We're asking a lot of people to upgrade their modules even though this hasn't been fleshed out properly.

In many ways, this seems like temporary fix for a larger problem, and one that takes us into the wrong direction. As we all know, there is nothing more permanent than a temporary solution. ;) Therefore, I'm inclined to rollback this patch, and to spend more time figuring out the guts of it. Given that so many people have an opinion about it, it should be easy to get a ton of good reviews during the Drupal 7 development cycle. :)

Based on past experience, I'm 100% confident that this will eventually lead to a better situation/solution in the long run.

I'll also try to get hold of Steven so I can discuss this some more with him. I'd like to know his final opinion.

Dries: sorry :) I meant in general unifying apis

To illustrate more clearly the points I made above, here is a quick and rough attempt to sketch in what I have in mind by an approach to this patch refactored to focus squarely on the core issue of data handling, and to be generic enough to serve as a small but sound step toward methods that could be applied to all transaction types. Lots is missing of course and I'm not suggesting there's any comparison between these code snippets and the fully working solution that went into the patch.

The approach of generating an array of pending operations is retained, but is pulled out to a hook, which would be eliminated when possible to be replaced by schema introspection.

We use a getter/setter pair of functions to enable any module at any time to determine the current state of a pending transaction.

Transactions are processed in a series of stages: prepare, confirm, execute, and then followup. At each stage, a hook_transaction_stage() allows modules to respond to, alter, or terminate a transaction.

Messaging is left to modules responding to hook_transaction_stage() or form_altering and determining the current transaction.

Talked to Steven and Gabor and we unanimously agreed to rollback the deletion API. We all support the features this patch added, yet not its actual design and implementation. After some talk, we decided that it would be better for Drupal not to go with a solution that isn't 100%. We also recognize that in the short term, this patch would have been a useful addition (it's better than nothing). I know it is unfortunate, and even painful, but I honestly believe this is beneficial in the long term and I hope you all support this decision.

hunmonk: I understand that you might not be inclined to work more on the deletion API after such a move, and I realize that rolling back this patch risks burning you out for a while. However, I do honestly want to figure out how to implement this properly in D7 ... and I'd be happy to spend an entire day with you to brainstorm about this and to lay out the foundations of how we could make this work. Feel free to take me up on that offer any time after the D6 release.

I guess that's that then.

Just a note that the old Deletion API docs were located at http://drupal.org/node/153904 ... now unpublished. Just so we don't lose that documentation, because it's very good.

subscribing

Resetting status

subscribing

subscribing

The discution remove warning modal dialogs and replace them with undo in the group "Mark Boulton Design Drupal 7 Project" is very related. This feature can be "technicaly" implemented in D7?

The workflow can be more smooth if we can remove the warning message for deleting something and replace it by a undo system.

WordPress just got this now..

Global undo/”trash” feature, which means that if you accidentally delete a post or comment you can bring it back from the grave (i.e., the Trash). This also eliminates those annoying “are you sure” messages we used to have on every delete.

http://wordpress.org/development/

Subscribing

This also came up during usability testing, although not a huge issue.

But certain participants when confronted with a confirm page just blindly deleted without reading it. They ended up deleting content and a shortcut link most likely without even realizing it. It'd be nice if they had such a feature to protect themselves when this happens.

(I've also acceidentally deleted my own user from groups.drupal.org and it would've been nice too ;))

We've been discussing making the default entity implementation use CRAP (create, read, archive, purge) instead of CRUD (create, read, archive, delete). With that, deleted items would stay in the database (possibly only in the revisions table), and could potentially be restored.

So an undo function would be adding restore capability on top of archive/purge - CRAPR.

I agree with that approach - this is what wikis like MoinMoin have been doing forever: anybody can delete a page (or trash content) fairly easily, but we can also rollback.

So this would make deletion non-destructive, and therefore wouldn't need to require confirmation.

I am not sure this is the right issue to followup on that however.

A restore from deleted feature would be fantastic. But I feel that we shouldn't be holding onto deleted data forever. I would suggest a clean up function to completely remove deleted items from the database that are older than a configurable length of time (with a default of perhaps 90 days).

catch wrote:
> We've been discussing making the default entity implementation use CRAP (create, read, archive, purge) instead of CRUD (create, read, archive, delete).

Yes please!

This is outdated and duplicate now to #2725447: WI: Phase D: Enable archive/purge storage