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

Zcalenie edit() i add() w kontrolerze

Gdy przyjrzysz się bliżej kontrolerom stworzonym przy pomocy narzędzia ‘bake’ (lub takim, jakie powstają po wykonaniu tutoriala) możesz stwierdzić, że łamią one koncepcję DRY. Jesteś w stanie powiedzieć, które z dwóch metod są niemal identyczne? Jeśli nie – sprawdź, które dwa widoki są niemal identyczne. Ok, może spaliłem swoją zagadkę, bo w tytule tego postu jest wyraźna sugestia o czym będę mówił. Zgadza się.

Najpierw rzućmy okiem na widoki (tu w przykładzie dla Modelu Thing, który ma jedno pole – 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>

Różnią się dokładnie w trzech linijkach (na 16 łącznie). Jeśli zmienisz coś w jednym (na przykład dodasz pole) to będziesz musiał poprawić formularz w dwóch plikach. Czysta głupota.
Przyjrzyjmy się jeszcze metodom:

// 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);
   }
}

Praktycznie całe ciało metody add() można by wkleić w miejsce drugiego if’a w metodzie edit() i niewiele by się zmieniło, prawda?

Kilka tricków pozwoli nam naprawić to marnotrawstwo. W zasadzie najważniejsza kwestia jest następująca – jak działa metoda $this->Thing->create()? Wymusi dodanie nowego elementu (dlatego, że metoda save() działa jako create lub update w zależności od stanu modelu). Ale nie przekazując żadnych parametrów do tej metody tak naprawdę sprowadzi się do wyczyszczenia $this->data i $this->id (this oznacza w tym przypadku model). W przypadku metody add() nie ma możliwości, żeby jakieś śmieci były w $this->Thing->data, więc możemy spokojnie usunąć tą linijkę – dalej będzie działać tak samo (czy na pewno? – spoiler).
Teraz całkowicie ciało metody add() mogłoby zawrzeć się w metodzie edit(). Dlatego trzeba ją usunąć (razem z jej widokiem). A wszystkie linki, które prowadziły do /things/add niech teraz prowadzą do /things/edit.
Warto sprawdzić, co się teraz dzieje?

W zależności od wersji – 1.2 się wywali, 1.3 pokaże informacje “Invalid thing”. Teraz skupię się na wersji 1.3 – bo na niej sprawdzam na bieżąco efekty pisząc ten wpis. (dla 1.2 jeśli dobrze pamiętam wystarczy wstępnie zmienić sygnaturę metody edit($id) na edit($id=null), dalsze kroki będą analogiczne, choć kod w szczegółach może się różnić)

Od teraz możemy założyć, że jeśli w url’u nie ma podanego id – to znaczy, że chcemy dodać nowy element, zatem tego if’a możemy usunąć:

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

Możemy spokojnie teraz dodawać nowe elementy przez formularz- wszystko działa.
Warto jeszcze dopieścić widok – bez sensu jest nagłówek “edit” przy dodawaniu elementu, oraz przycisk “delete”,
fragment z legend poprawiamy na taki (dodałem wcięcia dla przejrzystości):

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

a pierwszy element li otaczamy if’ami tak:

// 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; ?>

I tyle.

Usunięcie $this->Thing->create() nie takie bezpieczne?
Napisałem, że po usunięciu tego elementu nic się złego nie stanie. Oczywiście jest to prawdą pod warunkiem, że wykonasz wszystkie czynności opisane w tym poscie. Jeśli tylko usuniesz $this->Thing->create() narażasz się na niebezpieczeństwo, że ktoś wyśle spreparowanego post’a pod ten adres. Jeśli poda w nim pole o nazwie data[Thing][id] – będzie mógł zmienić dane dla elementu o danym id. Jeśli masz sytuację taką, że jedna grupa może dodawać elementy, a inna je edytować i polegasz tylko na acl’ach (dana grupa ma dostęp do edit(), a kolejna do add()) – to w ten sposób możesz stworzyć lukę. Jednak ta sytuacja jest dość rzadka i to jest raczej temat na inny wpis, więc jeśli interesuje Cię rowiązanie tego problemu – zachęcam do komentowania.

Share Button

AppView – wrapper View zupełnie jak AppController?

Gdy potrzebujesz wrappera dla widoków (takiego jak masz dla kontrolerów i modeli odpowiednio AppController i AppModel), wystarczy kilka prostych kroków:
1. W AppController dodaj

var $view = "App";

2. w /app/views/ utwórz plik App.php, jego zawartość to:

class AppView extends View {
}

Tyle :D (dzięki Ad7Six na http://groups.google.com/group/cake-php/browse_thread/thread/3d9e3449718f5114).

Po co to komu? Mnie akurat przydało się, żeby mojemu lenistwu stało się zadość. Nie chciało mi się w widokach pisać długich ścieżek do prototype:

echo $javascript->link('/forum/js/scriptaculous-js-1.8.2/lib/prototype', false);
echo $javascript->link('/forum/js/scriptaculous-js-1.8.2/src/scriptaculous', false);

więc utworzyłem AppView:

class AppView extends View {
var $prototype = '/forum/js/scriptaculous-js-1.8.2/lib/prototype';
var $scriptaculous = '/forum/js/scriptaculous-js-1.8.2/lib/prototype';
}

i teraz załączanie tych bibliotek wygląda tak:

echo $javascript->link($this->prototype, false);
echo $javascript->link($this->scriptaculous, false);

Oczywiście w razie, gdybym podmieniał biblioteki na nowsze – zrobię to tylko w jednym miejscu

Share Button