8000 [Form] Form::getErrors() method is confusing for new symfony users · Issue #25738 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] Form::getErrors() method is confusing for new symfony users #25738

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
enumag opened this issue Jan 10, 2018 · 21 comments
Closed

[Form] Form::getErrors() method is confusing for new symfony users #25738

enumag opened this issue Jan 10, 2018 · 21 comments
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature Form

Comments

@enumag
Copy link
Contributor
enumag commented Jan 10, 2018 •
edited
Loading
Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? yes
Symfony version 4.0

Several times now I saw new inexperienced symfony users getting confused by the Form::getErrors() method. The problem is the boolean $deep parameter with a false default value. Usually it goes like this:

  1. $form->isValid() returns false. Wait what's wrong?
  2. Oh I will call $form->getErrors() to see the problem!
  3. It's empty. If there is no error why is the form not valid?

And here they get stuck for half an hour or more. Of course there is an error but you'll only see it with $form->getErrors(true). I confess that this took me a while to figure out as well when I was new to Symfony. Boolean parameters are just evil.

To remove this confusion I suggest to deprecate the getErrors method in favor of two new methods getAllErrors and getOwnErrors. I can send a PR if this change is wanted.
@yceruto
Copy link
Member
yceruto commented Jan 10, 2018 •
edited
Loading

Could we to deprecate the false default instead and change it to true later?

@enumag
Copy link
Contributor Author
enumag commented Jan 10, 2018

@yceruto You mean as first step deprecating getErrors call without parameter and later on allowing it again with opposite default value? Yeah I think that might be possible as well.

@yceruto
Copy link
Member
yceruto commented Jan 10, 2018 •
edited
Loading

Yes, exactly, with !func_get_args() we can do the trick, if so, we should also suggest to pass false explicitly during this transition to avoid breaking things.

@javiereguiluz javiereguiluz added the DX DX = Developer eXperience (anything that improves the experience of using Symfony) label Jan 13, 2018
@webnet-fr
Copy link
Contributor

IHMO the actual behaviour of getErrors() (without parameters) is good because the method returns the errors of the current form field. In addition the Form class has the $errors property so the attened result of getErrors() is this property.

The real problem of beginners is that they don't understaind that a form is a tree of fields and not a flat array of fields. Maybe it is a good moment for them to realize that fact when they face the result of getErrors() being an empy array ?

@ostrolucky
Copy link
Contributor

Default false value is part of FormInterface, I don't see how would you deprecate that. Splitting it to two methods is better idea.

@COil
Copy link
Contributor
COil commented Feb 27, 2018

This was "true" before, but now the debug bar contains a form and a constraints panel. Beginners should learn to use the debug bar. The doc is also OK: https://symfony.com/doc/current/components/form.html#accessing-form-errors

@ostrolucky
Copy link
Contributor

Finding errors via profiler bundle is a workaround. It doesn't change the fact current behaviour of that method is confusing. Also, people use Form component outside of symfony framework too.

@yceruto yceruto mentioned this issue Jun 11, 2018
Merged
@noahduncan
Copy link

Just my $0.02 here: I'd suggest changing the name of the method from getErrors to getErrorIterator or something similar. My greener self and other new Symfony dev's I've worked with make the mistake of thinking that $form->getErrors() is going to return some kind of array of errors that will be simple to dump / analyze, but instead you get another object with specialized methods you need to call.

The name change would work well with the deprecation and switch of the default values as well.

@weaverryan
Copy link
Member

I very much like the original proposal - something like getAllErrors() and getOwnErrors(). Someone should make a PR :).

This was referenced Dec 8, 2018
Closed
Closed
@zebba
Copy link
zebba commented Dec 8, 2018

I've provided a PR. As stated there this will be a BC break after all.

@chindit chindit mentioned this issue Dec 8, 2018
Closed
@ro0NL
Copy link
Contributor
ro0NL commented Dec 19, 2018

There are 2 competing PRs now, we should close one :)

I like to suggest current getErrors() + a new getErrorsRecursive($flatten = true). IMHO getErrors() already conveys the meaning of "own errors", which happens to be the default behavior.

getErrorsRecursive() benefits from autocompletion discovery, by sharing a common prefix with the current method. And IMHO is self explanatory.

@enumag
Copy link
Contributor Author
enumag commented Dec 19, 2018 •
edited
Loading

IMHO getErrors() already conveys the meaning of "own errors"

@ro0NL The whole point of this issue is the fact that it does NOT convey that meaning at all. Read the initial post. I do agree that getErrorsRecursive sharing the same prefix has some benefit but I don't think it's enough in this case.

@ro0NL
Copy link
Contributor
ro0NL commented Dec 19, 2018

I did :) and i think providing 2 methods starting with getErr.. might takeaway the confusion also. Instead of introducing 2 new ones. This is practical yes.

I think assuming isValid=false meaning getErrors will not be empty is another perspective.

@yarcowang
Copy link
yarcowang commented May 2, 2019 •
edited
Loading

...Removed...

Updated on 2019-05-05:
It seems you can just use the json method from the ControllerTrait to output error information.
(for it internally call Symfony serializer)

In controller:

 return $this->json($form, Response::HTTP_BAD_REQUEST);

@robob4him
Copy link

IHMO the actual behaviour of getErrors() (without parameters) is good because the method returns the errors of the current form field. In addition the Form class has the $errors property so the attened result of getErrors() is this property.

The real problem of beginners is that they don't understaind that a form is a tree of fields and not a flat array of fields. Maybe it is a good moment for them to realize that fact when they face the result of getErrors() being an empy array ?

Why would the form be invalid with that logic? Validation is the current state + sum of the children's state. Why would errors be any different? "getErrors()" is the explanation to the validity state. IMO this method started out as a bug.

@yarcowang If that works that would be AMAZ-sauce.

@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

@ostrolucky
Copy link
Contributor

yes

@carsonbot carsonbot removed the Stalled label Dec 19, 2020
@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

@apfelbox
Copy link
Contributor

Yes

@carsonbot carsonbot removed the Stalled label Jun 21, 2021
@wouterj
Copy link
Member
wouterj commented Jun 21, 2021

Is someone up for a PR?

The bot is a sign that progress on this issue has stalled, which is never a good sign for a feature request

@xabbuh
Copy link
Member
xabbuh commented Jul 2, 2021

I am going to close here as we never managed to agree on any better solution.

@xabbuh xabbuh closed this as completed Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature Form
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
0