Skip to content

[Form] Update choice_loader.rst.inc #16500

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

Merged
merged 1 commit into from
Mar 11, 2022

Conversation

issamkhadiri1989
Copy link
Contributor

@issamkhadiri1989 issamkhadiri1989 commented Feb 12, 2022

Hello,
I think it is better to extend the class ConstantsType from the Form\AbstractTypeExtension instead. As we're using the function getExtendedTypes i think it might be better. This method is available in the AbstractTypeExtension class and not in the AbstractType class. I might be wrong but here we're about to create a type extension from the ChoiceType.

@carsonbot carsonbot changed the title Update choice_loader.rst.inc [Form] Update choice_loader.rst.inc Feb 18, 2022
@javiereguiluz
Copy link
Member

Thanks for your contribution. Sadly I don't know if it's correct or not, so I'd need the help from someone who understands Symfony Forms well. Thanks!

@javiereguiluz javiereguiluz added the help wanted Issues and PRs which are looking for volunteers to complete them. label Feb 18, 2022
@xabbuh
Copy link
Member

xabbuh commented Mar 3, 2022

We should change the method instead. So instead of having a getExtendedTypes() method we would implement getParent():

public function getParent(): string
{
    return ChoiceType::class;
}

@xabbuh xabbuh added this to the 5.3 milestone Mar 3, 2022
@issamkhadiri1989
Copy link
Contributor Author

issamkhadiri1989 commented Mar 3, 2022

We should change the method instead. So instead of having a getExtendedTypes() method we would implement getParent():

public function getParent(): string
{
    return ChoiceType::class;
}

Thank you very much @xabbuh for your reply. Yes, i totally agree with this suggestion too.

@javiereguiluz
Copy link
Member

@issamkhadiri1989 do you want to update this PR with the changes proposed by @xabbuh? Thanks!

@issamkhadiri1989
Copy link
Contributor Author

@issamkhadiri1989 do you want to update this PR with the changes proposed by @xabbuh? Thanks!

Hello @javiereguiluz

I appreciate your feedback. Of course, i've just updated the PR. Can you check please ?

Thanks !

@javiereguiluz javiereguiluz modified the milestones: 5.3, 5.4 Mar 11, 2022
@javiereguiluz javiereguiluz changed the base branch from 6.0 to 5.4 March 11, 2022 12:57
@javiereguiluz
Copy link
Member

Issam, thanks a lot for this contribution and all the updates that you did during the review!

Thanks a lot to reviewers too.

@javiereguiluz javiereguiluz merged commit d10e121 into symfony:5.4 Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Form help wanted Issues and PRs which are looking for volunteers to complete them. Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants