Have you ever laughed fate in the face?
Hi, The current way Drupal deals with delete -- using a confirm page -- is not a real good one. Think of it. It becomes a habit very quick to press Confirm, and also "of course I really want to delete it, that's why I pressed the button". A day later when you realize that this deletion was not a good idea, you can almost hear the whining of the computer "but you pressed confirm!". "A computer is like an Old Testament god, with a lot of rules and no mercy" -- how does this image mate with the friendly smiling face we have choosen as our icon? Chad Philips is working on a patch since last fall which would let Drupal get rid of these absolutely-not-useful confirm screens and give you a shiny moment when you find out that your precious content CAN be brought back from the dead. It deserves much more attention than it gets: http://drupal.org/node/35422 Please review! Thanks, NK
Op zondag 16 juli 2006 05:59, schreef Karoly Negyesi:
It deserves much more attention than it gets: Even more?
followup #101 says it all: 50+ patch revisions 100+ comments What's left to fix? Really, what difference would another 100 comments, opinions and nice-but-it-should-also followups make? Its been nearly a month ago since a core developer (drumm) commented. And Dries did not comment to this thread directly at all. Can we not get constructive critcs from (one of) them? Or some note wether of not they deem this important? Bèr -- [ End user Drupal services and hosting | Sympal.nl ]
Bèr Kessels wrote:
Op zondag 16 juli 2006 05:59, schreef Karoly Negyesi:
It deserves much more attention than it gets: Even more?
followup #101 says it all: 50+ patch revisions 100+ comments What's left to fix?
Really, what difference would another 100 comments, opinions and nice-but-it-should-also followups make?
Its been nearly a month ago since a core developer (drumm) commented. And Dries did not comment to this thread directly at all.
Can we not get constructive critcs from (one of) them? Or some note wether of not they deem this important?
http://drupal.org/node/35422#comment-88827 As long as my remarks there aren't addressed, this patch gets a -1 from me. Cheers, Gerhard
Op zondag 16 juli 2006 13:18, schreef Gerhard Killesreiter:
http://drupal.org/node/35422#comment-88827
As long as my remarks there aren't addressed, this patch gets a -1 from me.
Sorry, I missed your comment. Excuse my comment about the core comitters! Bèr
Bèr Kessels wrote:
Op zondag 16 juli 2006 05:59, schreef Karoly Negyesi:
It deserves much more attention than it gets: Even more?
followup #101 says it all: 50+ patch revisions 100+ comments What's left to fix?
100 comments - 50 patch revisions = 50 reviews (probably much less) Or less than 1 review per revision. No reviews (including testing) for the last revision, which was "... major HEAD changes;" it definitely needs review.
Really, what difference would another 100 comments, opinions and nice-but-it-should-also followups make?
Maybe a thorough test from someone.
Its been nearly a month ago since a core developer (drumm) commented. And Dries did not comment to this thread directly at all.
Can we not get constructive critcs from (one of) them? Or some note wether of not they deem this important?
I think this is a good change. Forgiveness is a good thing to have. But it simply can't be added without people looking at it. Same goes for every other feature. -- Neil Drumm http://delocalizedham.com/
On 16 Jul 2006, at 05:59, Karoly Negyesi wrote:
Think of it. It becomes a habit very quick to press Confirm, and also "of course I really want to delete it, that's why I pressed the button". A day later when you realize that this deletion was not a good idea, you can almost hear the whining of the computer "but you pressed confirm!". "A computer is like an Old Testament god, with a lot of rules and no mercy" -- how does this image mate with the friendly smiling face we have choosen as our icon?
Just got got back from a short trip. I reviewed the module and posted an update at http://drupal.org/node/35422. My problem with the trashbin patch is the way it works. I'm not convinced that it should transfer deleted records to a dedicated table. Right now, the trashbin patch stores deleted records in a separate table in serialized format. This is a little bit awkward as it has to circumvent the auto increment IDs or the sequence table when re-injecting the data. It's both ugly and clever, but at the end of the day, it doesn't feel right to me. Why don't we add a status-field with two states -- STATUS_ACTIVE and STATUS_DELETED -- to each of the database tables with records that want to take advantage of the trashbin functionality? Then, the trashbin patch might be a lot easier to grok -- and from an architectural point of view, less awkward. It wouldn't be particularly clever but that is OK: it is super-easy so there is no point being clever to begin with. I'd like to see us explore this path instead. Plus, this has a number of advantages. Most of all, we'd still able to query deleted data. For example, the filter form on the administer content page (?q=admin/content) would allow us to access delete nodes and we'd be able to use advanced query methods like 'show all deleted nodes from user Joe with the taxonomy term 'Apple'. That is, we get to reuse a lot of the existing UIs and modules. -- Dries Buytaert :: http://www.buytaert.net/
On Thursday 20 July 2006 12:26, Dries Buytaert wrote:
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.
While I've done the "timestamp as deleted flag" trick before myself and it's worked well, it does have one notable drawback: It becomes opt-out. Adding a "Deleted" column to node, comment, and various other tables means that every query against those tables would have to be modified to add "WHERE deleted=0". Between core and contrib, that's thousands of queries that would need to be updated. Granted, the update is smaller than, say, the Forms API conversion, which also broke everything in one fell swoop. But that was replacing one method of doing X with a different, better method of doing X. Once the transition is/was complete, there's not an unreasonably larger amount of work to do (IMHO) than before. However, a deleted flag column would essentially mean 99% of all entity queries (node, comment, user) from now on would have to remember to check "WHERE deleted=0". Do we really want to introduce that extra step? I'm not giving this thumbs up or down, just pointing out that there are more costs involved in such a mechanism than tweaking a few queries in the patch. -- Larry Garfield AIM: LOLG42 larry@garfieldtech.com ICQ: 6817012 "If nature has made any one thing less susceptible than all others of exclusive property, it is the action of the thinking power called an idea, which an individual may exclusively possess as long as he keeps it to himself; but the moment it is divulged, it forces itself into the possession of every one, and the receiver cannot dispossess himself of it." -- Thomas Jefferson
Larry Garfield wrote:
Adding a "Deleted" column to node, comment, and various other tables means that every query against those tables would have to be modified to add "WHERE deleted=0". Between core and contrib, that's thousands of queries that would need to be updated.
Exactly, no normal module should need to concern itself with something that is "deleted". But my original suggestion was for the abstraction layer to add the "deleted=0" condition by default. So what you are suggesting shouldn't be a problem. And to repeat. There could also be a way to call db_query() that allows special code (eg. upgrade, trashbin, deep search) to over-ride the default behaviour and include the deleted rows in the result set. I can't see any downside except execution speed in the abstraction layer.
On 7/20/06, Larry Garfield <larry@garfieldtech.com> wrote:
remember to check "WHERE deleted=0". Do we really want to introduce that extra step?
I'm not giving this thumbs up or down, just pointing out that there are more costs involved in such a mechanism than tweaking a few queries in the patch.
We already generally need a "where status = " on most queries so this is just one more where clause. I'm sure people protested about the "status =" when it was introduced, but it makes sense in the long run. This is similar - we will grumble while we transition and love it in the long run. Maybe I'm just being overly tolerant, maybe this is the straw that breaks the camel's back, but I don't think this is _that_much_ more painful than the current query system that it necessitates a whole new object API to replace SQL access. Regards, Greg -- Greg Knaddison | Growing Venture Solutions Denver, CO | http://growingventuresolutions.com Technology Solutions for Communities, Individuals, and Small Businesses
Greg Knaddison - GVS wrote:
We already generally need a "where status = " on most queries so this is just one more where clause. I'm sure people protested about the "status =" when it was introduced, but it makes sense in the long run. This is similar - we will grumble while we transition and love it in the long run.
Maybe I'm just being overly tolerant, maybe this is the straw that breaks the camel's back, but I don't think this is _that_much_ more painful than the current query system that it necessitates a whole new object API to replace SQL access.
I suspect a fair number of queries in contrib don't contain the where status = 1 already. Adding another just compounds that.
On 21 Jul 2006, at 04:07, Larry Garfield wrote:
Adding a "Deleted" column to node, comment, and various other tables means that every query against those tables would have to be modified to add "WHERE deleted=0". Between core and contrib, that's thousands of queries that would need to be updated.
Adding an extra where-clause to all queries is perfectly valid if that is the best solution from a technical point of view. We shouldn't go with a solution that is sub-optimal, and avoid a superior solution just because it is a lot of work. That sort of decisions tend to backfire in the long run. -- Dries Buytaert :: http://www.buytaert.net/
On Jul 21, 2006, at 12:38 AM, Dries Buytaert wrote:
Adding an extra where-clause to all queries is perfectly valid if that is the best solution from a technical point of view. We shouldn't go with a solution that is sub-optimal, and avoid a superior solution just because it is a lot of work. That sort of decisions tend to backfire in the long run.
if we're going to generate a patch that touches "thousands of queries" and test the results, the "best solution from a technical point of view" seems to be to port all said queries to some kind of query building interface so we never have to touch them again for this kind of thing... -derek
On Fri, 2006-07-21 at 01:04 -0700, Derek Wright wrote:
On Jul 21, 2006, at 12:38 AM, Dries Buytaert wrote:
Adding an extra where-clause to all queries is perfectly valid if that is the best solution from a technical point of view. We shouldn't go with a solution that is sub-optimal, and avoid a superior solution just because it is a lot of work. That sort of decisions tend to backfire in the long run.
if we're going to generate a patch that touches "thousands of queries" and test the results, the "best solution from a technical point of view" seems to be to port all said queries to some kind of query building interface so we never have to touch them again for this kind of thing...
-derek
workflow + actions in core could be an option. Have a workflow state for deleted, on state change to deleted, do action delete. yet another option.
workflow + actions in core could be an option. Have a workflow state for deleted, on state change to deleted, do action delete. yet another option.
+10. This is the solution we should get behind. Dries has targeted workflow+actions for next version. It's flexible. It's an API. There are a bunch of actions out there already (and they're easy to write). As part of "core" install profile, we could add a reasonable default workflow (with states like "unpublished" -> "published" -> "deleted/archived"). As to checking the "deleted" field. This can happen using hook_sql_rewrite. Any legit query looking at nodes should be calling this hook anyway. Also: this allows for the deletion to live in contrib, which sounds like what people want. -M
+10. This is the solution we should get behind. Dries has targeted workflow+actions for next version. It's flexible. It's an API. There are a bunch of actions out there already (and they're easy to write). As part of "core" install profile, we could add a reasonable default workflow (with states like "unpublished" -> "published" -> "deleted/archived").
yeah, i like this too. but i don't think it is fair to propose this as a solution since workflow is nowhere near ready right now, and noone is working on it AFAIK. chad has worked on the delete patch for many months, and proposing a vapor solution is not helpful.
As to checking the "deleted" field. This can happen using hook_sql_rewrite. Any legit query looking at nodes should be calling this hook anyway. Also: this allows for the deletion to live in contrib, which sounds like what people want.
clarification: currently, you only call db_rewrite_sql() when performing a listing of nodes, and not when retrieving a single node. it would be simple enough though to add the single node view case to node_access() function.
On Thu, 20 Jul 2006, Larry Garfield wrote:
(IMHO) than before. However, a deleted flag column would essentially mean 99% of all entity queries (node, comment, user) from now on would have to remember to check "WHERE deleted=0". Do we really want to introduce that extra step?
IMHO what Dries suggested with the status column is simple. status = 1 is already in queries. status = 0 means unpublished, another status can mean deleted. No need to modify queries (just review existing ones for the status queries). Gabor
On 20-Jul-06, at 1:26 PM, Dries Buytaert wrote:
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?
+1 for this. I'd like to see some standardization of 'status' semantics throughout (anyone else get annoyed that '0' is published for contents and '1' is published for nodes and users?). I also like that this can be further extended to include additional STATUS values. Shouldn't it be STATUS_ACTIVE, STATUS_UNPUBLISHED and STATUS_DELETED though? Or am I misreading your intention? As noted later in the thread - we already *should* have WHERE status=1 clauses in most modules (those that don't already have bugs, imo)... so this would be a natural extention ... i.e. most modules would only ever list where status=STATUS_ACTIVE. -- James Walker :: http://walkah.net/ :: xmpp:walkah@walkah.net
On 21 Jul 2006, at 19:57, James Walker wrote:
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?
+1 for this. I'd like to see some standardization of 'status' semantics throughout (anyone else get annoyed that '0' is published for contents and '1' is published for nodes and users?). I also like that this can be further extended to include additional STATUS values.
Shouldn't it be STATUS_ACTIVE, STATUS_UNPUBLISHED and STATUS_DELETED though? Or am I misreading your intention?
Good question. :) The workflow module isn't 100% compatible with the current status field in the node table (which makes for a confusing UI). I'd like to see the workflow people chime in so we can settle on a solution that is forward-compatible with the workflow module. Anyway, I think that STATUS_DELETED should be a workflow state like any other, and that we should be able to trigger an action when something is marked deleted. From an implementation point of view, it shouldn't be a special case. All in all, I think it is a reasonably straightforward task. :-) -- Dries Buytaert :: http://www.buytaert.net/
participants (15)
-
Bèr Kessels -
Darrel O'Pry -
Derek Wright -
Dries Buytaert -
Earl Miles -
Gabor Hojtsy -
Gerhard Killesreiter -
Greg Knaddison - GVS -
James Walker -
Karoly Negyesi -
Larry Garfield -
Mark Fredrickson -
Moshe Weitzman -
Neil Drumm -
sime