[Form] Changed Form::getErrors() to return an iterator and added two optional parameters $deep and $flatten

[webmozart]

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

See the changes in the UPGRADE files for more information.

[wouterj]

Typo: for

[webmozart]

I rebased this PR and changed the default value of $flatten to true.

[Taluu]

Shouldn't this be $this->elements rather than $this->errors ? Or shouldn't you at least declare the $errors property ?

[Taluu]

And I don't really see the point of having a duplicate between $this->elements and $this->errors, or to have a reference from the $errors array

[Tobion]

@webmozart this->errors is not declared

[Taluu]

My point, exactly :)

[webmozart]

Thanks. See #10418

[ahilles107]

👍

[alexsegura]

👍

This is a must have, currently Form::getErrors is just useless

[fabpot]

I don't like breaking BC at this point. What about adding a new method instead?

[Djeg]

👍

Totaly agree ! I have to do that recursively each time i need it, a must have.

[fabpot]

@webmozart What are we doing with this PR? I still think that breaking BC here is not a good idea. I know that FormatInterface is not marked with the @api tag but still...

[Seb33300]

I can understand you think that breaking BC here is not a good idea, but sometimes, i think it is a better way than keeping useless functionnalities. Especially when the new version looks similar to what the functionality should have been from the beginning.

[wouterj]

@fabpot the only thing that is breaking BC is if you implemented the FormInterface in a custom Form class or if you override that method. I don't believe many people did one of those things...

[fabpot]

@wouterj The returned value also changed... That's the biggest BC break for me.

[webmozart]

We basically have two choices:

(1) We leave this PR as is. This means:

• People who implement FormInterface must add the new optional arguments to getErrors().
• People who use getErrors() need to use iterator_to_array() if they pass the errors to a function that only accepts arrays (array type hint, array_*())

Before:

$errors = $form->getErrors();
echo $errors[0];
echo count($errors);

$mappedErrors = array_map($callback, $errors);

// Print all errors
echo $form->getErrorsAsString();

After:

$errors = $form->getErrors();
echo $errors[0];
echo count($errors);

$mappedErrors = array_map($callback, iterator_to_array($errors));

// Print all errors
echo $form->getErrors($deep = true);

// Count all errors
echo count($form->getErrors($deep = true));

// Get all errors as recursive iterator
$errors = $form->getErrors($deep = true, $flatten = false);

(2) We leave getErrors() unchanged and add the new functionality to a new method instead. Name proposal: getErrorIterator(). This means:

• People who implement FormInterface must add the new method getErrorIterator().

$errors = $form->getErrors();
echo $errors[0];
echo count($errors);

$mappedErrors = array_map($callback, $errors);

// Print all errors
echo $form->getErrorIterator($deep = true);

// Count all errors
echo count($form->getErrorIterator($deep = true));

// Get all errors as recursive iterator
$errors = $form->getErrorIterator($deep = true, $flatten = false);

I created a poll which lets you select one of these answers: http://poll.fm/4obja. I will close the poll in a couple of days.

[hhamon]

For BC I think it's safer to add a new method and deprecate the previous one (to be removed in 3.0).

Instead of modifying the current FormInterface, why not adding a new FormErrorInterface (or something similar) so that a Form instance implements both FormInterface and FormErrorInterface? Those two interfaces may be merged in branch 3.0.

[wouterj]

Instead of modifying the current FormInterface , why not adding a new FormErrorInterface (or something similar) so that a Form instance implements both FormInterface and FormErrorInterface ?

That's a huge code smell imo (sadly it's one of the result of the 100% BC promise). How am I going to typehint a form class where I want to use both the methods in FormInterface and the new FormErrorInterface? That's impossible...

[webmozart]

I agree with @wouterj here. Also, FormInterface is not marked as API, so it may be changed for a good reason.

[webmozart]

Since the poll didn't receive any votes anymore for a few days, I closed it now. The results can be seen here: https://polldaddy.com/poll/7853158/?view=results

• 30 people (51%) voted for keeping the PR as it is: add additional parameters to getErrors() and change the return type to an iterator
• 29 people (49%) voted for adding an extra method getErrorIterator()

The poll shows that the community is just as split about this issue as we are. However, we have a majority, so I propose to merge the PR as it is based on the decision made by the voters.

[Strate]

Method with several bool parameters looks ugly in code:

$errors = $form->getErrors(true, false);

So, in future it can easily evolve to:

$errors = $form->getErrors(true, false, false, false, true, true, false);

Very bad for reading and understanding without looking to method's definition.

If link to poll had been in previous "Week of symfony" blog entry, I would had voted for new method.

[wouterj]

@Strate the new method would have exact the same signature. The question was whether it was a problem that the method returned a Traversable instead of an array and it had a new extra parameter.

[webmozart]

@Strate Can you make a suggestion how to improve the API?

[Strate]

@webmozart For me it would be great having setters for options in iterator class. See examples:
Before:

$form->getErrors($deep=false, $flatten = true);

After:

$form->getErrors()->setDeep(false)->setFlatten(true);

This example more readable for me.

P.S.
Every set method should return NEW instance of iterator, because it can create enexpected behaviour when calling them inside foreach loop.

[webmozart]

@Strate This API is definitely more readable. I would also prefer clear commands over setters, for example:

$form->getErrors()->includeNested()->flatten();

$form->getErrors()->excludeNested()->asTree();

It's not necessary to return a new iterator, because the changed configuration would only take effect as soon as rewind() is called. What do you think?

[webmozart]

One more thought on this: If I call getErrors(), I expect that the result is a list of errors. It's kind of confusing that then entries in this list are able to change by calling additional methods.

[webmozart]

An alternative solution is to split getErrors() into getErrors() and getAllErrors($flatten = true).

// This form's errors only
count($form->getErrors());

// All errors
count($form->getAllErrors());

// All errors as tree
$form->getAllErrors($flatten = false);

[Taluu]

And whatif more parameters are introduced ?

AFAIC, I prefer the commands, which returns a new Iterator though, because as @webmozart said, we would expect a list of errors, not something that is manipulable... With a default set of options (the equivalent of deep = false and flatten = false)

[Tobion]

If we already had named params ... ^^

[Taluu]

Or another option would be to introduce the options not as parameters, but as an array of options : getErrors(array $options), which would contain deep, flatten, ... etc

Moreover, this would allow for different "get errors" implementation to return whatever they want with whatever options they want

[webmozart]

@Taluu I don't foresee that we'll pass more parameters to getErrors(). I don't see the point to introduce a dynamic "options" parameter just for that (small) possibility.

[Tobion]

Please, no "options". Worst invention ever. Most of the time you don't know which options are available. It's so magic.

[wouterj]

What about a new method getErrorCollection which returns the iterator including the commands?

$form->getErrorCollection()->rootOnly()->asTree();
$form->getErrorCollection()->rootOnly();
$form->getErrorCollection()->asTree();

[webmozart]

@wouterj That's the same approach I explained above (only with a different name). My concern stands that the returned collection is able to change by calling methods on it. This seems confusing to me. To put this into perspective, imagine the same in a different domain:

$tags = $article->getTags();

// wtf?
$tags->includeTagsOfRelatedArticles();

[wouterj]

@webmozart I thought you were talking about the same method, while I tried to explain that we might create an extra builder method. I can see your point about the commands :)

[Strate]

@webmozart your example with asTree() and includeNested() really much better than setters.
What about changeable collection, I think that it is not so bad idea. Look at Doctrine's collections:

$entity->getChildren()->map( ... ) // returns new collection, applied with closure.

[webmozart]

@Strate That's a different thing. With the Doctrine collections, you apply a callback on all elements of the collection. That makes sense.

includeNested(), on the other hand, would "magically" add new elements to the collection, without indicating where they come from. That's the point I don't like.

[odoucet]

I think this BC break should be stated clearly in official documentation (maybe here : http://symfony.com/doc/master/book/forms.html ?). I hit this bug because I was doing this :

$errors = $form->getErrors();
if (is_array($errors)) { ... }

As it is no longer an array, code stopped working after updating from 2.4.5 to 2.5.0

[wouterj]

BC breaks are documented in the changelog and this one exists in the CHANGELOG-2.5 file, so imo everything is correct.

[jonmchan]

How is the new deep=true argument supposed to be used? I have a field validation error, and I can get the error from $form->getErrors(true), but there is no information in the FormError object that ties it back to the field, so all I have is the error message - "This value should not be blank." Not really helpful without knowing which field shouldn't be blank...

[Tobion]

@jonmchan there is a second argument flatten. Have you used that?

[jonmchan]

@Tobion - I get the same result with flatten=true or false.

print_r($form->getErrors(true,true);
Array
(
    [0] => Symfony\Component\Form\FormError Object
        (
            [message:Symfony\Component\Form\FormError:private] => This value should not be blank.
            [messageTemplate:protected] => This value should not be blank.
            [messageParameters:protected] => Array
                (
                )

            [messagePluralization:protected] => 
        )

)

Nothing that I can use to tell me which field contains the error.

[wouterj]

@jonmchan do you mean deep=true or deep=false? In deep=true, you have access to the FormErrorIterator which contains information about the form (the ->form property) and thus the name of the field (using ->form->getName()).

For an example, see https://github.com/symfony/symfony/pull/9918/files#diff-d2d282c9214f4b80194de9d815412a1dR81-101