Call the expert: A refactoring story (Part 4/5)

In the last part, we have seen how to move code from the Controller to the Model. The principle is quite simple, you want thin controllers and fat models.

Today, we will see how the new form framework can help us keep this clean separation of concerns.

When the user is logged in as an administrator, he can update the information of each product and upload a new picture.

Product editing

This page is managed by the edit action of the product module:

public function executeEdit()
{
  $this->product = ProductPeer::retrieveByPk($this->getRequestParameter('id'));
  if (is_null($this->product))
  {
    $this->forward404();
  }
 
  $this->form = new ProductForm($this->product);
 
  if (sfRequest::POST == $this->getRequest()->getMethod())
  {
    $this->form->bind($this->getRequestParameter('product'), $this->getRequest()->getFiles('product'));
    if ($this->form->isValid())
    {
      // get the current product
      $product = $this->form->getObject();
      $image = $this->form->getValue('image');
      $filename = $product->getImage();
 
      if ($image)
      {
        // remove old image
        if (file_exists($file = sfConfig::get('sf_web_dir').'/images/products/'.$currentImage))
        {
          unlink($file);
        }
      }
 
      // update the product
      $product = $this->form->updateObject();
 
      if ($image)
      {
        // save new image
        $filename = sha1($image->getOriginalName()).$image->getExtension($image->getOriginalExtension());
        $image->save(sfConfig::get('sf_web_dir').'/images/products/'.$filename);
      }
 
      $product->setImage($filename);
 
      // save the product
      $product->save();
 
      $this->getUser()->setFlash('notice', sprintf('The product "%s" has been updated successfully.', $product->getTitle()));
      $this->redirect('product/index');
    }
  }
}
 

The code is self-explanatory:

  • The product is retrieved from the Model (Product)
  • A form is created for the given product (ProductForm)
  • And if the form is posted (POST request method)
    • The form is validated
    • The product is updated

Most of the code manages the image upload process:

  • If an image is uploaded, remove the old one
  • Save the new image with a unique name
  • Change the image path for the Product object

But the code that manages the image upload process does not really belongs to this Controller, does it?

It is pretty simple to know if the code is in the right place. Ask yourself: How can I reuse the ProductForm form elsewhere in my application? In this specific case, you will have to duplicate a lot of code. And you know there is a better way to do this. So, it is time to refactor.

If you have read the "symfony forms in action" book, you know that the form framework allows a clean separation between the Controller (the form itself), the Model (the validators), and the View (the widgets).

So, let's move some code from the action Controller to the form Controller. I want the action Controller to look like this after the refactoring:

public function executeEdit()
{
  $this->product = ProductPeer::retrieveByPk($this->getRequestParameter('id'));
  if (is_null($this->product))
  {
    $this->forward404();
  }
 
  $this->form = new ProductForm($this->product);
 
  if (sfRequest::POST == $this->getRequest()->getMethod())
  {
    $this->form->bind($this->getRequestParameter('product'), $this->getRequest()->getFiles('product'));
    if ($this->form->isValid())
    {
      $product = $this->form->save();
 
      $this->getUser()->setFlash('notice', sprintf('The product "%s" has been updated successfully.', $product->getTitle()));
      $this->redirect('product/index');
    }
  }
}
 

The code we have removed must go somewhere. The first idea is to move it to the form save() method. But there is a better place. The method we want to override is updateObject(). The updateObject() method is where symfony update the object with the cleaned up values. This method is called by save() within a database transaction.

So, here is the new ProductForm code:

class ProductForm extends BaseProductForm
{
  public function configure()
  {
    unset($this['created_at']);
 
    $this->widgetSchema['image'] = new sfWidgetFormInputFile();
    $this->validatorSchema['image'] = new sfValidatorFile(array('required' => false));
  }
 
  public function updateObject()
  {
    $image = $this->getValue('image');
    $filename = $this->object->getImage();
 
    if ($image)
    {
      // remove old image
      if (file_exists($file = sfConfig::get('sf_web_dir').'/images/products/'.$filename))
      {
        unlink($file);
      }
    }
 
    // update the product
    parent::updateObject();
 
    if ($image)
    {
      // save new image
      $filename = sha1($image->getOriginalName()).$image->getExtension($image->getOriginalExtension());
      $image->save(sfConfig::get('sf_web_dir').'/images/products/'.$filename);
    }
 
    $this->object->setImage($filename);
  }
}
 

As an added bonus, we only have to save the Product object once instead of twice as previously. The code is better encapsulated as we have a fairly standard executeEdit() action code that is self-explanatory and the ProductForm can be reused elsewhere with very little effort.

But now that the logic is in the form, we can do even better. As we have now direct access to the values variable, we can refactor the code like this:

public function updateObject()
{
  if ($image = $this->getValue('image'))
  {
    // remove old image
    if (file_exists($file = sfConfig::get('sf_web_dir').'/images/products/'.$this->object->getImage()))
    {
      unlink($file);
    }
 
    // save new image
    $filename = sha1($image->getOriginalName()).$image->getExtension($image->getOriginalExtension());
    $image->save(sfConfig::get('sf_web_dir').'/images/products/'.$filename);
  }
 
  unset($this->values['image']);
 
  // update the product
  return parent::updateObject();
}
 

The trick is here is to remove the image value from the cleaned up values after we have processed it. That way, the default updateObject() method of symfony won't update the image column. You know that Vince want his code to be perfect, so I even make it better by creating a dedicated method that update the image:

public function updateObject()
{
  $this->updateImageColumn();
 
  return parent::updateObject();
}
 
protected function updateImageColumn()
{
  if ($image = $this->getValue('image'))
  {
    // remove old image
    if (file_exists($file = sfConfig::get('sf_web_dir').'/images/products/'.$this->object->getImage()))
    {
      unlink($file);
    }
 
    // save new image
    $filename = sha1($image->getOriginalName()).$image->getExtension($image->getOriginalExtension());
    $image->save(sfConfig::get('sf_web_dir').'/images/products/'.$filename);
  }
 
  unset($this->values['image']);
}
 

As of symfony 1.2, you can even remove the form code (updateObject and updateImageColumn) altogether as Propel forms now does all this boiler-plate code automatically for you.

To sum up the work we have done during the first three parts: we have made our Controller thiner by moving code to the Model (ProductPeer), to the user (myUser), and to a form (ProductForm).

In the next part, we will see that some code must still be moved away from the Controller. We will also learn some nice shortcuts made available as of symfony 1.1.

Comments

I'm quite curious about that $this->forward404() invocation. How does it prevent the rest of the method from running if there is no “return” statement? Does it throw an exception?
yes it does throw an exception: sfError404Exception
Thanks Fabien!

2 questions:

1) Why not use $this->form->bindAndSave($this->getRequestParameter('product'), $this->getRequest()->getFiles('product'));

2) (Not specific to part 4) Why not use the new $request param like "public function executeEdit($request)" ?

There are often multiple possibilities of how to do it, and I'd like to hear which ones you prefer and why.
@klemens_u: Taht's why there is part 5 ;)
Thanks for shedding light on the new forms support as well as the refactoring issue.

Something that routinely comes up for us when handling file uploads just like this one: if the other fields don't pass validation, but the file upload does, it's generally a bad idea to to make the user select and upload the file over and over again. It's very slow, and the browser requires the user to re-select the file each time the form appears for security reasons.

We have dealt with that by designing our propel objects with a 'live' flag that doesn't get set until they pass validation completely. This way we have a place to store partially valid stuff (i.e., large files the use shouldn't have to re-upload just because they had a weird character in the phone number field).

Does the new forms system provide a standardized solution to this problem?

Thanks!
Where the new filename in the last to snippets is stored? Did i missed something?

Thanks!
Same question as Dejan Spasic asked: when new $filename is passed to the "image" column?
@Dejan Spasic & @SZoPer

I've had the same problem, just add $this->object->setImage($filename);
before the unset at the end of the function.

Daniel
After reading this article, i was asking myself on where to insert a slug/stripped title generation in the new model. Would you more insert it in an object class or in the updateObject of a form class ? I'm quite confused, and i would like my code to be the cleanest possible :)
I think we miss a
$this->object->setImage($filename);

in the update object no ?

Comments are closed.

To ensure that comments stay relevant, they are closed for old posts.