Skip to content

Conversation

tvlooy
Copy link
Contributor

@tvlooy tvlooy commented Nov 12, 2012

the index is wrong here and therefore the form_errors will be rendered on the wrong field

wouterj and others added 3 commits October 28, 2012 01:08
It now points to the real documentation of this bundle and the version used in Symfony2.1, which is 1.2.
index should start counting at 0 or form_errors() are rendered at the wrong field
@weaverryan
Copy link
Member

Hi Tom!

Can you explain this a little bit more? If we're applying a wrong index to the new field, when do we see the errors - after submitting and having errors on this new embedded form? Also, without reimplementing this code, I would have thought that giving the new field an index of collectionHolder.children().length makes sense, since if I have 3 fields, they're indexed 0, 1, 2 - so the new field would be 3 (then the next field would be 4).

But let me know - I'm assuming a lot, and it sounds like you've found an issue - just give me a few more details :)

Thanks!

@tvlooy
Copy link
Contributor Author

tvlooy commented Nov 24, 2012

I copy pasted the code from the docs into my project. I filled in some subforms (and left 1 blank) and pressed submit. Then, the errors from the blank form were displayed on the form that came before it. After doing a -1 on the collection it worked the way it should, so my guess was that the docs were incorrect.

Code where it didn't work (I hacked the error displaying to make it work):
https://github.com/tvlooy/SecretSanta/blob/738b99a993e70722063e2b631ff770fe0306dc2f/Symfony2/src/Intracto/SecretSantaBundle/Resources/views/List/create.html.twig

This is 1 commit later where it did work (error displaying hack removed):
https://github.com/tvlooy/SecretSanta/blob/b1c639f1fff065bfe1f49a48f9d47bbaffa1c93d/Symfony2/src/Intracto/SecretSantaBundle/Resources/views/List/create.html.twig

weaverryan added a commit that referenced this pull request Dec 26, 2012
…ing the children, which included the child elements *and* the single "Add a tag" link. Subtracting 1 was correct, but wasn't clear. Counting the number of actual form elements is more straightforward, and then we don't need to subtract 1.
@weaverryan
Copy link
Member

Hey Tom!

Sorry for the delay - I had to find some time to upgrade my local code for this entry to 2.1! And while I couldn't personally replicate any actual "error" display issues, you're absolutely right that we should have a -1, so I originally patched your change into the 2.0 branch at sha: e5360fb. This indeed makes new items have the correct index - e.g. if we have 2 items currently (index 0 and 1), then the new item will have an index of 2.

But upon closer look, if there are 2 children right now, then counting them should give us 2, which is the correct index. So why do we need to subtract 1 from this to get 2? The answer is that counting the children includes the embedded form elements AND the "Add another" link. So, subtracting 1 is technically correct, but I changed our approach to be more clear in sha: 3ab0a5c.

Thanks so much for brining up the issue - it's a tiny fix, but a really good one to track down.

Thanks!

@weaverryan weaverryan closed this Dec 26, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants