Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow multiple validation callbacks for uploads #4633

Closed
wants to merge 1 commit into from
Closed

allow multiple validation callbacks for uploads #4633

wants to merge 1 commit into from

Conversation

zburke
Copy link

@zburke zburke commented May 16, 2016

The current implementation of _execute discards all but the first validation rule for fields with empty post data. When a field corresponds to a file-upload field, this means it is not possible to, for example, handle the file upload in the first callback and perform validation of the uploaded file in subsequent callbacks.

This PR provides support for cascading callbacks for fields with empty post data.

The current implementation of _execute discards all but the first
validation rule for fields with empty post data. When a field
corresponds to a file-upload field, this means it is not possible to,
for example, handle the file upload in the first callback and perform
validation of the uploaded files in subsequent callbacks.

This PR provides support for cascading callbacks for fields with empty
post data.
@narfbg
Copy link
Contributor

narfbg commented May 16, 2016

I don't understand your description. Can you elaborate more, and possibly with a code example?

@zburke
Copy link
Author

zburke commented May 16, 2016

In short, this brings the handling of callback functions for file-upload fields into line with how callbacks are handled for regular post fields, allowing multiple callbacks to be chained like this:

$rules = array(
    array(
        'field' => 'poster_file', 
        'label' => 'Poster File', 
        'rules' => 'callback_handle_upload|callback_validate_page_count|callback_validate_page_size'
    ),
);

Here is a use case example: When uploading files, if you do the upload in a callback function via the Form Validation library then you can use the form validation methods like validation_errors() to handle any errors:

public function do_upload()
{
        $rules = array(
            array('field' => 'poster_file', 'label' => 'Poster File', 'rules' => 'callback_handle_upload'),
        );
        $this->form_validation->set_rules($rules);
        if ($this->form_validation->run()) {
            ...
        }
}

public function handle_upload($str)
{
    $this->load->library('upload', $config);
    if ($this->upload->do_upload()) {
        ...
        return TRUE;
    }

    $this->form_validation->set_message('handle_upload', $this->upload->display_errors());
    return FALSE;
}

This works fine if all you want to do is upload the file. But if you want to do any post-processing, you would have to do it all in the handle_upload callback because when you chain together multiple callbacks as in the first example above, everything after callback_handle_upload is silently discarded by the if-block starting near line 583.

You can work around this by performing the upload and the validation together in that first callback, but it's much cleaner to use separate callback functions for each distinct validation task. Additionally, since regular post fields support chained callbacks, it is more consistent if file-upload fields support it too.

@narfbg
Copy link
Contributor

narfbg commented May 16, 2016

I get what you're trying to do now.

You're making this so hard though ... this has nothing to do with file uploads, the library doesn't care for them at all. You're using a logical hack to somehow inject your uploads into the form validation process, and have then built your whole argumentation around that.

It's a very simple problem - CI_Form_validation::_execute() doesn't check for more than one callback rule (for non-required, non-submitted/empty fields), regardless of the field type or if it is the first rule or not. And that's a bug.

I'll work on a patch of my own, as I want to also make other adjustments to the relevant code, but please read our styleguide if you want to contribute in the future - your patch currently doesn't comply with it.

@narfbg narfbg added the Bug label May 16, 2016
@narfbg narfbg added this to the 3.0.7 milestone May 16, 2016
@zburke
Copy link
Author

zburke commented May 16, 2016

You're sharp. Sorry about my lousy report and formatting. Let me know if you want me to redo the PR with a better explanation and correct style, just in case you decide not to tweak the surrounding code at this time.

narfbg added a commit that referenced this pull request May 17, 2016
@narfbg
Copy link
Contributor

narfbg commented May 17, 2016

The code in question being a mess is why such bugs exist ... the commit linked about should improve that. :)

@narfbg narfbg closed this May 17, 2016
narfbg added a commit that referenced this pull request May 25, 2016
Really fix #4633
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants