A black sheep with the name Model::updateAll()

Published on and tagged with cakephp  model

Last week I wrote about Model::deleteAll(), today I want to have a look at its brother: Model::updateAll(). I call it a black sheep as its usage is quite different from what you would expect if you know its brothers Model::deleteAll() and Model::findAll().

I think it is best if we do a simple example. Before we start, let’s have a quick look at the API:

/**
 * Allows model records to be updated based on a set of conditions
 *
 * @param array $fields Set of fields and values, indexed by fields
 * @param mixed $conditions Conditions to match, true for all records
 * @return boolean True on success, false on failure
 * @access public
 */

Looks fine, and should be easy to use. So we are ready to code.

Let’s implement a method to soft-delete all finished events. We may come up with the following method in our Event model:

function removeAllFinishedEvents() {
    $this->updateAll(array('Event.finished' => false, 'Event.removed' => true), array('Event.finished' => true, 'Event.removed' => false)); 
}

The call of updateAll() looks correct, doesn’t it? Unfortunately, it doesn’t work. You will get some SQL errors, as the generated SQL is wrong:

UPDATE `events` SET `Event`.`finished` = ,`Event`.`removed` = 1 WHERE `Event`.`finished` = 1 AND `Event`.`removed` = 0

The first issue — UPDATE doesn’t use an alias for the table name for some reason — is easy to solve, we simply remove the aliases in our code:

function removeAllFinishedEvents() {
    $this->updateAll(array('finished' => false, 'removed' => true), array('finished' => true, 'removed' => false)); 
}

But the generated SQL still fails:

UPDATE `events` SET `finished` = ,`removed` = 1 WHERE `finished` = 1 AND `removed` = 0

It seems that the “false” value we provided in the first array is interpreted as an empty string (even though it doesn’t get quoted). So let’s provide 0 as a string:

function removeAllFinishedEvents() {
    $this->updateAll(array('finished' => '0', 'removed' => true), array('finished' => true, 'removed' => false)); 
}

And voilà, it works.

It wasn’t as easy as we thought, and we encountered different issues: no table alias is used, and the data types of the values are not recognized.

The problem is, at least partially, that we didn’t use updateAll() in the way it is supposed to be used. To quote a comment by Nate:

“The purpose of updateAll() is to allow updating of fields based on calculated values of other fields.”

That means it is only thought to be used for stuff like:

$this->updateAll(array('price' => 'price + 1'));

Not really obvious and logical…

22 comments baked

  • Frank

    **YAWN**

    again, things not going your way, Daniel? Why didn’t you provide input to the CakePHP team while you were part of it? With every one of your posts the reasons for you getting kicked from the CakePHP core team become more obvous: Things have to go your way, and if they don’t, you diffame the solutions that were chosen as being “unlogical”.

    Dream on, outsider.

  • Richard@Home

    This cakebaker bashing is getting a bit old.

    At no point does cakebaker have a go at anyone, he’s simply documenting a bit of code usage that behaved differently from what he expected and hopefully save someone from going down a blind alley in future.

    Thanks for pointing this out cakebaker. I was sure to stumble into this a little later in my current project.

  • Edmunds

    Thanks for the heads up, Daniel.

    And Frank — having a bad day or something?

  • NOSLOW

    I love Cake and I love learning about these small nuances of it through this blog. The comments (good and bad) just makes it more interesting.

    Having an open discussion on even the smallest nit-picking things is a good thing. It forces us all to think and consider how even the smalled decisions we make during coding can have a long impact on us later down the road.

    As with any sizeable project, our state of mind and reference will vary as time goes by. And to mix in other programmers along the way, not everyone will always agree on everything. At some point, you just have to make a choice and move forward.

    When it’s all said and done, (such in life and programming) it’s easy to look back and compare the numerous choices that you’ve made and point out the inconsistencies and such, and argue (with your new current perspective) why things should have been done differently.

    Now, back to the immediate subject: That thing about having to alias my conditions in some places and having to leave them out in others within the same framework is going to drive me mad! I’d love to hear the pros/cons/reasons on that particular debate.

  • nate

    Having an open discussion on even the smallest nit-picking things is a good thing. It forces us all to think and consider how even the smalled decisions we make during coding can have a long impact on us later down the road.

    Not necessarily true. This is also known as “bike shed painting” – see http://www.bikeshed.com/ or http://en.wikipedia.org/wiki/Color_of_the_bikeshed

    Also keep in mind that this particular method is still very much a work-in-progress, and that some of the issues Daniel has pointed out are things I plan to resolve before I consider it “completed”. One or two of the issues Daniel pointed out are legitimate problems which just haven’t come up before and deserve to be corrected. However, the rest of the issues addressed here are simply a logical consequence of how the method was designed, and if an attempt was made to “fix” them, the result would be that the method no longer works for it’s intended use case.

    So here’s my parting thought: consistency is important, context is more important (hint: this is key in understanding the parameter order), but neither of those excuse you from understanding how your tools work, or why they work that way.

  • Mariano Iglesias

    This applies so much to this situation that is freakish: “Just because you are capable of building a bikeshed does not mean you should stop others from building one just because you do not like the color they plan to paint it. This is a metaphor indicating that you need not argue about every little feature just because you know enough to do so. Some people have commented that the amount of noise generated by a change is inversely proportional to the complexity of the change.”

    Now we just need to repeat:

    “you need not argue about every little feature”

    Come on, try again:

    “you need not argue about every little feature”

    One more time wouldn’t hurt:

    “you need not argue about every little feature”

    ;)

  • Abhimanyu Grover

    LOL, Its getting too much now. I will take my words back now. Maybe this is just done to create controversies.

    @Mariano: You said it right in the last post.

  • cakebaker

    @all: Thanks for your comments!

    @Frank: Good question. I made mistakes, no question. But I cannot turn back the time, what happened happened. The only thing I can do is to learn from it and to move forward. (and regarding providing no input I recommend you have a look at trac)

    Things don’t have to go my way, but I am a critical guy and so I question things. That’s part of my job as a software developer if I want to write good software. I have to question my design decisions, especially if I am writing a framework.

    And if you look at the article, what’s wrong if I describe the way I expect the method to work and how it is effectively used? And that I think it is not logical that a special case is supported by the framework, but a more general use case is not supported?

    @Richard, Edmunds: Thanks!

    @NOSLOW: Yes, programming is a continuous learning process. What you did one year ago, would look differently if you did the same today.

    I think such details are important for the user experience. If something works as expected you experience it as pleasant, whereas if you have to read documentation/code before you can use something, then the user experience is probably not that good.

    @nate: Well, the issues I described here were already reported as a bug almost a year ago: https://trac.cakephp.org/ticket/1976. But the ticket was closed as “invalid”…

    @Mariano: What’s wrong with documenting the problems I encounter while using the framework? It is feedback for you and the team, you can use it or ignore it. It is up to you.

    @Abhimanyu: Well, it don’t do this to create controversies but because I encountered these issues myself. And so I document them.

  • NOSLOW

    @Nate: Point taken. Your references to wikipedia never cease to amaze me :). CakePHP, to me, is certainly more like that billion dollar science lab. I realize it’s a work in progress, though, and apologize for distracting you from the incredible job you’re doing with CakePHP. Just keep up the *consistently* good work ;)

    @Mariano: Also agreed. But it gets even freakier: If you lived in my subdivision (in the middle of Suburbia, USA), there would be no debate on the building of the bike shed: each would be the same size, style, color and location on your lot. To build any differently would require arguing your reasons to the home owner’s association (a community forum) why you would insist on breaking the established pattern. Although I may not totally agree with that sort of “establishment rule” in a physical community, I think it works well for a software framework. In varying contexts, though, I realize consistency may not always apply.

    Overall, I think the CakePHP framework is an amazing piece of work and appreciate the efforts of all involved. The developers have proven themselves in my mind, and I’m not one to question their choices. I’ll leave that to Cakebaker…and will learn more about Cake as he blogs about it.

  • Tim Daldini

    Blah.

    Keep up the good work Cakebaker. This WILL be helpful to people using the framework.

  • cakebaker

    @Tim: Thanks!

  • domin

    yet another confusing usage of cake api :/ it makes me sick. keep doing your work cakebaker! it’s needed!

  • cakebaker

    @domin: Yes, some stuff is quite confusing…

  • Abhimanyu Grover

    LOL, I can’t believe I just used “cakebaker blacksheep” keywords to find this article..

  • cakebaker

    @Abhimanyu: *g*

  • Bruno Bergher

    Hi,

    I ended up here while looking for a solution to my problem: my $model->updateAll(…) calls where returning true even if no rows had been updated.

    I then realized it’s not a Cake problem, but a logic error by myself. I’m writing just to give the tip to someone which might have the same problem.

    If you need to check if an update was successful in the sense of actuallt changing something, use the return of $model->getAffectedRows() after the call to $model->updateAll(…) to determine it.

    That’s it. Thanks for the space : )

  • cakebaker

    @Bruno: Thanks for the hint!

  • pragnatek

    Thanks cakebaker,

    I just couldn’t get the syntax of updateAll() correct until I found your article.

    Thanks

  • cakebaker

    @pragnatek: You are welcome! And good luck with your project!

  • xkunalx

    Daniel, thanks this was just what I was looking for. But I STILL cant get it to work. DOes updateAll() allow me to use Invoice->User->updateAll() ?

    That fails with a SQL error (shown below). If i remove the condition it works..but I need to add a condition here.

    $this->Invoice->User->updateAll( array(
    ‘plan_id’ => $trial_plan_id,
    ‘trial_invoked’ => date(“Y-m-d H:i:s”),
    ‘trial_endtime’ => date(“Y-m-d H:i:s”, $trial_endtime),
    ),
    array(‘id ==’ => “1”)
    );

    SQL error
    SQL Error: 1064: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ’22:13:36, `User`.`trial_endtime` = 2009-07-15 22:13:36 WHERE `id` = 1′ at line 1 [CORE\cake\libs\model\datasources\dbo_source.php, line 525]
    Query: UPDATE `users` AS `User` LEFT JOIN `plans` AS `Plan` ON (`User`.`plan_id` = `Plan`.`id`) SET `User`.`plan_id` = 2, `User`.`trial_invoked` = 2009-07-08 22:13:36, `User`.`trial_endtime` = 2009-07-15 22:13:36 WHERE `id` = 1

  • cakebaker

    @xkunalx: Thanks for your comment!

    Well, it seems like you have to treat the dates as strings, and so you have to use:

    ‘trial_invoked’ => "'" . date(”Y-m-d H:i:s”) . "'",
    

    Hope that helps!

  • redthor

Bake a comment




(for code please use <code>...</code> [no escaping necessary])

© daniel hofstetter. Licensed under a Creative Commons License