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

Deal with help in symfony forms #113

Merged
merged 1 commit into from
Jun 24, 2018

Conversation

odolbeau
Copy link
Member

Add a new extractor to deal with the help attribue available since sf4.1.

Fix php-translation/symfony-bundle#234

@Nyholm
Copy link
Member

Nyholm commented Jun 21, 2018

Thank you. This looks good. I will do a more thorough review tonight.

@Nyholm
Copy link
Member

Nyholm commented Jun 24, 2018

Could you help me by explaining the idea behind isKnownNode?

@odolbeau
Copy link
Member Author

To be honest, I have no idea! :)

This visitor is obviously heavily inspired by the FormTypePlaceholder visitor.
I moved the exiting code starting here in the FormTrait in order to use it in both visitors.

Anyway, I don't know in which case we can visit twice the same node exactly. :/

@Nyholm
Copy link
Member

Nyholm commented Jun 24, 2018

Ah okey. I see that you just moved that function. Good. But I think you forgot to remove from PlaceholderFormType, right?

I’m not sure what the use case is either. Try to temporarly remove it and see what test fail :) (if you want to of course. I do not suggest to remove it)

@odolbeau
Copy link
Member Author

The method is called from PlaceholderFormType too (see here).

If it's OK for you, I can remove this function call in another PR in order to not merge two different things at the same time?

I could either make a PR right now or after this one, as you wish?

@Nyholm
Copy link
Member

Nyholm commented Jun 24, 2018

Do it in another one. I think I like this PR as it is. I’ll merge it next time I’m on the computer.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you for this PR.

@Nyholm Nyholm merged commit 261a9c4 into php-translation:master Jun 24, 2018
@odolbeau odolbeau mentioned this pull request Jun 24, 2018
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.

2 participants