Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
Explained a more simple method of dynamic form handling available since #8827#2927
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
wouterj commentedAug 24, 2013
could you please includethe pull request format in your description? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
->add('sport','entity',array(...));
Burgov commentedAug 24, 2013
@wouterj I'll apply your changes accordingly. Thanks for the review. However, I'm up for a git challenge now. I created the branch for#2928 based on the branch for this one, rebased onto 2.3 and added an additional commit for the extra changes. If I rewrite and rebase -i the changes you explained in this PR, how can I cleanly merge them into#2928 without all git hell breaking loose? :) |
wouterj commentedAug 24, 2013
Well, as said in#2928, you should create that PR after this one is merged. The reason is that you now have 2 commits doing the same thing in 2 branches. That'll confuse GIT. So, first do this PR. Let it review by@weaverryan and let him merge this one. After that, create a new PR adding new stuff for the 2.3 branch. I know it's a worse workflow, but I didn't found a better one yet. |
Burgov commentedAug 24, 2013
@wouterj ah right. I thought that after rebasing, the exact same commit would be in both PR's, but then I forgot that I had to fix merge conflicts in the rebase, which was amended, effectively creating a new commit. I'll wait for the 2.3 PR then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's not really necessary to pass the factory here, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Since the chapter is about dynamic form modification, you're going to need the $factory at some point, even though it isn't in the example. I've changed the example to use it though, to make it more clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not necessarily, sinceForm::add() supports the same interface asFormBuilder::add().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah! I thought that was a 2.3 feature but I see it's been there since 2.2. I'll update the examples
webmozart commentedAug 24, 2013
Thank you@Burgov! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think theversionadded directive can be omitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Are you sure about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As far as I understand this was just a bugfix and not a new feature. There are no other places in the docs where we mention specific bugfixes. So I see no reason to do it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Interesting, I saw it as a new feature :) You could probably argue for both... I have removed the directive as suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
If you have a look at therelease notes, you can see that it is listed there as a bugfix, too.
weaverryan commentedSep 9, 2013
This is great - thanks@Burgov! I made some small tweaks at sha:b998f86 - there was one actual error, fixed here:b998f86#L0R501. I'm pretty confident in the change, but let me know if I'm wrong :). Cheers! |
symfony/symfony#8827
This PR explains the changes in the above mentioned PR.
Also I rewrote the chapter a bit, as it was pretty confusing. In the listener, the client data "event" was used to update the form in PRE_BIND, but in the example this field wasn't even added to the form, making the example broken.
cc @bschussek