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 for #2504 #3661

Closed
wants to merge 1 commit into from
Closed

Fix for #2504 #3661

wants to merge 1 commit into from

Conversation

frak
Copy link

@frak frak commented Mar 16, 2016

I encountered issue #2504 when trying to remove elements from the FOS User form. The removal was copy pasted from a working 2.3 install, so I was reasonably sure my code was not at fault. I dug around a little and found that by adding a safety check in the form macro then the issue is resolved.

I don't know much about the root cause, I am guessing it is to do with inherited properties but it is just that - a guess. Maybe someone who knows more about the project can come up with a more eloquent solution?

@@ -1,35 +1,37 @@
{% macro render_groups(admin, form, groups, has_tab) %}
<div class="row">

{% for code in groups %}
{% set form_group = admin.formgroups[code] %}
{% for code in groups %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in twig you can do this directly : {% for code in groups if admin.formgroups[code] is defined %}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed you can, thanks for the tip - I have added this and pushed it now

@core23
Copy link
Member

core23 commented Apr 3, 2016

This would work, but it's not a good solution. Would be better if the code key doesn't exist.

@frak
Copy link
Author

frak commented Apr 3, 2016

@core23 would you have any idea where I can check for and remove this key? I was hoping for a more eloquent solution, but I just need someone to point me in the right direction.

@core23 core23 mentioned this pull request Apr 3, 2016
@core23
Copy link
Member

core23 commented Apr 3, 2016

No idea, I was working on the same problem.

Meanwhile... Can you reproduce this issue on the latest dev-master version?

@core23
Copy link
Member

core23 commented Apr 3, 2016

This is an alternative solution #2814

@frak
Copy link
Author

frak commented Apr 3, 2016

I haven't tried since I created the PR, I will take a look when it is Monday and I am sober :)

@frak
Copy link
Author

frak commented Apr 6, 2016

@core23 Yeah, I just grabbed the latest version and the error is still present.

@soullivaneuh
Copy link
Member

According to the new Sonata version management and next major release plan, this project has been refactored regarding branching and versioning.

If you see this message, your PR concerns a patch or a minor release and is not targeting the right branch.

So I'm closing this one, but don't see it as a refusal. If you think your work is still relevant and want to continue, feel free to reopen it on the right branch (e.g. the default one).

Regards.

@core23 core23 mentioned this pull request May 8, 2016
@frak frak deleted the bugfix/issue-2504 branch May 11, 2016 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants