FIX Handle validation errors in inline UploadFields

[robbieaverill]

• Implemented redirect method to natively handle validation errors being sent back to the UploadField
• Resolves Validation failure on Page's has_one Image throws Exception #6086

I'm happy to discuss this approach - not sure if it's the "best" way to do it - the iframe and duplicated assets scared me off a little ;)

[dhensby]

Interesting, I take it there's no way to make this work without adding the redirect method?

We shouldn't really be putting new APIs in to the 3.4 branch...

[robbieaverill]

@dhensby you guys would know better than me! This is the code that handled the form validation error and does something. I'm guessing that the UploadField might be a bit of a unique case in that it's run inside a frame. Looks like it's being treated as a controller, but doesn't share the same interface methods as a controller.

Agree re: adding methods to that branch - shall I change to 3 and rebase?

[robbieaverill]

Hey @dhensby - I've changed to the 3 branch instead. Nice to see that GitHub didn't make me rebase :)

[tractorcow]

@robbieaverill can I ask why is the method redirect() vs redirectBack()? I thought that this was the issue causing #6086. Even with the changed redirect option, it could fall back to redirectBack() in some cases.

If you are implement the redirect() method then it would pay to follow the same method signature as Controller::redirect, which redirects to the given parameter.

On a side note, this seems like another side effect of the framework having poorly defined roles for Controller vs RequestHandler. Form expects a Controller instance as parent, but there are MANY places we pass a RequestHandler instead, e.g. GridField.

[robbieaverill]

Hey @tractorcow, I used redirect because is context seemed more relevant to the fact that the form is an embedded frame. The redirectBack behaviour would be the same if you didn't add the ->setRedirectToFormOnValidationError(true) flag.

I haven't done any edge case testing on this, just thought I would propose it since it does fix the immediate problem (like a band aid I suppose).

I know what you mean re the method signature, however it is not actually a redirect, so there is a semantic issue there which is more likely to be a problem in the way that class is architected or implemented. I don't want to go messing around with that of course. I didn't follow the method
signature because (A) it's not a controller, it's just pretending to be one to fit into the Form logic and (B) the destination argument has no use at all in its context. Since it doesn't inherit any kind of interface for the method I thought it could be treated more as a patch than anything. Figured it might be misleading to other developers if anything.

TL;DR: it's a quick fix for a bigger problem, one which I don't have time to look very far into I'm afraid.

I'm happy for you guys to do whatever you see fit with this PR, but the issue does exist even with a standard FormField with a validation rule, e.g. Max length as provided out of the box.

Cheers :-)

[tractorcow]

I had another think about it... and I'm wondering if we don't move some logic up from Controller into RequestHandler: redirect(), redirectBack(), and Link(). That way we can explicitly allow Form::setController() to take a request handler (which we do in many cases), rather than require a controller.

[robbieaverill]

Yes please @tractorcow

[robbieaverill]

Closing - not sure this is the best way to fix the problem for now.

[tractorcow]

I've proposed this in #6362