The irony of Model::deleteAll()
Today I wanted to implement some functionality to delete all records of a table. Easy, there is a deleteAll() method in the Model class I can use for this purpose. So I wrote the following statement:
$this->MyModel->deleteAll(array(), false);
As I executed that code, nothing happened. No delete statement was generated. Hm. So I looked at the source of the deleteAll() method, and what’s there? The method simply returns false if you provide an empty $conditions parameter:
if (empty($conditions)) {
return false;
}
And as this is not a bug (see ticket 3699), this means you have to provide a dummy condition if you want to delete all entries of a table:
$this->MyModel->deleteAll(array('1 = 1'), false);
or
$this->MyModel->deleteAll('1 = 1', false);
It’s a bit ironic that you have to use a workaround when using deleteAll() to delete all entries of a table, isn’t it? ;-)
I favor how it’s written. I don’t think you’d want to accidentally delete the contents of a table because you’ve left the params empty.
@John - how likely are you to accidentally put deleteAll in your code? You are more likely to accidentally click a link that deletes all your code. This workaround won’t keep you from doing that. I’ll have to side with cakebaker on this one - it doesn’t make sense.
Agreed. I think this doublechecking is unnecessary.
Okay, most of your gripe posts are a bit of a stretch anyway, so I can let those go, but I’m sorry, this is just outright idiotic. Model::deleteAll() is something you have to be careful with anyway, so making you go out of your way to do something that’s already potentially stupid is really not much to ask.
@dho: what about getting a life and letting go the fact that you are no longer part of the CakePHP team? I mean seriously, you are starting to sound like a bitchy ex-wife
@hydra12:
What are you talking about? Links? Not sure how that relates to the topic.
I’d still argue that it’s too easy to leave that param empty in some way. I like it how it is.
There is a better way to delete all records.
http://dev.mysql.com/doc/refman/5.0/en/truncate.html
@John:
I think I’m misunderstanding things here. Based on the context of the post, I can only see using this in an admin interface, where an admin could click on a link to clear all the records in a table. That’s where the links comment comes in. From that perspective, having to put in a fake conditional (as per the post) seems pointless.
On further thought, this could be used to just delete all users with blue eyes or green polkadots or something like that. From that point of view, I can see requiring the conditional. The name (Model::deleteAll) does seem to indicate that you can use it to delete all the records, though. As things stand, that’s not so easy to do (which I guess is the point). Personally, I’d think that putting a conditional in to delete all records that meet a certain criteria is a no-brainer, while having to put in a conditional to delete everything doesn’t make sense.
Having said all that, CakePHP is still the best thing out there for php development. Keep up the good work, cake team.
@cakebaker:
I like reading your stuff. Maybe you have a grudge like nate and Mariano think, and maybe not. All I know is, every time you post something about a cake function that doesn’t work the way you thought it would, I learn something. Even if it’s just learning that I need to read the api docs again, I learn something. IMHO, that’s a good thing. Thanks!
Just curious, what other frameworks have a workaround such as this built in? Was this idea influenced by other frameworks doing the same thing?
Why “potentially stupid’?
There are cases we design our app in such away that some tables need to be emptied at some point.
For instance, we have a table tracking ids of users having certain activities on our site. Weekly, we scan this table and send out some kind of notification to these users. Once notification emails have been sent, all the info in the table becomes obsolete and needs to be cleared (ready for next week).
In an other case we allow the administrator to delete all messages from a message table to start out a fresh forum.
Making it harder to use a function is not a great way to protect data, from developers point of view.
Obviously, the function is not to be used as an OS command line; before the code is up for use on the web site, we have it tested and make sure it works as intended / designed. The functions in the code are not accidentally got into our applications.
If the logic is wrong in the application, data might still be destroyed regardless how complicated it is to use the delete function.
Further more, the deleteAll has had ‘All’ in its name; it clearly serves as a warning.
Cake developers’ thoughtfulness is much appreciated, but nothing wrong with cakebaker’s pointing out something that is not “obviously straightforward” (or not documented) for other developers’ awareness.
@Malcolm: asking myself same question.
@Steffen - Yes, this can be a more efficient way to clear down a table - particularly if it is a large data set or temporary data. But beware - truncate is DDL not DML - if you use truncate within a database transaction, rollback will have no effect on the truncation.
Having a framework trying to save novice developers from themselves is not necessarily a bad thing - Although the API is clear to most, there may be an assumption by some that $Parent->Child->deleteAll() will delete all children of the parent via associations… Having said that, if they’re not testing, they deserve to get fired for deleting all the data :)
Deletes are always a bone of contention - I’m often asked ‘Why can’t you delete obsolete stock items?’ - (because they’re linked to historic orders). If in doubt, soft-delete (active=0) - disc space is cheap, reputation is not.
Perhaps if it was named “deleteAllConditionally”, this whole debate would have been avoided. :-/
But then again, reading technical blogs with a bit of drama does make it a bit more interesting and less dry. Oh wait, this is Cake… DRY is good. Never mind. :)
@Zonium & Malcolm: The decision wasn’t influenced by another framework, it was based on being safe my default, a.k.a erring on the side of caution. The docblock reads as follows: “Allows model records to be deleted based on a set of conditions”. But as NOSLOW says, maybe it would have been more obvious had I called it deleteAllWhere(), but then for consistency I’d have had to do the same to findAll().
Yes, I know what you’re thinking, and you’re wrong, because there’s a subtle but important difference: findAll() doesn’t nuke your table every time you call it without conditions. And for those of you complaining about “usability,” give me a break. I’m all for being lazy, but damn. Adding a three character string to a method call is not exactly a huge price to pay (actually you can just pass “1″ on MySQL) to mitigate the risk of having your application destroyed by rejecting user input that you forgot to validate. Don’t thank me, I’m just doing my job.
Yeah, data backups are great, and yeah, you should always check any user-submitted input to a method, but those things are really just like anything else. When was the last time you actually backed up your dev machine (*before* Time Machine)?
Yeah, that’s what I thought.
However, you could just as easily argue the following if deleteAll() *did* delete all records without providing a condition (and some dev whined that it “nuked” their table):
The function is “deleteAll” - what part of “delete all” made you think it *wouldn’t* delete …um… “all”?
@Silv: I see your point, but I think this idea of “safe by default” is lost on you. People can disagree all they want and that’s fine, because we’re sticking with the implementation we’ve got.
I agree with Dh. (not that I care about discussing this, I guess we all have our views on it)
I think it’s safe to say that this is an informative post, no matter whether you agree with Dh or not :-)
@NOSLOW: Agreed w/ you on the naming.
@nate: I also have guessed the reason for not naming it differently was to keep names consistent and hence the trade-off in terms of inconsistency in behaviors/required params.
Right, not a huge price to pay. Just wanted something perfect for cake - consistency and no documentation should (usually) be needed.
@nate Please don’t misinterpret my comment as a lack of understanding. I fully understand the “safe by default” design principle and think it’s a good one for many reasons.
Despite the obvious bad blood floating around, I’ve no desire to make the cake team defend themselves, they don’t need to, the success of the framework speaks for itself.
I merely was pointing out that there’s pretty valid arguments to both sides.
I don’t think the disagreement necessarily needs to have the goal of changing the design decision, but rather the goal of increased clarification and understanding.
Perhaps the choice of wording and overall approach of the post drew a conclusion rather than asked a question, the former sounding more like “this is wrong” and the latter more like “why is this”…
I believe it isn’t the API’s job to be securing for idiocy. Then again, if there was documentation that nicely explained the necessity to input a dummy condition, it would be fine.
Having to read the source code — to find out that there is an (in this case) unasked-for safety check, that would upset anyone.
I think it’s fare. Code completion can be tricky sometimes, and provide you with a method call that isn’t what you wanted to type.
It’s not fair to empty a table if you type $this->, Ctrl + Space, and misselect an option from the dropdown list, then refresh.
That might seem stupid or careless at first, but when you’re in the ‘heat’ of programming, making small changes and hitting F5 endlessly in your browser, this would be much more prone to accident if there was no ‘protection mechanism’.
[...] the comments on The irony of Model::deleteAll() Posted by Richard@Home Filed in CakePHP, Database, Quote Of The [...]
That dummy data may save me. Thanks alot for bringin up this topic.
I think the dummy data do have a purpose. It wasnt there just for nothing. Imagine.
@dho sorry man, i am completely see what would happen if theres no some kind of protection in deleteAll(). if you try to go back in your 1st year college years. can you remember your stupid classmates? Well i do. they were like
C:\>format c: then enter.
and
“Teacher, what does format c: do? Look it says 100%. what will happen?”
I hope you get what im trying to say.
@mariano : man, i always read your blog too. but your comments doesnt help anyone.
methods should to what the method name says. having said that, i don’t think a dummy-condition should be required.
when would accidenticaly emptying your table be a problem? on a production system.
who develops on a production system? no one should ever do so!
@Lobaman: it may help a divorce lawyer get some business. Look at the big picture. (that’s sarcasm btw)
@derelm: yeah, and there should be no wars in the world, nobody should starve, and pollution should be just the name of some pub. Unfortunately we don’t live in an ideal world.
@all: Thanks for your comments!
@John: As hydra12 already pointed out, it is rather unlikely you will delete the contents of a table by accident. And even if that should happen, you (hopefully) develop on a development machine with a development database where it doesn’t matter when the content of a table gets deleted.
@nate: Well, I recommend you calm down before writing such a comment the next time. What I described is a problem I encountered while using some framework code, a workaround for it, and that it’s strange you have to use such a workaround by design. That’s it.
I don’t like it if the framework tries to babysit me and forces me to write ugly code that’s harder to read, when there is a simple and elegant solution possible (which btw is also easier to implement, test and document for you). And if you think it is quite dangerous to call deleteAll without conditions, then why is the $cascade parameter set to true by default? And why is it possible to use findAll() without parameters, even though it is possible that someone may select all records of my huge table? I think the task of the framework is to provide some generic functionality, but how it is then used is up to the framework user. Or to say it differently: If I buy a knife and injure myself, then I hardly can blame the dealer for selling me a sharp knife.
@Mariano: Don’t worry, I have a life ;-) But if you didn’t notice, I still use cake and hence I encounter problems with it from time to time. Sometimes it is my fault, sometimes it is, like in this case, because of design flaws. As I am a software developer who cares about software design I simply notice such stuff, and I write about it. If you don’t like that and if you are not able to write a constructive comment (like others did who don’t agree with me), then please stop reading this blog.
@Steffen: Thanks for the link.
@hydra12: I don’t have a grudge, I simply like software design and so I notice it if something could be improved. I think what you can learn from this is that you have to be careful with the API docs. What I described here you don’t find in the API…
@Malcolm: I don’t know whether it was influenced by other frameworks. At least in Rails the conditions parameter for delete_all is optional.
@Zonium: Thanks for your examples. And I agree with you that the name “deleteAll” itself serves as a warning.
@GreyCells: Yes, I agree with you that it is not that obvious what $Parent->Child->deleteAll() does. But you will learn this soon ;-)
Great quote: “disc space is cheap, reputation is not.”
@NOSLOW: I think it would be a bit strange to have a method called “deleteAllConditionally”, as the first question will be “why is there no method to delete all records of a table?”, if you have to implement such a scenario ;-) No, “deleteAll” is a good method name, it should only work in the way you expect it to work.
@nate: Well, it makes it harder to understand the code. If I read “deleteAll(”1″)”, then it is not obvious what that means and I have to check the API to understand it. And with using “1 = 1″ as parameter you simply add noise to the code, which is not cool if you want clean code.
@Sliv: I think it is a typical usability problem, as for me as a user it is not obvious how to use this method for deleting all records, the most basic action you should be able to perform with this method. In my internal “model” (I don’t know the correct word for it at the moment) it is easy to perform this task, and a bit more complicated if there are conditions. So when you implement a function you should follow this pattern, or you will violate the users expectations (i.e. in this case the task which should be the easiest to perform has suddenly become the most difficult one).
@Rick: Yes, it would be a bit better if it is explained in the docs. On the other hand you have to ask yourself when writing the docs: “is it really necessary I have to explain this? Or is it simply a sign that there is something wrong in the code?”.
@Bruno: Yes, people make mistakes, and the scenario you described is possible. On the other hand you are in a development environment where it shouldn’t matter if you delete something by accident.
@Lobaman: Yeah, I can remember such guys and they hopefully learned their lessons ;-)
@derelm: I agree with you.
I agree with nate
Its good idea to add this kind of protection
uninted use of deleteAll can be harmful
@john doe: when will you ever call deleteAll when it’s not intended?
if you apply that logic you should add a condition to save, just in case you use it unintended :)
Because of condition parameter, I can use it to delete some records matching some particular conditions.
Lets say conditions are coming from user input. What if there is some flaw in client-side javascript, and passed variable will be empty?
I don’t want all records to be deleted.
This should be documented though, but maybe in future versions of tempdocs.
I agree with dho, and I agree with nate also.
PhpNut always says: to keep a clean house.
but i think different sound is more important.
IMO. i dislike Mariano Iglesias, “you are starting to sound like a bitchy ex-wife”
CakePHP need different sound.
[...] recently came across Daniel’s post, I see he did made a good point about a function not doing something which it was supposed to do. [...]
Personally I don’t mind whether you have to add a dummy parameter or not, as long as it mentions this in the docs.
If the method is going to fail when called with no parameters then I’d expect an error message (at least in debug mode) rather than failing silently. I hate things that fail silently. If I’m not told what’s wrong, I can’t fix it. Well, not as easily, anyway.
“I don’t like it if the framework tries to babysit me”
Amen to that. There are several places where cake tries to do this. I hate it.
@Nate - You do a great job, but every little decision you make is not impervious to question. Lot of people use the code you write. If they want to question things from time to time, you could minimally just not respond at all if you don’t agree. I think that would be much more productive than “this is just outright idiotic”. You make a ton of decisions on little things in cake, it is impossible that ALL of them are perfect.
As a way to potentially satisfy the ’safe by default’ goal while still avoiding the idea the necessity to put in a blank condition, just allow the condition for DeleteAll() to be the boolean value ‘true’.
$this->MyModel->deleteAll(true);
You can still ensure that an empty array doesn’t work, so you’re protected against accidental deletion, but you can explicitly call for a delete-everything without having to write an arbitrarily-true condition.
@dho: I think everyone should understand by now what the real problem is. I see people saying that your posts are very helpful, and you are helping the community. Fine. My problem with all this “ONE POST = ONE COMPLAIN” nonsense is that you were a CakePHP core developer for how long? A year? And while you were there, why didn’t you do anything to solve / workaround all these little issues you now suddenly find?
I don’t have a problem with people critizicing code. You call my comments “non-constructive criticism”, I call your posts the same way, because when you had a chance to do something about it nothing was being done.
When these feedback comes from people from the community I welcome it. Everyone in the Cake team learns constantly from user’s feedback, otherwise CakePHP wouldn’t be where it is now.
But then again, this is getting old. Let’s just move on, and hope that the next post will be constructive. For real.
Mariano, you’re right… but I guess it can also be defined as:
ONE POST = ONE COMPLAIN = A LARGE FEEDBACK OF USERS AND SO THE MORE CONTRIBUTION TO THE COMMUNITY
Its not just a complain, rather I would say these are the efforts to draw more people to the community and consider their suggestions.
On a topic like this for example, Its important to know what others are thinking, which will definitely help making Cake better.
I think dho has a strong point here. It’s so good of him to bring that up to the CakePHP community by blogging, just like the way I also wanted to do.
putting “1=1″ is not really a good practice,IMHO, to run an SQL statement without condition, and say this is a measure of protection to prevent accidentally deleting records. this issue is more like a programmer’s/designer’s responsibility to avoid accidents, troubles. How do you want to use a function that deletes all records but still needs a flag?
I agree with Daniel, framework should provide the functionality and the coder need to decide how to use it.
I also wanted to use this function in the past, but it wasn’t worked and I changed the way of doing it. I haven’t dig into the code, but now I realize what was the problem.
Am I seeing this right?
[https://trac.cakephp.org/browser/branches/1.2.x.x/cake/libs/cache/model.php]
/**
29 * Database Storage engine for cache
30 *
31 * @package cake
32 * @subpackage cake.cake.libs.cache
33 */
34 class ModelEngine extends CacheEngine {
[snip]
145 /**
146 * Delete all keys from the cache
147 *
148 * @return boolean True if the cache was succesfully cleared, false otherwise
149 * @access public
150 */
151 function clear() {
152 return $this->__Model->deleteAll(null);
153 }
154
155 }
156 ?>
Posts like this are a *very* powerful statement to how valuable a strong community is to an open source project! It’s great to see the many different views from everyone.
@Nate and Mariano
IMO, dho handled this situation correctly. He encountered an unexpected result, found the cause, documented it, and opened a ticket.
To me this post didn’t come across as a stab in the back but was both informative and educational. It shows a feature of the core that doesn’t work as expected. Isn’t that the point of a community - to teach others!
maybe when the final manual is release, some of these dummy conditions may go. since it was documented, no core dev will be blame. if you accidentally delete “ALL” your records.
so as for now, it is a good thing to have a dummy conditions like that. it is a consideration for the new user. i guess.
take a look for any delete in every OS. arent they ask you for confirmation? Why? simple because the OS doesnt want you to do something stupid. And how many times you accidentally deleted your files?
And sometime you are thinking, “How can i implement delete all records” ? hmmm. deleteAll();
and you type the code…..
$this->Model->deleteAll();
and press F5(refresh) it in your browser…
and you were like…
“What the fck! my data are all gone. Yeah i know! i ask for DeleteAll(). But cakephp should ask me for confirmation! Just like every OS i used. now im doomed. how am i gonna reconstruct it again.”
and you were like goto http://www.google.com and search for
“how to recover Deleted data in DB.”
sad.
@all: Thanks for your comments!
@john doe: The scenario you described is possible. On the other hand it is also possible that I have a similar scenario where it is desired that all records get deleted if there are no conditions. So it depends on the respective application on what should happen if there are no conditions defined, hence it is application-specific logic and shouldn’t be in the framework.
The framework should allow both scenarios, as both scenarios are valid. But the current implementation is quite restrictive: if there are no conditions, the method simply returns false. But what if I want to throw an exception in such a situation? I would have to re-implement the entire method in the AppModel…
@wangbo: I agree with you, that the way some members of the cake team communicate could be improved (not only here but in general, e.g. on trac).
@grigri: Yes, an error message would be helpful in such a case, even though it is better if such an error message is not needed at all ;-)
@Watts: Maybe it would be easier to remove the check from Model::deleteAll() and to add it to AppModel::deleteAll(). If you don’t need this functionality, you simply don’t add it to your custom AppModel.
@Mariano: Yes, I was a team member for around a year. And maybe I could have fixed more such issues. I don’t know. On the other hand I am not sure the reactions from the team would have been different at that time…
Anyway, whether you see this post (plus the comments) as nonsense or feedback is entirely up to you, it is your choice…
@grigri: Looks like a bug ;-)
@Lobaman: Thanks for your comment.
I fully agree with you if deleteAll() would be a part of a command line tool and would delete all records without any confirmation (even though you could also argument that it would be better to provide an undo function instead of a confirmation dialog, but I digress).
If you want to compare the situation with an OS, then the OS is your application and CakePHP is the “programming language” in which the OS is implemented. The “programming language” provides you for example with the functionality to delete files. But it doesn’t know whether there should be a confirmation. That’s up to the OS, it may ask you for confirmation if you delete files manually, but it will delete tmp files automatically without confirmation.
The same approach is used by the idea I described in the answer to Watts: to put the following code into the AppModel:
function deleteAll($conditions, ...) if (empty($conditions)) { return false; } parent::deleteAll($conditions, ...); }This gives you the flexibility to decide yourself whether you want such a safety net, a different safety net, or no safety net.
I have to say i like this precaution and i’m glad this post exists to explain the pros, the cons and the use of it.
dho, I think that such small glitches helps me a lot to understand the framework and go deeper into it. Thanks for that.
The “Are you sure?” parameters are more like a personal preference than necessity in the core code. It doesn’t hurts putting three letters and no it doesn’t gives extra protection. You can force the programmer to pass a parameter: “array(’1=Yes, I wan’t to delete all the records. I’m sure I want to do that. Yes I know what I’m doing!’ ” and still to accedently delete all the records.
Mariano Iglesias, will you stop being bitch and start giving some productive (or at least technical) comments? You sound like a priest who gets offended about his religion.
@purepear, Proleter: Thanks for your comments!
@Proleter: Yes, it is like the messages the OS shows you when you want to delete some files. They don’t prevent that you delete the wrong files.
The solution to this problem….
$delete_all_records = 1;
$this->MyModel->deleteAll($delete_all_records);
It’s obvious when you read over the code what it does and I doubt you could do it inadvertently :)
@ifunk: Thanks for your comment!
Yes, if I read your snippet it is obvious what it does. On the other hand it introduces some redundancies…
what about the slightly less redundant $this->MyModel->deleteAll($confirmdelete = true);
Or something. I haven’t tried it but I think it should pass a true boolean to the function.
@Tim: Yes, that would be better :)
Ok, coming in a little late…
how about a constant?
define(’DELETE_ALL_RECORDS’, true);
// ideally a class constant, maybe Model::$DELETE_ALL_RECORDS?
Just for use as a flag really. I like the idea of using “true” rather than just assuming blank conditions mean you want to do it (could be a bug for instance). A little conscious thought isn’t always a bad thing, and TRUE is a lot more intuitive than “1 = 1″.
@Cameron: At least to me it is redundant if you have to write something like:
But as you see in the other comments, half of the people think it is good to have such a flag, and the other half think it is not necessary ;-)