Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Form] Added option type checks in some FormTypes#12425

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

Closed
kix wants to merge1 commit intosymfony:masterfromkix:form_option_type_checks

Conversation

kix
Copy link
Contributor

@kixkix commentedNov 6, 2014

QA
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsnone
LicenseMIT
Doc PRnone

Follow-up to#11696. Some of the built-in form types allowed passing invalid options, which lead to errors that were not so easy to understand. This PR adds needed checks where applicable.

Allowed types inTimeType'sempty_value option were taken from the documentation.

@kixkix changed the titleForm option type checks[Form] Added option type checks in some FormTypesNov 6, 2014
@kixkixforce-pushed theform_option_type_checks branch 2 times, most recently from7136987 toeb7e9b4CompareNovember 6, 2014 19:12
@@ -239,7 +239,9 @@ public function setDefaultOptions(OptionsResolverInterface $resolver)
));

$resolver->setAllowedTypes(array(
'choices' => array('null', 'array'),
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong. It also support Traversable

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@stof, however, passing anArrayObject which implementsTraversable triggers an exception due toSimpleChoiceList::__construct$choices parameter type hint.
This is a test case that would fail against the currentmaster branch:

namespaceSymfony\Component\Form\Tests\Extension\Core\Type;useSymfony\Component\Form\Extension\Core\ChoiceList\ObjectChoiceList;useSymfony\Component\Form\Extension\Core\View\ChoiceView;class ChoiceTypeTestextends \Symfony\Component\Form\Test\TypeTestCase{private$choices =array('a' =>'Bernhard','b' =>'Fabien','c' =>'Kris','d' =>'Jon','e' =>'Roman',    );// ...publicfunctiontestChoicesOptionSupportsTraversable()    {$this->factory->create('choice',null,array('choices' =>new \ArrayObject($this->choices),        ));    }}

And this is how it fails:

There was 1 error:1) Symfony\Component\Form\Tests\Extension\Core\Type\ChoiceTypeTest::testChoicesOptionSupportsTraversableArgument 1 passed to Symfony\Component\Form\Extension\Core\ChoiceList\SimpleChoiceList::__construct() must be of the type array, object given, called in /Users/kix/Documents/Code/php/symfony/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php on line 176 and defined/symfony/src/Symfony/Component/Form/Extension/Core/ChoiceList/SimpleChoiceList.php:45/symfony/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php:176/symfony/src/Symfony/Component/OptionsResolver/OptionsResolver.php:836/symfony/src/Symfony/Component/OptionsResolver/OptionsResolver.php:769/symfony/src/Symfony/Component/Form/ResolvedFormType.php:109/symfony/src/Symfony/Component/Form/FormFactory.php:87/symfony/src/Symfony/Component/Form/FormFactory.php:67/symfony/src/Symfony/Component/Form/FormFactory.php:39/symfony/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php:1302

Should this become a separate issue? Thedoc clearly states that the a valid'choices' option has to be be anarray.

Copy link
Member

Choose a reason for hiding this comment

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

hmm, the choice type itself indeed does not support iterators. but child types (for instance the EntityType) are supporting it, while your change prevents it

Copy link
Member

Choose a reason for hiding this comment

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

And the condition in

if (!$options['choice_list'] && !is_array($options['choices']) && !$options['choices']instanceof \Traversable) {
let me think that iterators were meant to be supported, and that this is a bug in the SimpleChoiceList conversion

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Also, shouldthis check be implemented in theChoiceType at all? It looks like anOptionsResolver's responsibility to filter out invalid values.

Copy link
Member

Choose a reason for hiding this comment

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

The OptionsResolver cannot make an option required only when the other is not there.

@kix
Copy link
ContributorAuthor

kix commentedNov 7, 2014

Also, looking atthis linewhich @stof has linked to, I think that the exception message could be more clear. If I pass a non-\Traversable and non-array argument inside'choices' option, I'd get an exception thrown that says I'm not passing anything, while I'm passing bad values.

@kix
Copy link
ContributorAuthor

kix commentedNov 27, 2014

Seems like we should wait for#12148 here?

@fabpotfabpot added the Form labelDec 7, 2014
@kix
Copy link
ContributorAuthor

kix commentedDec 12, 2014

Ping@stof@webmozart
This is referenced insymfony-docs#1226, that's 7th page of issues. Looks like a 500+100 :)

@stof
Copy link
Member

stof commentedJan 4, 2015

Once the validation for the choices option of the ChoiceType is fixed to avoid breaking BC in child types, I think this should be merged (and it should go in older branches than master)

@stof
Copy link
Member

@kix can you update the PR to fix the case of the ChoiceType ?

@kix
Copy link
ContributorAuthor

kix commentedJan 18, 2015

@stof, yeah, sorry for the wait, been a bit too busy by the end of the year.

@kix
Copy link
ContributorAuthor

kix commentedJan 19, 2015

I've had to remove type hints in someChoiceList's for the test suite to pass. The check is implemented inChoiceList::__construct() instead. Is this wrong?

@@ -100,7 +108,7 @@ public function __construct($choices, array $labels, array $preferredChoices = a
* @param array $labels The labels belonging to the choices.
* @param array $preferredChoices The choices to display with priority.
*/
protected function initialize($choices,array$labels, array $preferredChoices)
protected function initialize($choices, $labels, $preferredChoices)
Copy link
Member

Choose a reason for hiding this comment

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

changing this signature is a BC break

Copy link
Contributor

Choose a reason for hiding this comment

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

@stof is right. However, this class is not an API class, so technically this change is possible, as long as the change and the upgrade path is documented in the UPGRADE file (as perour BC policy). Can you add that information to the UPGRADE file please?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@webmozart, sure, will do.

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a note in the UPGRADE file, it looks like this is the only remaining thing to do before merging.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fabpot This file has been removed in 3.0.

This PR should be rebased before a new review IMHO.

Copy link
Member

Choose a reason for hiding this comment

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

A new PR on master is indeed needed.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

OK, will try to fix it up.

@webmozart
Copy link
Contributor

Looks good! Can you please rebase and document how to upgrade subclasses of ChoiceList in the UPGRADE file? After that, I'm 👍.

@kixkixforce-pushed theform_option_type_checks branch from10508cf to753a08eCompareJune 17, 2015 18:19
@kix
Copy link
ContributorAuthor

kix commentedJun 17, 2015

@webmozart, does7d623ab look good to you? Anything I should add or remove?
On build failure: investigating.

@@ -110,6 +110,9 @@ UPGRADE FROM 2.x to 3.0

### Form

* Type hints for `ChoiceList::initialize()` method's `$labels` and `$preferredChoices`
Copy link
Contributor

Choose a reason for hiding this comment

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

"The array type hints..."

@webmozart
Copy link
Contributor

Could you please rebase your PR onto 2.8? You need to open a new PR for the 2.8 branch as well.

@kix
Copy link
ContributorAuthor

kix commentedJun 18, 2015

@webmozart, sorry, I must've rebased better. Will fix and write a comment once it looks good, OK?

@kixkixforce-pushed theform_option_type_checks branch from7d623ab to34e3a24CompareJanuary 22, 2016 00:18
@kixkix changed the title[Form] Added option type checks in some FormTypes[Form][3.0] Added option type checks in some FormTypesJan 22, 2016
@kixkix changed the title[Form][3.0] Added option type checks in some FormTypes[Form] Added option type checks in some FormTypesJan 22, 2016
@fabpot
Copy link
Member

@kix This one looks easy to finish :)

@kix
Copy link
ContributorAuthor

kix commentedMar 2, 2016

@fabpot, sure!
2 марта 2016 г. 8:36 PM пользователь "Fabien Potencier" <
notifications@github.com> написал:

@kixhttps://github.com/kix This ones looks easy to finish :)

—
Reply to this email directly or view it on GitHub
#12425 (comment).

@xabbuh
Copy link
Member

@kix Any news here? :)

@kix
Copy link
ContributorAuthor

kix commentedMar 29, 2016

@xabbuh, sorry for the wait, I'll try to finish this by the end of the week (hopefully, if nothing unusual comes up)

@HeahDude
Copy link
Contributor

@xabbuh@kix This PR is deprecated by#14050, those changes are not needed anymore.

Excepted allow typearray forentry_options which might be relevant today. Since it's normalized, it will actually make php crash on the call to$value['block_name'], but it's common sense IHMO thatentry_options option inCollectionType is an array, that's probably why no one complained until now.

@kix
Copy link
ContributorAuthor

kix commentedMar 29, 2016

@HeahDude, hmm, thanks, I'll take a look at$value['block_name'], I think. Is it OK to close this PR?

@HeahDude
Copy link
Contributor

Yes :)

@kixkix closed thisMar 29, 2016
@HeahDude
Copy link
Contributor

If you do you should do it on 2.8 I think. Throw the right exception should be a bug fix IMO.

@kixkix deleted the form_option_type_checks branchMarch 29, 2016 13:37
@kix
Copy link
ContributorAuthor

kix commentedMar 29, 2016

@HeahDude, agreed, thanks :)

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

7 participants
@kix@stof@webmozart@fabpot@xabbuh@HeahDude@javiereguiluz
[8]ページ先頭
©2009-2025 Movatter.jp