Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix nested subform settings #316

Merged

Conversation

vith
Copy link
Contributor

@vith vith commented Nov 5, 2014

Form settings were not being maintained when subforms are involved. If a subform changed a form setting, the parent form would inherit the new value from the subform after the subform was rendered.

I'm assuming the reverse-inheritance wasn't an intended behavior. This patch saves and restores form settings as a stack so that parent forms have their original state after a subform ends.

@vith vith force-pushed the fix_nested_subform_settings branch from 6617297 to a0e05ac Compare November 5, 2014 02:44
@vith vith force-pushed the fix_nested_subform_settings branch from a0e05ac to 9cff642 Compare November 5, 2014 02:50
@vith
Copy link
Contributor Author

vith commented Nov 5, 2014

Not sure what the hhvm test failure is; doesn't look like it was me.

@florianeckerstorfer
Copy link
Member

Thanks for the PR.

The HHVM tests fail because of a bug in HHVM (see facebook/hhvm#3797), it's already been fixed but it seems that v3.3.1 has not been deployed to Travis CI yet.

I'm not really sure if it is a good idea to throw an Exception when the stack is empty? Wouldn't it be better to fall back to the defaults?

@vith
Copy link
Contributor Author

vith commented Nov 5, 2014

It seems to me that situation would be equivalent to having unbalanced pairings of opening and closing <form></form> tags on the page. It shouldn't be possible at all unless something goes very wrong, or someone manually calls the internal-use-only functions I added. Personally I would rather have the thing blow up if that somehow happens.

If you want a way to restore the defaults maybe that should be a separate function?

But if you still would rather it restore defaults let me know and I will change it.

florianeckerstorfer pushed a commit that referenced this pull request Nov 5, 2014
@florianeckerstorfer florianeckerstorfer merged commit 1a76cb0 into braincrafted:develop Nov 5, 2014
@florianeckerstorfer
Copy link
Member

Ok, thanks for the explanation for this behaviour.

@florianeckerstorfer florianeckerstorfer added this to the 2.1.1 milestone Nov 5, 2014
@vith vith deleted the fix_nested_subform_settings branch November 5, 2014 21:27
@vith
Copy link
Contributor Author

vith commented Dec 9, 2014

@althaus How are you triggering {% block form_end %} without {% block form_start %}? I don't explicitly call {{ form_start(form) }} in my templates. The start and end are implicit when using {{ form(form) }}.

@althaus
Copy link
Contributor

althaus commented Dec 9, 2014

@vith We had some rare cases, where we had to manually render the <form> tag and then just used {{ form_end(form) }} to finalize the form with CSRF and end tag.

Nevertheless the reason I had more trouble than expected was our custom {% block form_start %} which included my patch for bootstrap-bundle which got merged inbetween (d81a349) and I just missed to remove it after updating to latest master. So sorry for causing the confusion.

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