Symfony forms – Flexible widgets based on user credentials (sfcontext is evil)

Background

This issue has come up many times in the symfony forum and on IRC, and whilst it seems like a fairly trivial one, it is important to discuss the best practice around it. Often we want to modify a form based on something outside the form’s scope, like a user’s credentials or the page they are on, or maybe some session values – lets take the example of a dropdown widget where admin users get to see a few more options.

The bad way

Everyone that has been using symfony for a while is aware of the context singleton. This magic sprite allows us to grab all sorts of information about the context we are in, including the user, session, request, view, and many more things. Whilst it certainly has its place, most of the time, however, we should avoid using it. The clue to the reason why is carefully disguised in the class name “Context”.

When we refer to a context in our code, we are locking ourselves in to the fact that the context must exist, so every time we use it we are basically saying that this class can now only be used in a symfony project with a fully initialised symfony stack hanging over it. This becomes a problem in things like unit tests, where you have to mock up a loaded context object with bells and whistles in order to test a simple function or class.

So this is what you might think of doing:

// In your form class
public function configure()
{
  $choices = array(1 => "something boring", 2 => "something dull");
 
  $currentUser = sfContext::getInstance()->getUser();
 
  if ($currentUser->isAuthenticated() && $currentUser->hasCredential("admin"))
  {
    $choices += array(5 => "something cool", 6 => "something leet");
  }
 
  // ....
}

The reason this is bad is that the forms framework is a standalone framework – it should be possible to pick up your form class and drop it into any project. It should also be possible to test it independently of symfony, without being tied in by sfContext. So what is the better way?

The dependency injection approach

You might read that and think “woah, this is getting complicated!” but we’re not talking about dependency injection containers here, we’re simply saying that you can make your form object depend on something to run. The thing it depends on should not be the context singleton, it should be the minimum thing that the form needs to operate correctly – which in this case, is a user object that supports credentials.

// In your actions.class.php
$this->form = new myForm(array(), array("currentUser" => $this->getUser()));
 
// In your form class
 
// ...
if ( !($this->getOption("currentUser") instanceof sfUser))
{
  throw new InvalidArgumentException("You must pass a user object as an option to this form!");
}

The test here is where we are making this form “dependent” on a user object. In this case we are insisting that the object is an instance of sfUser, which you may argue is tying us in to symfony again, but you could use any test here to ensure that the object will have the necessary functionality you need, maybe check for the existence of a “hasCredential()” method for example.

When writing a test for this form class, we now only need to instantiate a user object and load it with some credentials – much easier than doing the same thing and locking into a context singleton. There may be other times when this form could be useful in a lightweight environment, where you can get speedy access to a user object but don’t want the overhead of the symfony context – you might not think of one now, but it’s best to code this way and you’ll have less reasons to kick yourself further down the line.

The completed code, for our single widget form

// In your form class
public function configure()
{
  if (!($this->getOption("currentUser") instanceof sfUser))
  {
    throw new InvalidArgumentException("You must pass a user object as an option to this form!");
  }
  else
  {
    $currentUser = $this->getOption("currentUser");
  }
  $choices = array(1 => "something boring", 2 => "something dull");
 
  if ($currentUser->isAuthenticated() && $currentUser->hasCredential("admin"))
  {
    $choices += array(5 => "something cool", 6 => "something leet");
  }
 
  $this->widgetSchema['my_dropdown']    = new sfWidgetFormChoice(array("choices" => $choices));
  $this->validatorSchema['my_dropdown'] = new sfValidatorChoice(array("choices" => array_keys($choices)));
}

Wrap up

Think about this example the next time you think about modifying a query based on a user object, or session value in a peer class, or Doctrine table class… Maybe you should have passed a parameter there too? Every class and method you write, think about how you can reuse it, will it even be possible the way you have written it? If something simply must be coded in a symfony specific way, think about making a parent and a child class for the problem you are trying to solve. In the parent class, you can make things as generic as possible – so you can re-use that class to your heart’s content. In the child class, you can add the symfony specific code – kept to a minimum.

Customising Symfony forms – be careful with base class inheritence

Background

Recently I was working on a form for updating a couple of very simple values for a single table. When creating such forms, where we only need a subset of the available columns to be editable, we always have the option of either unsetting the fields we don’t need, or overriding the widgetSchema. In this case, I opted for the latter, since I only needed to edit 2 columns out of a possible 10, I didn’t think adding 8 fields to the unset() function was the cleanest way. The following examples contain obfuscated data.

Overriding the widget schema

public function configure()
{
  $this->setWidgets(array(
    'amount'               => new sfWidgetFormInput(),
    'reduced_amount' => new sfWidgetFormInput(),
    ));
 
   //Labels and decorator stuff here
}

My plan was to inherit the validators that already exist in the base class, since they do the job for what I need.

The error

The form worked fine for an insert, but when I came to update an existing record, the error was quite strange:

SQLSTATE[23505]: Unique violation: 7 ERROR: 
  duplicate key value violates unique constraint 
  "body_fee_version_pkey"

The problem it seemed was that my versionable behaviour was not incrementing the version value before attempting to insert a new version record. After a long period of debugging the versionable behaviour, along with some of my other custom behaviours, I was no closer to a solution.

I started to dig into the form classes, working backwords through all the object update methods, save, dosave, etc. Until I finally stumbled across this line:

$this->values = $this->validatorSchema->clean(
   self::deepArrayUnion($this->taintedValues, 
   self::convertFileInformation($this->taintedFiles))
 );

Before this call, everything seemed ok, but after this call, my values array, which at this stage only contained the two fields that had been posted, now suddenly included a value for all the fields in the table. Why? It then occurred to me that the entire validator schema was being processed, not just the fields that are actually posted! This means that all the validators that are required=false will silently return a “clean” value, which is most likely the database default.

So what did this mean? Well, it meant that the validator was “cleaning” all the columns that had not been submitted with the form, including the version column, which was being set to null. When the versionable behaviour kicked in, it read this null value and incremented by 1 for the next version, which then became 1 – a version which of course already existed, causing the error.

The solution

The solution is blindingly simple, we don’t just declare the widget schema, we must also declare the validator schema. Whilst this seems like it makes sense, I feel that it is a shame that I have to essentially copy and paste the necessary validators from the base class. The alternative of course would have been to unset the offending fields, but then we are back to option 1 above, unsetting 8 of 10 fields when it seems cleaner just to declare the 2 fields I actially need.

  $this->setWidgets(array(
    'amount'               => new sfWidgetFormInput(),
    'reduced_amount' => new sfWidgetFormInput(),
    ));
 
// Messages declared here as array since they are the same
 
$this->setValidators(array(
  "amount"  => new sfNumberValidator(
                       array('required' => true), $messages),
  "reduced_amount" => new sfNumberValidator(
                       array(), $messages),
 ));
 
   //Labels and decorator stuff here

Conclusion

I have been working with Symfony for over a year and a half, and with Symfony forms since they were born – and yet I was still caught out by something that seems quite simple, because I assumed it would be ok. There are so many things that could have alerted me to this problem and saved me a lot of time, for example if any of the extra fields had been required=true then at least I would have had some form errors to give me a clue!

I may suggest that the default behaviour should be to only process the validators that match the widgets, or maybe at least for the form to err if extra validators are found that are not used, as I feel that this is a mistake that others could make, and as I mentioned above, it’s a shame that we have to re-declare validators that are already present in the base class.