How to merge edit() and add() methods in cakephp…

When You get a closer look at baked controllers You can get the idea that they’re breaking DRY principle.

Can You tell which of two methods in baked controller are almost identical? If not – check out which two views are even more similar. It’s not much of a riddle if You read title of this post again, though. That’s right.

Lets look at views first (in this example we have Thing model wit one field called ‘name’):

// app/views/things/add.ctp
<div class="things form">
<?php echo $this->Form->create('Thing');?>
   <fieldset>
      <legend>
         <?php printf(__('Add %s', true), __('Thing', true)); ?>
      </legend>
   <?php
      echo $this->Form->input('name');
   ?>
      </fieldset>
<?php echo $this->Form->end(__('Submit', true));?>
</div>
<div class="actions">
   <h3><?php __('Actions'); ?></h3>
   <ul>
      <li>
         <?php echo $this->Html->link(
            sprintf(
               __('List %s', true), 
               __('Things', true)
            ), 
            array('action' => 'index')
         );?>
      </li>
   </ul>
</div>
// app/views/things/edit.ctp
<div class="things form">
<?php echo $this->Form->create('Thing');?>
   <fieldset>
      <legend>
         <?php printf(__('Edit %s', true), __('Thing', true)); ?>
      </legend>
   <?php
      echo $this->Form->input('id');
      echo $this->Form->input('name');
   ?>
   </fieldset>
<?php echo $this->Form->end(__('Submit', true));?>
</div>
<div class="actions">
   <h3><?php __('Actions'); ?></h3>
   <ul>
      <li>
         <?php echo $this->Html->link(
            __('Delete', true), 
            array('action' => 'delete', $this->Form->value('Thing.id')), 
            null, 
            sprintf(
               __('Are you sure you want to delete # %s?', true), 
               $this->Form->value('Thing.id')
            )
         ); ?>
      </li>
      <li>
         <?php echo $this->Html->link(
            sprintf(__('List %s', true), __('Things', true)), 
            array('action' => 'index')
         );?>
      </li>
</ul>
</div>

They differ in 3 line (of 16). If there’s a change in model (ie. adding one field) – It’s needed to edit two files. That’s stupid.
Lets take a look at methods:

// app/controllers/things_controller.ctp
function add() {
   if (!empty($this->data)) {
      $this->Thing->create();
      if ($this->Thing->save($this->data)) {
         $this->Session->setFlash(__('The thing has been saved', true));
         $this->redirect(array('action' => 'index'));
      } else {
         $this->Session->setFlash(
            __('The thing could not be saved. Please, try again.', true)
         );
      }
   }
}

function edit($id = null) {
   if (!$id && empty($this->data)) {
      $this->Session->setFlash(__('Invalid thing', true));
      $this->redirect(array('action' => 'index'));
   }
   if (!empty($this->data)) {
      if ($this->Thing->save($this->data)) {
         $this->Session->setFlash(__('The thing has been saved', true));
         $this->redirect(array('action' => 'index'));
      } else {
         $this->Session->setFlash(
            __('The thing could not be saved. Please, try again.', true)
         );
      }
   }
   if (empty($this->data)) {
      $this->data = $this->Thing->read(null, $id);
   }
}

It’s clear that edit() metdod encloses add() method body. There would be no much change if You just paste add() method into second if statement, wouldn’t it?

We’ll fix it with few tricks. We need to know how $this->Thing->create() exactly works. It forces adding of a new element into DB (because save() method works as ‘update’ or ‘create’ and it depends on current model state). But if You don’t pass any parameters to create() method the only thing it does is cleaning $this->data and $this->id (‘this’ means model Thing in current context).
There’s no way that $this->Thing->data contains any junk while executed, so it’s safe to drop this line – it will still work (is it?! – spoiler).
Now whole method body is identical with code block in edit method. Lets drop add() method and it’s view. Alter old links pointing at /things/add to /things/edit/ (without param).
Lets try out how does it work right now?

Based on version – 1.2 will crush, 1.3 shows “Invalid thing” alert. I’ll focus on 1.3 version (as far as I remember changing method signature from edit($id) to edit($id=null) should get You back on track in 1.2).

From now on we can assume that when there’s no id in url – it means that someone want to add new Thing, so it’s ok to delete this if statement:

// app/controllers/things_controller.php
		if (!$id && empty($this->data)) {
			$this->Session->setFlash(__('Invalid thing', true));
			$this->redirect(array('action' => 'index'));
		}

Now we can add new Things through edit() form- everything works ok now.
It’s good to polish this view, though. “edit” header look silly while adding new Thing… so delete button.

pimp up Your legend tag:

// app/views/things/edit.ctp
 		<legend>
 		   <?php printf(
 		      (!is_null($this->data["Thing"]["id"])?__('Edit %s', true): __('Add %s', true)),
 		      __('Thing', true));
 		   ?>
 		 </legend>

and surround first li element in if block:

// app/views/things/edit.ctp
<?php if(!is_null($this->data["Thing"]["id"])): ?>
   <li>
      <?php echo $this->Html->link(
         __('Delete', true), 
         array('action' => 'delete', $this->Form->value('Thing.id')), 
         null, 
         sprintf(
            __('Are you sure you want to delete # %s?', true),
            $this->Form->value('Thing.id')
         )
      ); ?>
   </li>
<?php endif; ?>

That’s it.


Removing $this->Thing->create() not so secure?

I said that after deleting this line nothing bad will happen.
It’s true, but only if You make all the changes explained in this post. If You just remove $this->Thing->create() You’ll get an security issue. Someone could send prepared post request to this address with field named “data[Thing][id]” and edit an element through add action.
If you heavily depends on ACL authorisation, and one group of users can add Things, and other can edit them – member of the first group can raise his privileges by well prepared post request.
I think it’s rather rare situation. If You are interested in fixing this – please comment this post.

Share Button

Leave a Reply

Your email address will not be published. Required fields are marked *