Skip to content

[Form] ViolationMapper - Make form submission requirement optional #12528

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

Closed
wants to merge 1 commit into from

Conversation

bwoodmansee
Copy link

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

It would be useful to have the ability to decide if the form will accept errors. We have found that in certain use cases this functionality would be needed.
For example, we have a use case where a user is uploading a large amount of product data via spreadsheet. We present validation errors for individual products using the form component. In this case, since the data is "submitted" via the file upload, it is no longer possible to use form to present errors as the form has not been submitted in the conventional sense.

Related:
#10567

@bwoodmansee bwoodmansee force-pushed the protected_accept_errors branch 3 times, most recently from 09ddd6c to 0f9fd9b Compare November 20, 2014 21:44
@bwoodmansee bwoodmansee force-pushed the protected_accept_errors branch from 0f9fd9b to d17338b Compare November 20, 2014 21:57
@bwoodmansee bwoodmansee changed the title [Form] Allow override of ValidationMapper::acceptsErrors [Form] ViolationMapper - Make form submission requirement optional Nov 20, 2014
@fabpot fabpot added the Form label Dec 7, 2014
@merk
Copy link
Contributor

merk commented Feb 20, 2015

👍 - I have just encountered this issue where I wish to display validation failures from the bound object when the form is initially displayed.

@bwoodmansee Test changes/additions are probably required for this PR to be accepted

@DanielBadura
Copy link
Contributor

👍 - we need this too

@odolbeau
Copy link
Contributor

👍 Same here.

// requests.
// https://github.com/symfony/symfony/pull/10567
return $form->isSubmitted() && ($this->allowNonSynchronized || $form->isSynchronized());
if ($this->isFormSubmissionRequired()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is totally useless, you should just check:

if ($this->formSubmissionRequired) {}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

@webmozart
Copy link
Contributor

Hi, thank you for taking the time to submit this PR! How would you validate the form in this case? I.e., before its submission?

@merk
Copy link
Contributor

merk commented Jun 18, 2015

Right now I've got code that imitates the ViolationMapper - but it'd be nice to be able to extend or reuse the ValidationListener in a form type that has a special requirement for validation besides POST_SUBMIT.

It isnt possible right now because the ViolationMapper checks specifically for submitted forms.

https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/Form/Extension/Validator/EventListener/ValidationListener.php#L72

@webmozart
Copy link
Contributor

So - if I understand correctly - you want to create a form, validate it, display it and then handle the submission (+ validate again, obviously), right?

@merk
Copy link
Contributor

merk commented Jun 19, 2015

Yep, I'd like the initial display of the form to contain validation errors.

@bwoodmansee
Copy link
Author

Here's how we're validating in this case:

/**
 * @return array
 */
public static function getSubscribedEvents()
{
    return array(FormEvents::POST_SET_DATA => 'postSetData');
}

/**
 * @param FormEvent $event
 */
public function postSetData(FormEvent $event)
{
    $mapper = new ViolationMapper();
    $mapper->setFormSubmissionRequired(false);
    $validationListener = new ValidationListener($this->validator, $mapper);
    $validationListener->validateForm($event);
}

Then dealing with the form errors like normal

@rejinka
Copy link

rejinka commented Jan 21, 2016

+1 for this. Simply said: We have the exact same use-case.

@fabpot
Copy link
Member

fabpot commented Mar 4, 2016

@webmozart What's your final call on this? Should we add this?

@webmozart
Copy link
Contributor

@fabpot I think it's useful. I've seen a few projects where this was a problem.

However, we can't add an argument to mapViolation(), since that would break BC as of symfony/symfony-docs#5735.

@fabpot
Copy link
Member

fabpot commented Mar 7, 2016

But the current PR does not add any argument, so looks like it is safe regarding BC breaks.

@webmozart
Copy link
Contributor

@fabpot That's true, however as of now the PR is not very useful. This functionality is only useful to have in core if it can be controlled through an option when creating a form. Setting this as a global flag (the violation mapper is registered in the DIC) would create problems with bundles that expect the flag to be set to a specific value.

Given that the violation mapper is a global service, we have to pass the option into mapViolation(). Alternatively, we can add a setter to ViolationMapper, but we can't add that setter to the interface. Also, that setter makes the service stateful and I'd rather avoid that to prevent side effects.

@edhgoose
Copy link

I'd also be interested in this feature being added. We're in the process of upgrading from 2.3 to 2.8 and this has caused us problems.

Our use case, if it helps, is that a (GET) request is submitted to us by a client which contains the parameters to populate a form. In a lot of cases, the form is fully populated and the submission works fine. We make a decision there to ignore the form and to go straight to the next part of our journey. This provides a positive user experience in our case as it's slightly faster.

In other cases however, users have errors in their data. We'd like to therefore show them how they can resolve the errors. Sometimes we have no data, in which case we can just show them an empty form and showing errors is redundant.

However, if they provide, say, 2/3rds of the data, we don't want them to have to manually figure out which ones are submitted and which aren't. We'd like to instead show the errors that they need to resolve.

Because of the isSubmitted check it will never show the errors. Removing the isSubmitted check returns the functionality to how it was before.

@xabbuh
Copy link
Member

xabbuh commented Mar 24, 2016

@edhgoose looks like #18053 could solve your issue

@HeahDude
Copy link
Contributor

I don't understand why not just submit null to trigger a first validation before creating the view ?

In case the form has a csrf protection there is still this workaround:

$form = ... // create the form

$form->handleRequest($request);

$view = $form->createView(); // generates the token

if ($form->isSubmitted() && $form->isValid()) {
    // Process it
} else {
    // Force a first validation (you can set empty_data option to have full control here)
    $form->submit(array('_token' => $view->children['_token']->vars['value']));
}

$view = $form->createView(); // has initial errors if any

@fabpot
Copy link
Member

fabpot commented Jun 9, 2016

Closing as @webmozart is right when saying that having a global config is a no-go for such a feature and because #18053 looks like a good alternative.

@fabpot fabpot closed this Jun 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.