[Form] Created FormErrorBag #9099

wouterj · 2013-09-22T18:37:26Z

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

The only BC break in this PR is checking it with is_array. Iterating
over the array works just the same as previously:

Normally iterating over the RecursiveIterator results in only
iterating over the errors of the current form.
Iterating using RecursiveIteratorIterator results in iterating over
all errors recursively.

Todo

Adding __toString() support and deprecating Form::getErrorsAsString().
Use this class in Form:getErrors()
Fixing tests
Use field name as key

stof · 2013-09-22T19:40:44Z

src/Symfony/Component/Form/FormErrorCollection.php

+     */
+    protected $errors = array();
+
+    private $_invalid = false;


Please remove the leading underscore. We don't use it in Symfony

I used it because invalid is not a real property of the object, it's just a helper property. But I'll remove it

wouterj · 2013-09-24T07:44:50Z

I've fixed the tests, added some things for BC (see below) and added the last missing features. I think it's ready for review @stof @bschussek

The FormErrorCollection now also implements ArrayAccess, to be BC with accessing the errors using $errors[1]
All fixes in the Validator extension tests are not a proof of BC break, because nobody is going to match the return value of Form::getErrors() with some expected stuff.
The ButtonTypeTests were failing because they passed an array to Form::addError(), I fixed this by converting this array to a FormErrorCollection

wouterj · 2013-09-24T07:46:04Z

src/Symfony/Component/Form/FormErrorCollection.php

+ *
+ * @author Wouter J <wouter@wouterj.nl>
+ *
+ * @since v2.4.0


not sure if this is needed...

Yes, but you can change v2.4.0 to 2.4.

stof · 2013-09-24T08:26:36Z

I think your ArrayAccess is not BC as the offset does not concern only errors but also child error collections. So getting errors[1] can give your a FormErrorCollection while it was giving you the second error previously

webmozart · 2013-09-24T08:26:48Z

src/Symfony/Component/Form/FormErrorCollection.php

+     */
+    public function addCollection($formName, $collection)
+    {
+        // BC with using arrays with errors


Why do we need to support arrays here?

some tests are failing without this. BTW, do you know why the tests are failing now? It seems there is another BC bug...

wouterj · 2013-10-01T16:21:22Z

fixed all comments. I know I'm one day after the feature freeze, but I'd like to see this included in 2.4...

fabpot · 2013-10-01T16:26:39Z

@wouterj You're lucky as I'm still working on merging the last PRs for 2.4... let's wait for @bschussek thoughts first.

Tobion · 2013-10-01T16:51:36Z

src/Symfony/Component/Form/FormErrorBag.php

+        return array_keys($this->errors);
+    }
+
+    public function __toString()


missing phpdoc for many methods

do we really want to have PHPdoc for PHP build-in methods, like __toString etc?

yes because otherwise it's not documented what the string representation actually returns

and this is one of the main points of FormErrorBag

also its not doccumented that it accepts a parameter

the string representation wasn't documented on getErrorsAsString() too, I don't really know why we should do it here. And the parameter is only for internal use.

where is the parameter used internally?

It's used to indent the children forms with 4 spaces

ok as long as its not used outside the class

Tobion · 2013-10-04T09:30:34Z

src/Symfony/Component/Form/CHANGELOG.md

+2.4.0
+-----
+
+ * changed getErrors() to return a FormErrorBag, which is countable,


missing bc break info

what i also mean is that it should be tagged with [BC BREAK], see other changelog entries

also getErrors now returns all errors, whereas it previously returned only direct child errors, or?

@Tobion no it does not. It returns a RecusiveIterator instead of an array. So you can have all errors if you wrap it in a RecursiveIteratorIterator, but the default iteration still gives you only the form errors only (and not the direct child errors as you said in your comment)

asm89 · 2013-10-11T14:34:09Z

@fabpot @bschussek Any chance of getting this in 2.4?

Tobion · 2013-10-11T15:22:09Z

src/Symfony/Component/Form/Form.php

-        }
-
-        return $errors;
+        return (string) $this->getErrors();


it should be $this->getErrors()->__toString($level)

why? Level should be the default, 0, and explicitly calling a magic method seems wrong to me. But if there is any reason why this is better or if this is the CS, I'm happy to change this

@wouterj because if you ignore the $level argument, you are not providing BC totally (even if it was considered as an internal argument, I'm sure some people were using it)

hugohenrique · 2013-11-25T23:28:09Z

ping

asm89 · 2013-11-27T10:01:43Z

ping @fabpot @bschussek I guess this won't make 2.4...

Koc · 2013-12-11T16:00:42Z

is it possible to convert this bag to array like

'metal_companiesbundle_company[companyPhones][1][phone]' => 
    array (size=1)
      0 => string 'Error description.'

?

Where key is form field full name and value is list of the errors for field. It is useful for ajax responses (bind tooltips with errors to fields)

wouterj · 2013-12-25T16:23:29Z

@Koc no, that's not possible with the method itself. But with some array changer functions, you can create something like that.

fabpot · 2013-12-28T13:15:01Z

src/Symfony/Component/Form/Form.php

        }

        return $errors;
    }

    /**
+     * @deprecated Deprecated since version 2.4, to be removed in 3.0. Covert


should be 2.5 and Convert instead of Covert.

Also, can you put everything on the same line?

fabpot · 2013-12-28T13:18:32Z

@wouterj Besides some small typos, looks good to me. Can you also rebase on current master and add a ticket for the documentation? Thanks.

This is fully BC with returning an array and it deprecates Form::getErrorsAsString()

wouterj · 2013-12-28T14:22:21Z

@fabbot everything is now fixed and rebased

wouterj · 2013-12-28T14:23:36Z

doc issue: symfony/symfony-docs#3397

fabpot · 2013-12-28T15:15:43Z

apparently, tests are broken

wouterj · 2013-12-28T19:27:20Z

I'm now stuck... Maybe someone can help me?

When using the RecursiveIterator directly, it should only return the FormError instances in the root of the array and not the FormError instances in the FormErrorBag. This all works fine in the current method, we just skip the value when it's an instance of FormErrorBag and see if the next one is a FormError instance.

However, in the valid method, we can't do it that way. If we skip the FormErrorBag instances in that method and only return true when the next element exists and it is a FormError instance, we can't recursively traverse over the child form errors.

Imagine FormErrorBag::$errors contains this:

array(
    [0] => FormError instance (...),
    [child] => FormErrorBag instance (...)
)

When iterating, the following things happen:

FormErrorBag::valid() is called and returns true, [0] exists
FormErrorBag::current() is called and returns [0] element
FormErrorBag::next() is called and sets the pointer to the next element
FormErrorBag::valid() is called and returns true, [child] exists
FormErrorBag::current() is called and returns [child]

In the 5th step it should not have returned the FormErrorBag object, but it can't do nothing else. The valid method already said the element exists and the current method has to return a FormError object. It can't go to the next element, as there is no next element.

How can we fix this, so that the 5th step isn't executed? We can't add a test on instance in the valid method...

igorw · 2013-12-28T21:12:00Z

From what I can tell, you really have two different cases. And you should not be trying to make both work with the same iterator instance.

The original iterator is a FormErrorBag that may recursively contain other FormErrorBag instances. I'll call it $errors. Here's the two cases:

Case A is looking at the top level errors only. In simple terms, this means filter('isFormError', $errors), where isFormError checks for instanceof FormError.

Case B is recursively walking over over the nested FormErrorBag instances (which happen to implement Iterator) and as such you can use a RecursiveIterator. Or in a different sense, you are flattening the nested list-of-lists to a list of FormError elements.

So what you really want is FormErrorBag to contain two methods. One getTopLevelErrors() that returns itself wrapped inside a FilterIterator. And a getAllErrors() that returns itself wrapped inside a flattening RecursiveIterator.

igorw · 2013-12-28T21:32:15Z

After looking at the current implementation I can also pinpoint the problem. It is calling next() from within valid(). It is using recursion on a mutable object which is going to cause lots of problems because it actually already advances the iterator while you are checking the validity.

What it comes down to is this. Currently, you are trying to do the recursion inside FormErrorBag directly, instead of wrapping it inside a RecursiveIterator, which would keep that responsibility separate.

wouterj · 2013-12-30T11:30:45Z

I think you right @igorw we can't mix them into one class. It would be nice if we could though...

However, we have to be BC with an array, so I propose to have a FormErrorBag which extends ArrayIterator (or we keep the current implementation). And it has a method getAllErrors() which returns a RecursiveArrayIterator.

@bschussek I'd love to hear your opinion about this.

🎆

wouterj · 2013-12-31T14:12:21Z

Closing in favor of #9914

stof reviewed Sep 22, 2013
View reviewed changes

wouterj reviewed Sep 24, 2013
View reviewed changes

webmozart reviewed Sep 24, 2013
View reviewed changes

Tobion reviewed Oct 1, 2013
View reviewed changes

This was referenced Oct 4, 2013

[RFC][Form] Add form::getErrorsAsArray #7205

Closed

[Form] Add getErrorsAsArray method #7512

Closed

Tobion reviewed Oct 4, 2013
View reviewed changes

Tobion reviewed Oct 11, 2013
View reviewed changes

fabpot reviewed Dec 28, 2013
View reviewed changes

Created FormErrorBag

725d9ee

This is fully BC with returning an array and it deprecates Form::getErrorsAsString()

wouterj mentioned this pull request Dec 28, 2013

Document form errors symfony/symfony-docs#3397

Closed

wouterj added 2 commits December 28, 2013 20:18

Fixed test

b6bc8c0

[INVALID] Tried to fix the tests

f002137

wouterj mentioned this pull request Dec 31, 2013

[Form] Created FormErrorBag (Approach 2) #9914

Closed

wouterj closed this Dec 31, 2013

wouterj deleted the fix_7205 branch December 31, 2013 14:12

Navigation Menu

[Form] Created FormErrorBag #9099

[Form] Created FormErrorBag #9099

Conversation

wouterj commented Sep 22, 2013

Todo

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wouterj commented Sep 24, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Sep 24, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wouterj commented Oct 1, 2013

fabpot commented Oct 1, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asm89 commented Oct 11, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hugohenrique commented Nov 25, 2013

asm89 commented Nov 27, 2013

Koc commented Dec 11, 2013

wouterj commented Dec 25, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Dec 28, 2013

wouterj commented Dec 28, 2013

wouterj commented Dec 28, 2013

fabpot commented Dec 28, 2013

wouterj commented Dec 28, 2013

igorw commented Dec 28, 2013

igorw commented Dec 28, 2013

wouterj commented Dec 30, 2013

wouterj commented Dec 31, 2013