8000 [3.0] [Form] Inconsistency between isValid() and the "valid" template variable · Issue #7737 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[3.0] [Form] Inconsistency between isValid() and the "valid" template variable #7737

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.

Sign up for GitHub

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

Jump to bottom
Closed
webmozart opened this issue Apr 20, 2013 · 21 comments
Closed

[3.0] [Form] Inconsistency between isValid() and the "valid" template variable #7737

webmozart opened this issue Apr 20, 2013 · 21 comments
Labels
Enhancement Form

Comments

@webmozart
Copy link
Contributor
webmozart commented Apr 20, 2013 •
edited by javiereguiluz
Loading

I'm calling out for you to decide how to resolve the inconsistency between the Form::isValid() method and the valid variable available in the template:

  • isValid() returns false if a form was not submitted. This way it is possible to write concise controller code:
$form = $this->createForm(...);
$form->handleRequest($request);

if ($form->isValid()) {
    // only executed if the form is submitted AND valid
}
  • valid contains true if a form was not submitted. This way it is possible to rely on this variable for error styling of a form.
<div{% if not form.vars.valid %} class="error"{% endif %}>

We have two alternatives for resolving this problem:

  1. Leave the inconsistency as is.
  2. Make isValid() return true if a form was not submitted (consistent with valid)
  3. Revert to the 2.2 behavior of throwing an exception if isValid() is called on a non-submitted form (not consistent with valid).

Both 2. and 3. will require additional code in the controller:

$form = $this->createForm(...);
$form->handleRequest($request);

if ($form->isSubmitted() && $form->isValid()) {
    // only executed if the form is submitted AND valid
}

What do you think?
@drola
Copy link
drola commented Apr 20, 2013

What about option 4: isValid() in controller and valid in template return false for unsubmitted form, but we add hasErrors() that returns true only for submitted form that contains errors and false in all other cases. In this variant names of the attributes/methods would be pretty much self explanatory.

@webmozart
Copy link
Contributor Author

@drola Changing valid breaks BC. Also, hasErrors() and errors|length can't be used, because they return true only if a form itself has errors, but not for its children. It is possible to add a parameter $deep to hasErrors(), but not so for the errors template variable. So this is unfortunately not an option.

@egulias
Copy link
Contributor
egulias commented Apr 20, 2013

I would go for n°3 as an unsubmitted form is neither valid or invalid. Despite the additional code.

@drola
Copy link
drola commented Apr 20, 2013

In that case I vote for option 3 too. I believe it's the best one, because the developer learns about it in a pretty explicit way.

Option 2 would be bad because it changes behaviour without throwing any errors - a pretty hard to find bug in an application. Especially because one does not expect to get true in response to isValid() on an unsubmitted form. Form like that simply isn't valid (but is it invalid?)

@webmozart
Copy link
Contributor Author

You could consider a "valid form" to be simply one without errors. Option 2 satisfies this. And the valid variable makes the same assumption.

@drola
Copy link
drola commented Apr 20, 2013

Yeah, it really depends whether you define "valid" as errorless or one containing data ready for further processing... Which was the initial source of inconsistency.

@wouterj
Copy link
Member
wouterj commented Apr 20, 2013

👍 3

@mvrhov
Copy link
mvrhov commented Apr 20, 2013

function isSubmitted(bool $validCheck = false) ?

@wouterj
Copy link
Member
wouterj commented Apr 20, 2013

@mvrhov then I would prefer isValid($submittedCheck = false), but I don't like that (and it causes a BC break)

@mvrhov
Copy link
mvrhov commented Apr 20, 2013

either way is fine by me. :) Whatever is shorter...

@gunnarlium
Copy link
Contributor

My first instinct would be to not consider an unsubmitted form to be valid. An unsubmitted form is in effect a form where all values are unset, which will very rarely be considered valid. And even if that were allowed for a form, the user submitting the form wouldn't have explicitly declared that the intention was for all fields to be blank.

If anything, isValid/valid should return null, but that wouldn't be very useful in this context. hasErrors seems to be more correct semantically. What you want to do is to display some HTML if the form has errors. I think the use of valid in the current template code (not form.vars.valid) is misuse of a "bug", and not the preferred wording for the use case.

I also believe $form->isSubmitted() && $form->isValid() reads much better than either combination with a true/false. With true/false, you need to look up the documentation to see what the code does, and you are quite likely to not remember the meaning of true/false. It could just all be isValid($skipCheckSubmitted) as isValid($checkSubmitted). If two methods are too much, it should at least be isSubmittedAndValid().

@gunnarlium
Copy link
Contributor

@bschussek re

"You could consider a "valid form" to be simply one without errors. Option 2 satisfies this. And the valid variable > makes the same assumption."

The fact that you could consider a "valid form" form to mean two different things says to me that the term valid form shouldn't be used. If something can be misinterpreted, it will be misinterpreted.

@peterrehm
Copy link
Contributor

@gunnarlium But from the view perspective the form can be valid if not submitted which just means no errors to display.

@bschussek How about a rename in the view and combining isValid with isSubmitted? Of course keep aliases to avoid bc breaks.

@gunnarlium
Copy link
Contributor

@peterrehm This supports my argument that the term "valid" isn't the right one in this context. What you want is to do something if the form has errors: if (hasErrors) displayErrors().

@webmozart
Copy link
Contributor Author

isValid and isSubmitted can't be combined, they have a different logic.

hasErrors is even more ambiguous than isValid, as it is not clear whether you check whether this specific form node has errors or if any node in the form tree has errors.

@webmozart
Copy link
Contributor Author

The fact that you could consider a "valid form" form to mean two different things says to me that the term valid form shouldn't be used. If something can be misinterpreted, it will be misinterpreted.

That's true, but it's hard to find a good alternative. Documentation helps to clarify such uncertainties.

@peterrehm
Copy link
Contributor

@bschussek If it is not possible to combine both I would consider leaving the inconsistency and documenting it.

@malberts malberts mentioned this issue Oct 9, 2014
Closed
@webmozart webmozart changed the title [Form] Inconsistency between isValid() and the "valid" template variable [3.0] [Form] Inconsistency between isValid() and the "valid" template variable Oct 16, 2014
@ghost
Copy link
ghost commented May 13, 2015

What do you think now? Because it is referenced to 3.0 a BC break would be allowed.

@dosten
Copy link
Contributor
dosten commented May 13, 2015

I prefer the option 2, seems to be the more intuitive.

@xabbuh
Copy link
Member
xabbuh commented May 14, 2015

I would go for n°3 as an unsubmitted form is neither valid or invalid. Despite the additional code.

I feel the same. Though we then again would have another inconsistency: If the form was not submitted before, you would not be able to call isValid(), but you would still be able to use valid in the template.

@GuilhemN GuilhemN mentioned this issue Feb 1, 2016
Merged
@GuilhemN
Copy link
Contributor
GuilhemN commented Feb 1, 2016

please see #17644

fabpot added a commit that referenced this issue Jun 17, 2016
@fabpot
feature #17644 Deprecate using Form::isValid() with an unsubmitted fo…
7ccfdb4 
…rm (Ener-Getick)

This PR was merged into the 3.2-dev branch.

Discussion
----------

Deprecate using Form::isValid() with an unsubmitted form

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

> I'm calling out for you to decide how to resolve the inconsistency between the Form::isValid() method and the ``valid`` variable available in the template:
>
> - ``isValid()`` returns ``false`` if a form was not submitted. This way it is possible to write concise controller code:
>
> ```php
> $form = $this->createForm(...);
> $form->handleRequest($request);
> if ($form->isValid()) {
>     // only executed if the form is submitted AND valid
> }
> ```
>
> - ``valid`` contains ``true`` if a form was not submitted. This way it is possible to rely on this variable for error styling of a form.
>
> ```twig
> <div{% if not form.vars.valid %} class="error"{% endif %}>
> ```
>
> We have two alternatives for resolving this problem:
>
> 1. Leave the inconsistency as is.
> 2. Make ``isValid()`` return ``true`` if a form was not submitted (consistent with ``valid``)
> 3. Revert to the 2.2 behavior of throwing an exception if ``isValid()`` is called on a non-submitted form (not consistent with ``valid``).
>
> Both 2. and 3. will require additional code in the controller:
>
> ```php
> $form = $this->createForm(...);
> $form->handleRequest($request);
> if ($form->isSubmitted() && $form->isValid()) {
>     // only executed if the form is submitted AND valid
> }
> ```
>
> What do you think?

This PR implements the option 3 as it was the most chosen in #7737

Commits
-------

2c3a7cc Deprecate using Form::isValid() with an unsubmitted form
@fabpot fabpot closed this as completed Jun 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Enhancement Form
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
0