Remove hydrator from collections

[fabiocarneiro]

This line shouldn't exist. It shouldn't be necessary to add a hydrator to every collection you have in your project. In the ProductFieldset test asset, there is a collection and there isnt any hydrator for that collection, so we should follow the same pattern.

This was a hack, introduced to get the tests pass, where they should fail, and this is responsible for hiding an error that should be fixed. Instead of adding a hydrator to that collection, the collection class setObject method should be fixed to proper handle nested collections.

[Ocramius]

@fabiocarneiro can you add another failing test the ProductFieldset test asset that exposes the problem that you are outlining here?

[fabiocarneiro]

@Ocramius The rest of the test is ok, the problem is that line that shouldn't be there. Its a 'hack' to make the collection class look like it is working, while it isnt.

If you take a look in the ProductFieldset test asset:(https://github.com/zendframework/zf2/blob/master/tests/ZendTest/Form/TestAsset/ProductFieldset.php#L45)
You'll see its not being defined any hydrator for the CategoryFieldset Collection. That ProductFieldset wasn't created by the same person who wrote this test. So why would this setHydrator() be necessary in this test method, if it wasnt necessary in that test asset? Only because that is the first level and this is the second level? Only because in that case you didnt have nested collections?

That was added, because the recursion in the Collection class isnt working like it should, and when you have nested collections, there is a bug that the collection tries to set an array to a fieldset(which expects an object). You can remove that line and run the test, you'll se what i'm talking about.

With that, the test is passing, but that doesnt solve the problem, if you had a 3rd level, you would need to have a hydrator for that too. What if you have 10 nested collections (i know it seems impossible, but who ever know...)? Also, that's very non-intuitive, i would never guess i had to set a hydrator for a collection if i didnt read this test. And even reading this, it didnt work in my project, because in my project that arrays are doctrine ArrayCollections, so that hydrator didnt work, and i dont know why, the doctrine hydrator didnt work too.

Collections are arrays of form elements. In most of cases, they're arrays of fieldsets. If you have an array of a Product Entity, and you set it to a collection, the collection should get each element(object) of that array and set it to the fieldset of that collection. All collections work the same way, so why would it need a hydrator? As @danizord told me and i agree with him, the collection class shouldnt even allow us to set a hydrator. You already can set the hydrator in the Fieldsets, you dont need it in the Collections.

I know i'm not providing the solution to this problem, i would if i knew how to, but nobody will also do that if the error is hidden in the tests. I'm aiming to get more people involved in this issue, since we cant wait to zf3 with something that isnt working.

If you dont trust me, talk to @danizord, he has more experience with zf than me and we've been working on this issue together.

[danizord]

Really makes no sense assign a hydrator to a collection. We can just traverse it and set data to each child element.

[prendit]

I've struggled with the same problem as @fabiocarneiro and discussed it over the IRC channel. No one could figure it out back then, so I've created a gist to show how I did solved the recursion problem. Of course this is not an acceptable solution, that's why I asked on the first place.
Here is a link to the gist, where I posted the "fix" for the extract() function of the fieldset (back then I thought it was because collection was extending fieldset, which caused the behavior, @fabiocarneiro explained):
https://gist.github.com/prendit/8237520

[weierophinney]

I understand the implications, but with that line removed, tests fail -- which either indicates the line needs to be there, or, as you assert, it was incorrect. If it was incorrect, then the question is: what needs to be done to get tests passing again? Was the original test invalid? Do we need to make changes to the code to properly implement the behavior being tested?

Personally, I've never used collections, and I've struggled understanding them when merging PRs that deal with them. I need some assistance here from people who understand the feature and/or the problem. I'm looking at you, @fabiocarneiro and @danizord ...

[texdc]

A Collection has no direct need of a hydrator. However, having a $defaultHydrator to use for the collected elements could be useful.

[weierophinney]

@Maks3w Can you assist @fabiocarneiro with a fix on this, by any chance?

[radnan]

I've got a fix for the test here radnan@d0fb650.

I've explained it in the commit message, but essentially either the collection needs to have a hydrator, or its target element needs to have an object and the right hydrator in order to be able to extract the values.

[Maks3w]

@radnan I've open a PR of your patch to the fork branch fabiocarneiro#1

[fabiocarneiro]

@radnan and @Maks3w, that test seems fine. However, I dont agree with that merge, because you're calling setObject(). That method is bind() extract() responsability. Once you bind your model object to the form, it already calls setObject() and extract(), that extract() call will handle all the nested setObject calls. It is only a matter of having the right hydrator in each fieldset.

I'll agree with the second part, "target element needs to have an object and the right hydrator in order to be able to extract the values", but not with the first, since collections are always arrays and since they always receive the same type of data, they don't need a hydrator.

[radnan]

@fabiocarneiro I can see where you're coming from but the setObject() and setHydrator() calls go hand in hand. The bind() and extract() process comes after. This is especially true for nested fieldsets, where you're not binding to anything explicitly and the nested setObject() calls have to handle that case.

Let's take an example. Say we have a Shop entity which has a product property. This product property is supposed to contain a Product entity. But when we first create a shop, we haven't set a Product entity yet. So, when we bind this Shop entity to the ShopForm, the ProductFieldset inside doesn't have anything to bind to. But we need an object to hydrate and return - where is it going to get this object? In comes setObject(new Product). Now the fieldset can hydrate a brand new Product entity unless the bind() call from the form attaches an existing object to the fieldset.

[fabiocarneiro]

You're right. That's true for the default object in the fieldset. I forgot i call setHydrator(...)->setObject(new ...) in my fieldsets, but since this is a test, it isn't possible to do like that.