Return validation errors with in the response (blueimp)

[mitom]

The validation errors are ignored in most controllers. This commit will pass the validation error to the response with the blueimp frontend. Works with single-request uploads too, but had to extend the response object for multi-dimensional array support.

Also fixes a small mistake accessing multiple files in the FileBag.

Didn't touch the documentation, but it should mention to use the

$request->addToOffset($value, $offset,...);

instead of simple array access, in case of single-request file uploads.

[sheeep]

Hi @mitom
Just that you know. I'll have a look at this PR this weekend. It looks great, despite the fact that the tests are failing at this point. Do you know how I can contribute to this pull request? Otherwise we should probably merge it to a separate feature-branch.

Another thing: You linked to the documentation of the jQuery File Uploader and I've seen that the return value should follow a strict scheme. IMHO it would make sense to create a specific Response type for the blueimp backend where the mentioned scheme is honored.

Do you have any experience using this uploader? Are there any pitfalls regarding the documentation (for example: the key error is not mentioned there)?

[mitom]

That reference only applies if one wants to use the ui-version, also with the default settings. You can (quite the work actually) create your own interface for it, that's why specifying the array in the controller makes it less flexible. On the other hand if one decided to write his own frontend, overriding that controller would be the tiniest problem.

Regarding the documentations, it is a huge doc, still does not cover the whole project, which would be hard. The error key is not mentioned, that is correct but they do handle it in the script.In fact the create it. But, that script is not mandatory to have to make the uploading work, only to make the theming (unless you write your own interface) work.
Actually, as long as you return a files key that will have arrays of individual files, you can return any information about them and process them with the templating engine (once again if you choose to use it).

Therefore I think it is not a good idea to create it's own Response object, as it is very flexible in that sense.

As with contributing to it, I am not sure if the best practice for the validation error handling is this. This way, the validation listeners can't be flexible enough for different frontends (given that they don't have their very specific error response scheme, in which case it could stay in the controller). I think it would be better to pass the request to the validation, so the listeners can edit it. Or to have some sort of configuration for the error handling, which I think would be the least effective.

In the above case the addToOffset() part is still necessary - to better (still not proper) multi dimensional array handling.

In any scenario I think there should be a way to return the validation errors in the response, for a better user experience.

About the response part, I am not sure if the ease-of-use that the ArrayAccess provides out-weights the lack of functionality it causes (for example lack of multi-dimensional array support is pretty bad). Wouldn't it be better to use something similar to Symfony's ParameterBag?

And finally, it would great if we could figure this out soon, a project of my depends on this change and it's really ugly at the moment :)

[sheeep]

In the above case the addToOffset() part is still necessary - to better (still not proper) multi dimensional array handling.

This is correct. I wasn't aware of the limitations regarding the ArrayAccess interface in the first place. The addToOffset method will be implemented.

In any scenario I think there should be a way to return the validation errors in the response, for a better user experience.

In the end it is a question on how the validation workflow should work. It's probably not well designed in the first place, because using Exceptions has some serious pitfalls. For example: It is currently not possible to return more than one validation error, as the execution flow will be stopped once an exception is thrown. Additionally I think that using exceptions to transport any kind of data is wrong. An exception should never be designated for an enduser, instead it should be a system-internal error management, what it was designed for in the first place. This is why I think this should be written in another way.

$response->addToOffset(array("error" => $e->getMessage()), 'files');

But! You brought up a possible solution. In my opinion, it would be best to include the Response in ValidationEvent. This way you can implement your error management truly frontend agnostic. Having the addToOffset method on the Response object, this would be as easy as pie! :)

On and this change gave me a headache. ;) It's quite bitchy to test, but I think I've got it correct again. mitom@a643e90#L0R18
Do you know if this key is customizable by the frontend?

Now, to sum up:
What do you think of adding the response object to the validation events, and let the user decide how to handle the error. I think this would provide by far the highest level of abstraction, what could be useful if someone decides to create his own UI for the blueimp uploader.

And finally, it would great if we could figure this out soon, a project of my depends on this change and it's really ugly at the moment :)

I know this feeling. Just let me know what you think, and I'll give my best to push it out asap. =)

[mitom]

For example: It is currently not possible to return more than one validation error, as the execution flow will be stopped once an exception is thrown.

It is not true. Well it was true, but if you don't return here as was the case, then with addToOffset every file can return 1 error. To be honest I have no idea what would be the case if multiple errors happened, but it would probably be either the first or the last.

Nevertheless I agree with not using exceptions for validation errors, but I think it is a good solution in this case. I also agree with passing the response to the validators, but the exceptions (or something) is required to let the controller know the validation had actually failed otherwise the response will be correct, but the file-uploading will proceed anyway.

It raises another problem: the validators of this bundle would be sort-of forced to check the config for the frontend and put the error in the correct place in the response. Which could be possible, but the flexibility of (at least blueimp) the frontends in the response would once again be 'compromised'

All in one, I think to correct the validation there should be either configuration on where to insert the validation errors or all the validation must be done by the user, which would make it a bit harder to fully implement the bundle, therefore I wouldn't go with that.

For the configuration I think doing something like

validation:
  error_offset: ['files']

and then using addToOffset and then doing something like

try {
    //....
} catch (UploadException $e) {
        $response->addToOffset($e->getResult(), $this->config['validation']['error_offset']);
} 

with slightly changing addToOffset to take an array instead of using func_get_args(), and adding a variable to the exception to be able to store any data (without calling it 'message') could make it work I think.

Let me know what do you think and please note, I am really bad at naming things, feel free to change them, if that's the only issue.

you know if this key is customizable by the frontend?

Sadly/happily yes, it is. You could return anything to blueimp, I think it also doesn't have to be JSON. It is a real pain to make anything work with it if you want to keep the flexibility. On the other hand, I really can't see anyone choosing to create their own interface for it. The one they have works pretty well after you figure it out, implements twitter bootstrap and can be easily themed. The only major downside is the lots of dependencies, but in the end, I think it is not the common case.

Let me know if you need any help with implementing it, or if you know a better/different solution, I am happy to elaborate on it.

[sheeep]

tl;dr: I had an idea regarding the customizable error handling. I pushed the implementation to a new error-handler branch. Take a look at it here.

Let's see if I can break down the main idea.

ErrorHandler

The fact that the blueimp uploader is highly customizable makes it a pain to not get in the way of a developer deciding to implement his own frontend. So I thought it could be a good idea to use configurable error handler. This is why I added a new configuration field error_handler with a default NoopErrorHandler that simply does nothing when getting called. Furthermore there is a BlueimpErrorHandler which calls the addToOffset having the error-message as an argument.

This way a developer could just define his own service if he don't like the default way of returning error messages to the frontend.

addToOffset

I don't really get, why we use func_get_args() in this method, so I decided to further simplify the method (probably also strips down the number of features).

public function addToOffset($offset, array $value)
{
    if (!array_key_exists($offset, $this->data)) {
        $this->data[$offset] = array();
    }

    $this->data[$offset] += $value;
}

It sure has its limitations (for example if you want to add $value to deeper path in the array).

Exceptions

The ValidationException is now extending the Symfony2 internal UploadException instead of \DomainException. This allows to use a type hint in the ErrorHandler. Additionally there is a new set of methods to set and get a special error message. If no error message is set, it will return the default exception message. This feature is not used yet by the provided validators.

There is still quite a bit work to do.

• Add documentation for these new features, mainly error handlers.
• Add a new tag, for this could be a BC-break (if one has implemented own Controllers)
• Add tests for the addToOffset method
• Add tests for error handlers

But the most important part: This was just a prototype and therefore not tested thoroughly. Feedback is highly appreciated. :)

/cc @mitom

[sheeep]

It sure has its limitations (for example if you want to add $value to deeper path in the array).

I'm sorry, not my brightest moment :) I reimplemented your original function in 2db6ca7

[mitom]

I'm sorry, not my brightest moment :) I reimplemented your original function in 2db6ca7

I was going to ask about that, but you fixed it before I got to my computer :).

One thing which is my mistake: you might be right and using func_get_args() is overcomplicating things. Using an array as 2nd parameter of addToOffset could make it easier to understand, and might give better programmatic control. It's not a big change tho.

    /**
     * The \ArrayAccess interface does not support multi-dimensional array syntax such as $array["foo"][] = bar
     * This function will take a path of arrays and add a new element to it, creating the path if needed.
     *
     * @param mixed $value
     * @param array $offsets 
     *
     * @throws \InvalidArgumentException if the path contains non-array items.
     *
     */
    public function addToOffset($value, array $offsets)
    {
        $element =& $this->data;
        foreach ($offsets as $offset) {
            if (isset($element[$offset])) {
                if (is_array($element[$offset])) {
                    $element =& $element[$offset];
                } else {
                    throw new \InvalidArgumentException("The specified offset is set but is not an array at" . $offset);
                }
            } else {
                $element[$offset] = array();
                $element =& $element[$offset];
            }
        }
        $element = $value;
    }

If you want you can also drop in an php unset($element); in the end, that's why travis was complaining, but I think it just makes it uglier with almost no gain. You can also remove the doc entirely/partly if you want, but I think it is good practice to keep it.

What it could gain to use arrays instead of infinite parameters is that they can also be programmatically built and ordered. May also look a bit better.

Testing wise your solution seems to work just fine. It did return the validation errors properly for my validators (the first error, but I think that is the expected behaviour in error handling). Tested it with multiple files/request and single file/request. In both cases it worked and returned the errors for the files in question and successfully uploaded the ones that passed.

From my point of view this is a correct and flexible solution to handle the validation. I also like the fact that it takes advantage of Symfony's UploadException.

Looking forward for it to be merged and let me know if there is something further you need input on.

[sheeep]

Using an array as 2nd parameter of addToOffset could make it easier to understand, and might give better programmatic control. It's not a big change tho.

Merged.

Looking forward for it to be merged and let me know if there is something further you need input on.

If anything fails, don't hesitate to reopen this issue :) Yey. We made it. 🎉

[mitom]

My turn to live with the not the brightest moment. Could you add the []? I am ashamed that I made that mistake twice now..

[sheeep]

Fixed in c076429. :)