Deletion API

Log in or register to post comments

Comment #13

hunmonk commented 27 March 2007 at 02:05

Status	File	Size
new	dapi_4.patch	38.77 KB

broken by confirm form fix/language patch. updated patch attached.

Log in or register to post comments

Comment #14

somes commented 27 March 2007 at 10:05

Does this allow easier user interface deletion with AJAX - might a pop out div be used with the new system to confirm that the node should be indeed deleted

Log in or register to post comments

Comment #15

hunmonk commented 28 March 2007 at 15:41

@Somes: this is a back-end API, not front end user candy -- AJAX delete would be feature creep for this patch. please file another issue to discuss that very valid feature request. :)

Log in or register to post comments

Comment #16

hunmonk commented 28 March 2007 at 15:47

Status	File	Size
new	dapi_5.patch	38.77 KB

keeping up w/ HEAD.

Log in or register to post comments

Comment #17

hunmonk commented 6 April 2007 at 17:51

Status	File	Size
new	dapi_6.patch	37.72 KB

broken by menu module updates. updated patch attached.

Log in or register to post comments

Comment #18

hunmonk commented 15 April 2007 at 17:59

Status	File	Size
new	dapi_7.patch	37.72 KB

attached patch fixes a PHP 5 issue where array_merge_recursive() results in NULL if NULL is merged into another array.

Log in or register to post comments

Comment #19

hunmonk commented 24 April 2007 at 21:11

Status	File	Size
new	dapi_8.patch	37.81 KB

keeping up with HEAD.

Log in or register to post comments

Comment #20

hunmonk commented 27 April 2007 at 14:54

Status	File	Size
new	dapi_9.patch	37.79 KB

broken by the watchdog translation patch. attached patch corrects.

Log in or register to post comments

Comment #21

hunmonk commented 30 April 2007 at 21:09

Status	File	Size
new	dapi_10.patch	37.78 KB

broken by the menu translation patch -- updated patch attached.

keeping up can be murder sometimes... *wipes brow* :)

Log in or register to post comments

Comment #22

hunmonk commented 10 May 2007 at 16:11

Status	File	Size
new	dapi_11.patch	37.79 KB

keeping up w/ HEAD.

Log in or register to post comments

Comment #23

hunmonk commented 14 May 2007 at 14:52

Status	File	Size
new	dapi_12.patch	37.74 KB

broken by the fapi 3 patch -- updated patch attached.

Log in or register to post comments

Comment #24

hunmonk commented 15 May 2007 at 16:37

Status	File	Size
new	dapi_13.patch	37.78 KB

broken by fapi changes. updated patch attached.

Log in or register to post comments

Comment #25

hunmonk commented 15 May 2007 at 18:57

it's now two weeks before the code freeze, and this issue has yet to receive any feedback from a core committer. if anybody cares about improving drupal's very limited deletion cycle capabilities, now is the time to speak up! :)

patch applies to current HEAD, works as advertised, and there's a test site w/ exhaustive documentation. i need feedback on this patch ASAP, so i have time to make any necessary corrections before the freeze.

i have carried the deletion torch for two major release cycles, with no real progress in core -- this will be my last attempt to help this area of drupal... :)

Log in or register to post comments

Comment #26

merlinofchaos commented 15 May 2007 at 19:01

Priority:

Normal

» Critical

subscribing. Upping the priority a bit, becuase I think this is good functionality.

Log in or register to post comments

Comment #27

hunmonk commented 16 May 2007 at 14:58

Status	File	Size
new	dapi_14.patch	37.62 KB

attached patch makes some minor documentation corrections -- all functionality is the same.

i've taken the time to make sure the documentation/examples on the test site are all in order as well:

Log in or register to post comments

Comment #28

Stefan Nagtegaal commented 16 May 2007 at 17:03

Status:

Needs review

» Reviewed & tested by the community

This is very nice.. Itested this very heavily, and I dare to set this RTBC..
There are no errors showing up, and everything works smoothly and without any problems...

Dries pls commit this or leave your comment here.
Hunmonk is keeping this patch up for ages now and the functionality is really good..

Log in or register to post comments

Comment #29

hunmonk commented 16 May 2007 at 17:50

Status	File	Size
new	dapi_15.patch	33.52 KB

well if we're going to be bold, let me pull out the example hooks i had in comment module, since they should probably be part of another patch!

attached patch is exactly the same, with the example comment hooks removed.

i'm hesitant to leave this RTBC, but what the hell... ;)

Log in or register to post comments

Comment #30

hunmonk commented 17 May 2007 at 15:31

Status	File	Size
new	dapi_16.patch	33.4 KB

minor conflict w/ the multiple menu patch. attached patch corrects, and has been tested as working.

Log in or register to post comments

Comment #31

drewish commented 17 May 2007 at 18:41

Status:

Reviewed & tested by the community

» Needs work

- I don't think it's safe to hijack confirm_form() to delete specific.
- The form for confirming the deletion of comments via admin/content/comment has T as a button title.
- The UI at admin/content/node is very confusing when part of a deletion is aborted. The user is told that their node cannot be deleted but still offered a button to try it anyway.

Log in or register to post comments

Comment #32

drewish commented 18 May 2007 at 04:45

The deleting multiple comments from /admin/content/comment redirects to itself so you get the menu tabs and help text which is confusing. I checked a clean copy of HEAD and this is an issue there as well.

Log in or register to post comments

Comment #33

hunmonk commented 18 May 2007 at 06:19

Status:

Needs work

» Needs review

Status	File	Size
new	dapi_17.patch	39.04 KB

I don't think it's safe to hijack confirm_form() to delete specific.

i disagree with that assessment. the confirm_form isn't being hijacked at all -- i've simply repurposed it with a structure similar to the recent l() and url() patches, and gave it sensible defaults. since the confirm form is most often used for delete confirmation, it makes sense for it to have a default setup for deletions -- the core code for delete confirm forms gets much cleaner this way. and the confirm form can still be used in all the ways that it used to be...

The form for confirming the deletion of comments via admin/content/comment has T as a button title.

fixed.

The UI at admin/content/node is very confusing when part of a deletion is aborted. The user is told that their node cannot be deleted but still offered a button to try it anyway

also fixed. i struggled with how to handle this -- it's tricky b/c you have multiple packages, but you want everything displayed through one confirm form. i tackled it by a new feature in the API -- allowing package-specific confirm form elements to be passed in. this way, if the package is aborted, then those elements can be tossed instead of merged into the confirm form.

along the way, i greatly improved the redirection logic for the API, resulting in much more sensible fallbacks in the case of aborted deletions.

The deleting multiple comments from /admin/content/comment redirects to itself so you get the menu tabs and help text which is confusing. I checked a clean copy of HEAD and this is an issue there as well.

since this is a current problem in HEAD, i'd like to vote that we tackle it with another issue... :)

new patch attached. let's keep banging away!

NOTE: http://drupal.org/node/141131 broke comment forms in HEAD! i've adjusted the test site to accomodate this until the issue is fixed. if you want to try this patch locally, you can comment out the call to setrawcookie() in comment.module to work around the issue.

Log in or register to post comments

Comment #34

hunmonk commented 18 May 2007 at 07:14

Status	File	Size
new	dapi_18.patch	39.04 KB

minor typo that was preventing comment nodeapi deletions from being properly checked.

also, i've put a small example module on the test site which better demonstrates custom message and abort features of the API -- if a node has more than two comments, deletion will not be allowed. if a comment subject has the word 'abort' in it, it also cannot be deleted. messages regarding the number of comments deleted with a node are posted to both the confirm form, and as a user message after deletion.

i'll try to load up that example module so that folks can install it locally if they'd like.

and as always...

Log in or register to post comments

Comment #35

hunmonk commented 18 May 2007 at 07:31

Status	File	Size
new	comment_protect.tgz.txt	1.83 KB

ok, attached is the example module i described in the previous comment.

Log in or register to post comments

Comment #36

hunmonk commented 18 May 2007 at 07:52

Status	File	Size
new	comment_protect.tgz_0.txt	1.86 KB

whoop. that last example module i posted had a bug in it. this one should do much better. :)

Log in or register to post comments

Comment #37

hunmonk commented 19 May 2007 at 05:43

Status	File	Size
new	dapi_19.patch	39.2 KB

drewish didn't like the way the abort messages at admin/content/node and admin/content/comment where displayed both on the confirm form, and after the deletions were executed. after further thought, i agreed that it was potentially confusing, so the following patch corrects that UI issue.

it's actually quite slick now that confirm form elements can be added per package -- you simply include the hidden confirm form elements in that way, and if a package is aborted, then it's not even considered on the confirm screen at all!

Log in or register to post comments

Comment #38

drewish commented 21 May 2007 at 16:09

hunmonk, I'm really impressed by what you've gotten done here. The interface is great. I've started trying to look through the code. Noticed a few things but I'll give it a closer look later.

+++ modules/comment/comment.module	2007-05-19 05:34:57 +0000
@@ -232,7 +232,8 @@ function comment_menu() {
 
   $items['comment/delete'] = array(
     'title' => 'Delete comment',
-    'page callback' => 'comment_delete',
+    'page callback' => 'drupal_get_form',
+    'page arguments' => array('comment_delete', 2),
     'access arguments' => array('administer comments'),
     'type' => MENU_CALLBACK,
   );

should that be arg(2)? since it should be a comment id?

+  // array_filter() returns only elements with TRUE values, array_keys() returns the nids.
+  $nids = array_filter($edit['nodes']);

I don't think you call array_keys().

The parameter format of drupal_delete() is a little odd. i'd almost rather see all the options passed in as an array rather than a variable number of parameters.

I'm also going to voice my opposition to making confirm_form() default to confirming a deletion.

Log in or register to post comments

Comment #39

hunmonk commented 21 May 2007 at 17:21

Status	File	Size
new	dapi_20.patch	39.2 KB

should that be arg(2)? since it should be a comment id?

i'm pretty sure that's the correct way to pass arg 2 in the new menu system...

I don't think you call array_keys().

whoop, i removed it from the code and not the comment :) attached patch corrects.

The parameter format of drupal_delete() is a little odd. i'd almost rather see all the options passed in as an array rather than a variable number of parameters.

i'm not sure how viable that is, since the function needs to be so flexible. leaving the variable params makes it very easy to pass in the query stuff. also, $id and $op are always present, while the other args change depending on $op. i'm pretty sure what i have is the cleanest approach, but i'm open to hearing better ones.

I'm also going to voice my opposition to making confirm_form() default to confirming a deletion.

it wouldn't be that hard to make another function, delete_confirm_form(), and make that do what i'm making confirm_form() do now. i'm open to adding that, but i really don't think it's necessary -- as i outlined above, most uses of confirm_form() are for delete confirmation, and the function is still flexible enough to do everythign it used to do.

Log in or register to post comments

Comment #40

hunmonk commented 21 May 2007 at 17:28

Status	File	Size
new	dapi_21.patch	52.17 KB

gah, last patch wasn't quite right -- here's the one w/ the fix...

Log in or register to post comments

Comment #41

hunmonk commented 21 May 2007 at 17:32

Status	File	Size
new	dapi_22.patch	39.17 KB

i swear this is the right one... :/

Log in or register to post comments

Comment #42

chx commented 22 May 2007 at 07:51

Status:

Needs review

» Needs work

Additional arguments are passed to the function, depending on the operation to be performed: Give full function call examples, it's very confusing otherwise.
I really would like to see a research into this api crossed with batch, not necessarily for D6, but onwards.
drupal_delete('', 'set package', $package_id); No need. Just $delete_id = $package_id will do.
if (count($package_info) > 3) { 'Once the number three, being the third number, be reached, then, lobbest thou thy Holy Hand Grenade of Antioch towards thy foe, who, being naughty in My sight, shall snuff it.' is this the reason? or...?
module_invoke_all('delete_execute', $package_info); make this a drupal_alter('delete', $package_info).

Log in or register to post comments

Comment #43

hunmonk commented 22 May 2007 at 08:37

Status:

Needs work

» Needs review

Status	File	Size
new	dapi_23.patch	40.48 KB

great review chx! all of your concerns are addressed in the attached patch. i'll need to post a new .tgz of the example module with that change to drupal_alter(). to follow shortly...

http://deleteapi.xcarnated.com

Log in or register to post comments

Comment #44

hunmonk commented 22 May 2007 at 08:39

Status	File	Size
new	comment_protect.tgz_1.txt	1.89 KB

... and the new example module...

Log in or register to post comments

Comment #45

hunmonk commented 23 May 2007 at 01:23

Status	File	Size
new	dapi_24.patch	40.49 KB

after some more thought on drewish's suggestion to not hijack the confirm_form() function for the deletion API, i've rearranged a few things to keep that function more general. i did keep the options array, because it seems more in keeping with the current trend to eliminate functions with eight billion arguments. :)

the submit handler for DAPI has been moved into the api itself, and a more specific function name has been given to the callback. the _only_ thing left in the confirm form that's specific to deletion is that the 'yes' button defaults to 'Delete'. the original default was 'Confirm', which i don't think was all that useful anyways.

patch attached.

Log in or register to post comments

Comment #46

hunmonk commented 25 May 2007 at 16:56

Status	File	Size
new	dapi_25.patch	40.43 KB

broken by the schema patch. update patch attached.

Log in or register to post comments

Comment #47

he/him

commented 27 May 2007 at 07:46

@chx (from a long ways back) MySQL 4.1 supports transactions fine. It's MyISAM that doesn't.

Log in or register to post comments

Comment #48

chx commented 27 May 2007 at 08:32

Sure. I know. TO be more on topic, what else is needed to get this in for D6?

Log in or register to post comments

Comment #49

he/him

commented 27 May 2007 at 08:47

Sorry, but I think this patch is a bad idea. It reinvents a wheel carefully rounded, inflated, and balanced by the database world. Worse yet, it uses new terminology (like "packages") to refer to concepts long familiar to database developers. It would be much better to add foreign key support to the schema API and support code to emulate foreign key CASCADE DELETEs for MyISAM tables. (They would run slower than native CASCADE DELETEs but work.)

There might be justification for allowing modules to attach messages to delete forms that will affect their data, but modules can already do that though hook_form_alter().

Log in or register to post comments

Comment #50

he/him

commented 27 May 2007 at 09:41

After playing with the demo, deletion API seems to also implement an analogue of RESTRICT DELETE on foreign keys.

Log in or register to post comments

Comment #51

http://drupal.org/node/113435

he/him

commented 28 May 2007 at 00:08

I should re-characterize my discontent. I'd love this patch if:
* Terminology were changed to make it consistent with standard database operations (foreign keys and transactions)
* It tried to use native database operations where possible
* (Maybe) it integrated with Schema API to identify deletion dependencies for RESTRICT and CASCADE

I'm sorry to be so hard on this patch, but the more we do in PHP, the slower, more chatty, and less reliable operation becomes with the database.

Log in or register to post comments

Comment #52

nedjo

he/him/his

English

commented 28 May 2007 at 03:39

I like the idea of a deletion API and appreciate the thought and effort that's gone into this patch.

However, I share some of David's concerns. A deletion API should be a step toward a unified, schema-based data API. This patch doesn't read as such a step.

Is this the approach we will also want to be taking for loading, creating, and updating objects?

I've tried to sketch out some of the steps toward a unified data API in the patch

(which includes a minimalist deletion API, also triggered by a drupal_delete() function)

and the issue

http://drupal.org/node/145684

I suspect that little of the complex handling implemented in this patch will be needed when we have consistent schema-aware data APIs.

Log in or register to post comments

Comment #53

hunmonk commented 29 May 2007 at 01:53

I should re-characterize my discontent. I'd love this patch if:
* Terminology were changed to make it consistent with standard database operations (foreign keys and transactions)
* It tried to use native database operations where possible
* (Maybe) it integrated with Schema API to identify deletion dependencies for RESTRICT and CASCADE

in response:

i have no problem changing the terminology -- we can call packages 'widgets' -- i don't care :) if you'll lay out more clear terms instead of the ones i currently have, i'm happy to change them. i consider this to be an easily fixable problem.
why are we discussing database level transaction functionality, when we can't support it?? afaik, we're still going to support MyISAM tables, so doing transaction level stuff at the database isn't an option. i'm pretty sure most people with a MySQL install are using MyISAM.
the central point of this API is to provide application level support for deletions -- if we don't have it in some form, then the drupal's deletion cycle is blind to any information that outside (non-core) modules have regarding that deletion.

I suspect that little of the complex handling implemented in this patch will be needed when we have consistent schema-aware data

i haven't looked at the data API closely yet, so i won't comment on the idea that the approach obsoletes this one. what i do know is this patch works now, solves deletion related problems now, and has been heavily tested. afaik, there's no guarantee that the data API will go in next cycle, either, and we can benefit from this now.

Log in or register to post comments

Comment #54

he/him

commented 29 May 2007 at 02:17

@hunmonk It's important to discuss foreign keys and transactions because those are built into InnoDB and PostgreSQL, and they run far faster and more reliably than any equivalent in PHP. We're really going to hurt ourselves if we only use what MyISAM supports natively. What we can do is either emulate the functionality for MyISAM in PHP or provide a safe fallback.

This patch -> database equivalent
* Deleting associated items automatically -> Foreign key with CASCADE
* Restricting deletion when associated items exist -> Foreign key with RESTRICT
* Responding in a custom way to deletions -> Triggers

If we set this system up in a DB-centric way, we can map as many operations as possible to the native DB system. As this code is structured now, it's impossible to offload tasks to the database.

Log in or register to post comments

Comment #55

hunmonk commented 29 May 2007 at 14:03

@David Strauss:
i'm *pretty* sure that a large majority of people are using MySQL with MyISAM tables for their installation. if this is the case, then it's doubtful that we would drop support for MyISAM anytime soon. And from what i recall from my research into database engines, MyISAM is actually the right choice most of the time for a website -- it's not transaction-safe, but it's very fast on reads, which is what most websites need.

if we support both systems, then we've got a new problem: more code to maintain. if you strip out the code comments from this patch, it weighs in pretty light, and it works for all systems. that's a pretty big advantage. i'll agree that it's slower, but so what? in almost all site scenarios, deletions are a fairly rare occurrance -- so performance isn't as much of a consideration as it would be for something like a function that is called every page load.

so far i'm not convinced that moving things to the database layer would be an overall win.

Log in or register to post comments

Comment #56

hunmonk commented 29 May 2007 at 14:52

Status	File	Size
new	dapi_26.patch	40.43 KB

keeping up w/ HEAD.

Log in or register to post comments

Comment #57

chx commented 29 May 2007 at 15:04

No patch can save the world, not even a Drupal core patch. If it works and does something useful on its own, then it is good to go. One of the worst things you can do is elaborate on other, equivalent approaches and suggest complex extensions to the patch. Additional features can be added later. A scalpel is often better than the Swiss Knife.

This patch is a good one, various goodness based on schema can happen. And yes, it should happen later but for now we have a working Deletion API here and no code for a schema based deletion API.

Log in or register to post comments

Comment #58

drewish commented 29 May 2007 at 15:53

I agree, with chx. it'd be great to have this done at the DB layer but right now we've got nothing and hunmonk has come up with a solution at the application layer. As he points out deletions are rare so efficiencies aren't as critical as they might be if this affected viewing.

Log in or register to post comments

Comment #59