Validation Observers don't run when a model is _not_ new.

[itsa-sh]

Issue description

when an Orm\Model instance is used to forge a \Fieldset instance, upon saving the model, there is an issue with the Orm\Observer_Validation::before_save where by it doesn't run the validation rules added to the fieldset; but only when the Orm\Model instance is not new.

I traced the problem down to the following piece of code:

https://github.com/fuel/orm/blob/1.6/master/classes/observer/validation.php#L224-L231

Erronous example

The following example will execute and validate as expected. When $model->save() is called, the validation rule will execute accordingly (because $allow_partial is false).

$model = Model_Example::forge();

$fieldset = \Fieldset::forge(get_class($model), [])->add_model($model)->populate($model);

$fieldset->add('image')->set_type('file')->set_label('Image')->add_rule('uploaded_image');
$fieldset->add('submit', '', ['type' => 'submit', 'class' => 'btn btn-primary', 'value' => 'Save']);

if (\Input::method() == 'POST') {
    /* .. implement the model property definitions here .. */
    $model->save(null, true);
}

However, when the following example executes, the validation behaves incorrectly. When $model->save() is called, the validation rule will not execute accordingly (because $allow_partial is an array of the properties that have changed).

$model = Model_Example::find($id);

$fieldset = \Fieldset::forge(get_class($model), [])->add_model($model)->populate($model);

$fieldset->add('image')->set_type('file')->set_label('Image')->add_rule('uploaded_image');
$fieldset->add('submit', '', ['type' => 'submit', 'class' => 'btn btn-primary', 'value' => 'Save']);

if (\Input::method() == 'POST') {
    /* .. implement the model property definitions here .. */
    $model->save(null, true);
}

Resources

1. uploaded_image is a validation rule that exists on Model_Example.
2. An observer also exists on Model_Example to Orm\Observer_Validation

'\\Orm\\Observer_Validation' => ['events' => ['before_save']],

Identical results occured with the following observer configuration:

'\\Orm\\Observer_Validation' => ['events' => ['before_insert', 'before_update']],

Proposed solution

My proposal was to implement similar functionality from another angle. Instead of iterating over the implemented models' properties, the iteration would occur over the fieldset fields.

$properties = array_keys($obj->properties());

foreach (array_keys($fieldset->field()) as $p)
{
    if (in_array($p, $properties))
    {
        if ( ! in_array($p, $obj->primary_key()) and $obj->is_changed($p))
        {
            $input[$p] = $obj->{$p};
            is_array($allow_partial) and $allow_partial[] = $p;
        }
    }
    else
    {
        $input[$p] = $fieldset->field($p)->value;
        is_array($allow_partial) and $allow_partial[] = $p;
    }
}

[WanWizard]

$allow_partial is there because on a new object, you want to validate ALL properties of the model, and on updates, only the one's that have actually changed. You absolutely want this for performance reasons.

A fieldset you construct in the controller (for the purpose of generating a form) as your example shows has nothing to do with Observer_Validation. That uses the set_fields() method which fetches the validation rules from the models property definition.

In this particular case, running the validation on this fieldset would also not work, since that would validate posted data, and since you're using an input field of type "file", that will not be present in $_POST, and can therefore not be validated.

edit:

If (like I think you are trying to do here) want to re-use the fieldset instance in the Observer, then if your model contains a property called "image", and if the value of that property was changed on the object, the rule should kick in without problems. I don't see anything in set_fields() that would destroy the rules on already defined fieldset fields.

[itsa-sh]

I understand the concept of $allow_partial, personally I don't think it would be a noticeable difference in performance. My proposal was to continue using $allow_partial, also implement an alternative way of validating fields that exist in the fieldset whilst not existing in the model.

Nevertheless my problem in the example above is; when Model_Example does not contain a property called 'image'. We do not have a field in the DB table therefore we don't want a property in the model, so I add a field to the fieldset anyway to upload a file.

Upon saving a new object, the validation executes as expected; Upon saving an existing object, the validation does not execute as expected. This is due to Observer_Validation only validating the model properties and not the fieldset fields.

[WanWizard]

I'm pretty sure the intention here was to validate data going into the database, not validating a form (which should be done in the controller). In that sense, the fact it happens when new is more the bug then the other way around...

Extending this to the entire fieldset will not only shift form validation to an ORM model observer, it will also create additional properties on the model (as after succesful validation all fields in $input will be assigned to the model object).

I'm more inclined at the moment to fix the anomaly in the is_new() situation, the fact that it runs the validation on non-model fields in the fieldset is due to code in Validation, which looks of a posted variable if the input array doesn't contain the field to be validated. Which in itself is dubious behavior at best.