fValidation flexibility.

[Dazzel]

Ticket #750 by @netcarver

Currently the member variables and the check* methods are all declared private, making it impossible to extend the class and override any of those methods or access the member variables. Making these protected would allow extension if needed.

Comment by @audvare

I'd really like to know what you intend to do in your extension. I think the check* methods are private for good reason and I have yet to have a need to mess with fValidation's internals or extend on it.

fValidation has fValidation::addCallbackRule() which essentially allows custom validation on a per-object basis. And as such, you can simply create constants that define callbacks (so they look nicer in code).

class User extends fActiveRecord {
  const nameIsAvailable = 'User::nameIsAvailable';

  public static function nameIsAvailable($name) {
    try {
      new self(array('name' => $name));
      return FALSE; // is taken
    }
    catch (fNotFoundException $e) {}
    return TRUE;
  }
}

// later code
try {
  $validation = new fValidation;
  $validation->addRequiredFields('name');
  $validation->addCallbackRule('name', User::nameIsAvailable, 'Sorry, that user name is taken.');
  $validation->validate();
  fURL::redirect('/');
}
catch (fValidationException $e) {
  fMessaging::create('validation', '/registration' $e->getMessage());
  fURL::redirect('/registration');
}

I have a longer example here which uses fValidation: https://github.com/tatsh/sutra/wiki/Tutorial

Comment by @netcarver

Hello Andrew,

Thank you for reply and the great examples, especially in your link which I found particularly interesting.

My request is motivated by wanting to have callback rules be able to return an error message directly, rather than having to define it when the rule is added to the validation object. This would allow callbacks to provide messages that are appropriate to, say, some value of an input or other combinations of factors.

An additional motivation is the supply of custom strings to required elements such that the message displayed for, say, a missing age can be different to that displayed for a missing check box confirming acceptance of conditions. Whilst this can probably be done using a compose callback on fText, it might be easier to have a version of addRequiredFields() that accepts an array with key=>value pairs that match form element names to messages.

Comment by @audvare

The fValidaion::validate() method can also be controlled to not throw and instead return messages by passing TRUE as the first parameter. Passing TRUE as the second parameter removes filed names from the messages.

fValidationException goes through fText::compose(). You can use fText::registerCoposeCallback() in order to change messages. I know this may not be ideal for the situation you describe here.

You may instead need to skip fValidation for that purpose, still use fValidation but after you run fValidation::validate() perform more validation. If you do it in the try/catch block like in my example, then you can simply throw a new fValidationException when an error occurs (or of course in the method/funcion you call from there).

try {
  $validation = new fValidation;
  // Add conditions
  $validation->validate();

  // Method that throws with a specific message
  SomeClass::validateAdditionalConditions();
}
catch (fValidationException $e) {
  // Do something based on the error
}

You would not have to use fText::compose() as fValidation has fValidation::addStringReplacement() which makes str_replace() get called before each message is about to be composed.

I like the idea of specifying messages for required fields more easily. I suggest 2 solutions.

It would be interesting to have a method like fValidation::setRequiredFieldMessages(), basically an easier way than calling fValidation::addStringReplacement() many times and far easier to use since you don't have to know what to replace.

$validation = new fValidation;
$validation->setRequiredFields('accepted'); // checkbox
$validation->setRequiredFieldMessages(array(
  'accepted' => 'You must accept the terms and conditions to register.',
));
$validation->validate();

Any messages not overridden will be left alone.

Or, since you can use an array with fValidation::addRequiredFields() then a few new signatures (honestly the former is probably a lot easier to maintain):

class fValidation() {
  public function addRequiredFields(array $fields, array $messages) {}
  public function addRequiredFields(array $fields_to_messages) {} // as you described
}

If you make a change directly to fValidation and it works, post a patch.

Do not know if every comment should be mentioned. Please take a look at it: http://flourishlib.com/ticket/750

[Tatsh]

From my last comment on the original issue:

A method like fValidation::addCSRFField() would be awesome.

https://github.com/flourishlib/flourish-classes/pull/49

[wbond]

Comment by @netcarver:

Hello Andrew,

Thank you for taking the time to post in such detail. As you pointed out in your reply to my first idea, it is possible to use the fValidation::validate() method without having it throw an exception. In fact, externally defining additional conditions is what I have resorted to doing in the absence of a more flexible way of having the error messages defined via fValidation::addCallbackRule() itself.

In regard to your second reply, I think you are probably correct in saying that another method would be better than another signature to fValidation::setRequiredFields().

Originally I had envisaged a patch to fValidation that opened the internal rule storage and check* methods so that I could subclass fValidation and provide my own solutions to these two issues without touching fValidation any further. However, having read your replies, I now see that it would be equally acceptable (to me at least) to simply have two new methods that added these features right into fValidation without loosening the access controls on the members and methods.

If I do work up a patch for this I intend to submit it to Will.

[wbond]

Ticket #54 addresses the issue that callback rules should be able to set a message. There is also the discussion at http://flourishlib.com/discussion/2/572 which has got me thinking about ways to handle customizing messages.

Let's talk a little bit more about this before writing any code. I feel like we need to get down to a more core level decision related to custom validation messages for other more complex validation. This should also affect the whole ORM validation system since fValidation and the ORM are based on the same principles, albeit not currently sharing any code (for historical reasons).

[wbond]

Comment by @tatsh:

Might I also suggest not outputting HTML if fJSON::sendHeader() or fXML::sendHeader() has been called? Currently my solution to that is to add a callback for fText that just strips HTML 'post' fText::compose() if the request is made via AJAX (fRequest::isAjax()).

[wbond]

Comment by @tatsh:

Another thought came to mind on 'extending'. If all you want are preconfigured fValidation objects, I would make a class aptly named somethingValidation like (bValidation if your site classes were regarding a blog). It would just have static methods that return pre-configured fValidation objects.

class bValidation extends fValidation {
  const checkCSRF = 'bValidation::checkCSRF';

  /**
   * @param string $csrf
   * @param string $url
   * @return boolean
   */
  public static function checkCSRF($csrf, $url = NULL) {
    try {
      fRequest::validateCSRFToken($csrf, $url);
    }
    catch (fValidationException $e) {}
    return FALSE;
  }

  /**
   * @param string $csrf_field_name
   * @param string $printed_name
   * @return fValidation
   */
  public static function initWithCSRF($csrf_field_name, $printed_name = 'Csrf') {
    $validation = new self;
    $validation->addRequiredField($csrf_field_name);
    $vaildation->addCallbackRule($csrf_field_name, bValidation::checkCSRF, __('Unknown error occurred. Please try again.'));
    $validation->overrideFieldName($csrf_field_name, $printed_name);
    return $validation;
  }
}

Most if not all of my forms have a CSRF token to validate. Typing to add the callback rule over and over is a bit annoying.

A method like fValidation::addCSRFField() would be awesome.