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

• TheFormErrorCollection now also implementsArrayAccess, 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 ofForm::getErrors() with some expected stuff.
• TheButtonTypeTests were failing because they passed an array toForm::addError(), I fixed this by converting this array to aFormErrorCollection

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

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

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

@fabpot @bschussek Any chance of getting this in2.4?

ping

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

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)

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

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

@fabbot everything is now fixed and rebased

doc issue:symfony/symfony-docs#3397

apparently, tests are broken

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

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

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

ImagineFormErrorBag::$errors contains this:

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

When iterating, the following things happen:

1. FormErrorBag::valid() is called and returnstrue,[0] exists
2. FormErrorBag::current() is called and returns[0] element
3. FormErrorBag::next() is called and sets the pointer to the next element
4. FormErrorBag::valid() is called and returnstrue,[child] exists
5. FormErrorBag::current() is called and returns[child]

In the 5th step it should not have returned theFormErrorBag object, but it can't do nothing else. Thevalid method already said the element exists and thecurrent method has to return aFormError 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 thevalid method...

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 meansfilter('isFormError', $errors), whereisFormError checks forinstanceof FormError.

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

So what you really want isFormErrorBag to contain two methods. OnegetTopLevelErrors() that returns itself wrapped inside aFilterIterator. And agetAllErrors() that returns itself wrapped inside a flatteningRecursiveIterator.

After looking at the current implementation I can also pinpoint the problem. It is callingnext() from withinvalid(). 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 insideFormErrorBag directly, instead of wrapping it inside aRecursiveIterator, which would keep that responsibility separate.

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 aFormErrorBag which extendsArrayIterator (or we keep the current implementation). And it has a methodgetAllErrors() which returns aRecursiveArrayIterator.

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

🎆

Closing in favor of#9914