fuel / core

Fuel PHP Framework - The core of the Fuel v1 framework
http://fuelphp.com
813 stars 345 forks source link

Ability to have unique error messages for form fields that use the same validation function #247

Closed Mitchell64 closed 13 years ago

Mitchell64 commented 13 years ago

This issue references a forum discussion that might be useful to read. http://fuelphp.com/forums/topics/view/2603/

WanWizard suggested I add this as an issue for Jelmer since it deals with the Validation class.

The summary is that I extended the Validation class and added a new validation function: _validation_check_upload() which was to check multiple form fields where files were uploaded. Since it's possible for each file upload to have a different upload error, I wanted to display the appropriate error for each file upload. The problem is that there is only one error message allowed per validation function.

validation->error_messages[] contains those error messages. So each time the validation function fails, the error message is overwritten and the same error message is displayed for each form field that fails that validation. Normally, this is not a problem, but in the case of multiple file uploads where each file can fail for a different reason and you would also like to display the name of the file that failed, this is a problem.

If a form field fails validation, an array of class Validation_Error is created and added to the Validation instance. A Validation_Error is created for each field that fails. The class Validation_Error contains a variable $message. So there is a place for a unique error message for each form field, however I couldn't find anywhere that references the $message variable and it's always empty.

As a quick workaround, I overwrote Validation_Error __construct() and Validation_Error get_message(). In the constructor, I save the current error message in Validation_Error->message (actually the code is $this->message). In Validation_Error get_message(), if there's a message saved, it's returned instead of the normal Validation error message.

I also thought of another approach but it has a problem in the current implementation. The concept is to allow the user to update the rules params for the form field, i.e. add a function set_rule_param() to to the Fieldset_Field class . If that could be done, then the error message could be constructed so that the specific error message and the file that failed upload could be specified in the params which are unique for each form field. The current problem is that if the params are updated in the validation function, it's too late because the params have already been saved as a local variable in Validation class run() function. So if _run_rule() fails and a Validation_Error is created, only the original params are saved--not the params added by the validation function.

nerdsrescueme commented 13 years ago

Is this a duplicate of Issue #241?

Mitchell64 commented 13 years ago

Not exactly. When I first described the problem, WanWizard opened issue #241, however I don't think I described it clearly, so ticket #241 is a more elaborate feature that would allow more than one error to be reported for each form field. For example, one form field could have multiple errors: file exceeded max file size AND file type not allowed.

This issue is to allow a different error message to be displayed for each form field for the same validation function. For example, form field #1 could have the upload error: file exceeded max file size and form field #2 could have the upload error: file type not allowed.

jschreuder commented 13 years ago

The new feature and fix for issue #159 allows you to give each full callback rule a name to use as validation rule. This won't work with the included validation rules like required though unless you use it like this: add_rule(array('named_required' => array('Validation', '_validation_required')))

Also you can replace the message before output by using $validation_error->get_message('Some message with :label and maybe other stuff.');

Mitchell64 commented 13 years ago

Jelmer, I'm not sure I understand how to use the new feature. My code is as below where the validation rule upload_ok is specified for each file field of the form. This generates the same error message (the last one encountered) for each file form field. I'm probably implementing your fix incorrectly.

/**************************************************************************
* This function checks to see that the file has been uploaded successfully.
*
* Example usage: array('upload_ok', $form_field),
*
* $form_value: will be null
***************************************************************************/
public function _validation_upload_ok($form_value, $form_field)
{
    // Get files uploaded with errors
    $upload_errors = Upload::get_errors();

    // Check if this form field had an upload error
    foreach ($upload_errors as $error)
    {
        if ($error['field'] == $form_field)
        {
            $this->set_message('upload_ok', $error['message'] . ' (' . $error['name'] . ')');
            return false;
        }
    }

    return true;
}
jschreuder commented 13 years ago

The problem for you probably is that the fix currently only works with a full callback and not with the short syntax. This means that you'd have to add this function as a full callback instead of a callable.

As this isn't a static method it always has to be called on an object instance, in that case you could add it like this to name it:

add_rule(array('named_upload_ok' => array($instance, '_validation_upload_ok')))
Mitchell64 commented 13 years ago

I apologize if reopening this issue is the wrong way to handle this, but I wasn't able to implement the new fix.

Currently, my model has:

$form->add('file_1',
    "Upload File 1",
    array(
        'type' => 'file',
        'size' => 50,
    ),
    array(
        array('upload_ok', 'file_1'),
    )
);

$form->add('file_2',
    "Upload File 2",
    array(
        'type' => 'file',
        'size' => 50,
    ),
    array(
        array('upload_ok', 'file_2'),
    )
);

And my extended validation class has:

/**************************************************************************
* This function checks to see that the file has been uploaded successfully.
*
* Example usage: array('upload_ok', $form_field),
*
* $form_value: will be null
***************************************************************************/
public function _validation_upload_ok($form_value, $form_field)
{
    // Get files uploaded with errors
    $upload_errors = Upload::get_errors();

    // Check if this form field had an upload error
    foreach ($upload_errors as $error)
    {
        if ($error['field'] == $form_field)
        {
            $this->set_message('upload_ok', $error['message'] . ' (' . $error['name'] . ')');
            return false;
        }
    }

    return true;
}

How would I change the code to allow different error messages for file_1 and file_2?

Thanks.