Tag: cakephp beforesave

More pondering about HABTM (let's save new tags with a post)

April 21st, 2009 · · CakePHP No comments - Tags: , , , ,

There is a number of examples (some you can find even on this blog) on how to save a Post with a few Tags.
Most of them, so far, had shown how to easily save Tags when the ID’s are already known.

Now, we’ll take a look at how to save a Post and allow a user to enter a bunch of tags (hmmm… let’s say separated by a comma) and save it all together with very little hassle.

Let’s define some goals first:

  • We would like to allow the user to pick from a list of existing tags
  • We would like to allow the user to input a bunch of tags separated by comma
  • We need to ensure that we do not save already existing tags
  • If the user doesn’t select or input any tags, we should just go ahead and save the Post
  • If the user does something silly like input “some, some, some, tag”, we should only save two tags from the list

I think that should be good enough to get started…

First we’ll, build a simple a form add.ctp:

  echo $form->create();

  echo $form->inputs(array('name', 'body'));
  echo $form->input('Tag', array('multiple'=>'checkbox'));
  echo $form->input('Tag.new_tags');

  echo $form->end('Save');

As you see, we’ll have a bunch of checkboxes to allow the user to pick from existing tags, we’ll also allow a free-hand input for “new tags” $form->input(‘Tag.new_tags’);

Now, let’s see our “add” action in the controller:

function add() {

   if (!empty($this->data)) {
      $this->Post->save($this->data);
   }

  $this->set('tags', $this->Post->Tag->find('list'));
}

Nothing out of the ordinary so far. Note that, $this->set(‘tags’, $this->Post->Tag->find(‘list’)); will let CakePHP automagically build a bunch of checkboxes from existing tags in the DB.

Now, let’s add a little something-something to our Post model:

   function beforeSave() {

        $tagIds = $this->_getTagIds();

        if(!empty($tagIds)) {
            if(!isset($this->data['Tag']['Tag'])) {
                $this->data['Tag']['Tag'] = $tagIds;
            }
            else {
               foreach($tagIds as $tagId) {
                    $this->data['Tag']['Tag'][] = $tagId;
               }
            }
        }

        return true;
    }

    function _getTagIds() {
        $tags = explode(',', $this->data['Tag']['new_tags']);

        if(Set::filter($tags)) {
            foreach($tags as $tag) {

                $tag = strtolower(trim($tag));

                $existingTag = $this->Tag->find('first', array('conditions' => array('Tag.name' => $tag)));

                if(!$existingTag) {
                    $this->Tag->create();
                    $this->Tag->saveField('name', $tag);

                    $tagIds[] = $this->Tag->id;
                }
                else {
                    $tagIds[] = $existingTag['Tag']['id'];
                }
            }

            return array_unique($tagIds);
        }

        return false;
    }

Let’s examine what’s happening here…

In beforeSave() we prepare the array of Tag ID’s to be saved, by relying on CakePHP’s HABTM conventions.
We’ve also created a custom function _getTagIds() to process the input from the user from the “new_tags” field (remember our add.ctp?)

So, if the user did not provide any input we simply go ahead and save the Post (and possibly some selected tags), otherwise we check if the tag already exists and if so, grab the ID. If it does not exist we save it, and then grab the ID.

Once all that is accomplished, we go ahead and properly prepare $this->data['Tag']['Tag'] to save our Post and Tag relationship.

Of course, any questions and suggestions about the code are as always welcomed.

P.S. If you are completely lost about what’s going on here, I suggest you check out this post, and then come back to review the code in more detail.

Controller makeover (from ugly to beautiful)

August 12th, 2008 · · CakePHP 13 comments - Tags: , , , , ,

Let’s take a controller ridden with problems and see how we can improve it. Hopefully this little experiment can help you beautify your code and optimize your app…

This is our ugly Users controller:

class UsersController extends AppController {

var $name = 'Users';
var $uses = array('User', 'Profile');

function add() {
   if(!empty($_POST)) {
      $this->User->create();
      $this->User->set($this->data);

      if($this->User->validates()) {
            //create the process_type hash
            $this->data['User']['process_type_hash'] = md5($this->data['User']['process_type']);

           //prepare user code messages
          switch($this->data['User']['error_code']) {

               case('00'):
                   $this->data['User']['error_message'] = 'Success';
               break;

               case('01'):
                   $this->data['User']['error_message'] = 'User suspended';
               break;

               case('02'):
                   $this->data['User']['error_message'] = 'User account cancelled';
               break;

               case('03'):
                   $this->data['User']['error_message'] = 'User not verified';
               break;
         }

         $this->User->save($this->data);
         $userId = $this->User->getLastInsertId();

         $this->Profile->create();

         if($this->Profile->validates()) {
                 $this->data['Profile']['user_id'] = $userId;
                 $this->data['Profile']['temp_id'] = $this->generateTempId();

                 $this->Profile->save($this->data);
         }
     }
  }
}

function view($id=null) {
        $this->set('users', $this->User->findAll('id='.$id));
        $this->set('profiles', $this->Profile->findAll('user_id ='.$id));
}
}

Let’s analyze the problems line by line and see what we can do it make it better…

Line 4
The $uses array is not needed (as in the vast majority of cases). Our models are related, therefore both User and Profile are already available to your controller

Line 7
When checking for POST’ed data, it’s best to rely on $this->data rather than $_POST

Line 8
No need to create() a model in this case, save() will handle that for you later on.

Line 9
This line really depends on the validates() method. Generally speaking there is no need to call this method separately, since it is called by save() for you, therefore setting the model’s data is no longer necessary.

Line 11
Based on the point above, we will not call validates() method, but will rely on save() instead.
Well, what about the data we need to prepare before saving?…

Lines 12 – 33
This work should never be done in the controller, and definitely not in this manner. Considering our points above, whenever we have any data, which has to be processed/prepared prior to save(), we should build a beforeSave() method in a given model to accomplish that.

Line 35
Calling save() without a false param, (as in save($data, false)) will trigger validation again. Either we get rid of the validates() method and rely on save() to handle validation for us, or we accept that data is already validated and we use ‘false’. Regardless of the approach, certainly there is no need to validate the same data twice.

Line 36
There is no need to call getLastInsertId(); After saving model’s id is automatically set to the last inserted record’s id. So in our case $this->User->id is already set.

Lines 38 – 45
Related models can be easily saved in one shot with saveAll(), there is simply no need of all this extra code.

Line 42
Looks like we’ve made some utility function in App Controller to generate our temp id. There are a few problems with this approach… First of all, the function is not protected, so we could access it by a URL, which is probably not desired. An easy way to avoid this would be to rename the function to function _generateTempId(), then you’d call it like $this->_generateTempId();. Still, this type of processing is better done in the model’s beforeSave() method.

Lines 51-52
We’ve got a few problems with our view() action. First of all our models are related, so we can easily retrieve the information for both by using $this->User->find(‘all’); Secondly, findAll() is deprecated so it’s best to use find(‘all’) instead. Third, the conditions, while working, are written poorly. A better syntax would be: find(‘all’, array(‘conditions’=>array(‘User.id’=>$id)));
And last, but not least, we are not checking for the value of $id prior to find(). If $id is indeed ‘null’, we should not call find() at all (a simple if() statement should handle it for us).

So what would a fixed-up controller look like?

class UsersController extends AppController {

    var $name = 'Users';

    function add() {
        if(!empty($this->data)) {
           $this->User->saveAll($this->data, array('validate'=>'first'));
        }
    }

    function view($id=null) {
        if(!empty($id)) {
           $this->set('users', $this->User->find('all', array('conditions'=>array('User.id'=>$id))));
        }
    }

}

That’s all folks. A lot cleaner and prettier.

Well, what happened with all the code?

saveAll() will handle validation and saving of our User and Profile models, so all that code above can be easily condensed into a single line of code.

The data preparation i.e. the hash and error message creation will be moved into the beforeSave() of the User model.
And the preparation of temporary id will be moved to beforeSave() of the Profile model.

One note here, is that placing such a switch statement directly into your beforeSave() method is ugly. Instead, make a little utility function that you could use to process your data, for example:
$this->data = $this->_processErrorMsgs($this->data); … and move your switch code block into that utility function, it will make your code more flexible in the long run.

The main point here is that if you feel/see that your controllers are getting a little fat and your actions seems to have all sorts of ‘if’ statements, consider taking a step back and evaluating whether some of the functionality should be offloaded to models, utility methods or plain old simplified by using cake’s built-in tools.