Skip to content

[Form] Deprecated usage of "choices" option in sub types #21919

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.

Sign up for GitHub

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
Apr 5, 2017
Merged

[Form] Deprecated usage of "choices" option in sub types #21919

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions UPGRADE-3.3.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,34 @@ Finder

* The `ExceptionInterface` has been deprecated and will be removed in 4.0.

Form
----

* Using the "choices" option in ``CountryType``, ``CurrencyType``, ``LanguageType``,
``LocaleType``, and ``TimezoneType`` without overriding the ``choice_loader``
option has been deprecated and will be ignored in 4.0.

Before:
```php
$builder->add('custom_locales', LocaleType::class, array(
'choices' => $availableLocales,
));
```

After:
```php
$builder->add('custom_locales', LocaleType::class, array(
'choices' => $availableLocales,
'choice_loader' => null,
Copy link
Contributor

Choose a reason for hiding this comment

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

That doesn't look intuitive to me.

Copy link
Member

Choose a reason for hiding this comment

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

Well, this is because choice_loader has priority over choices in ChoiceType (which is not documented IIRC btw)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And this should not be used ideally, the ChoiceType is more accurate in such cases, I'll update the doc to clarify this.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be "better" to drop this example and just keep the second code snippet? And remove the recommendation to use choice_loader as null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand this does not look pretty, but this is the actual quick fix for some breaking app.

What we can't do is to stop supporting "choices" option as it's inherited from the ChoiceType, and in such cases null is needed because of its precedence.

What we can do is to better document these types options, and explicit that "choices" or "choice_loader" options should be overridden on a ChoiceType or an EntityType only, not on a child already doing so, unless the previous value is reused.

I would like to address your comment, but as I said in a previous outdated diff:

It makes no sense IMO to enforce lazy load something that is already loaded, if you have your choices, just pass them, do not encapsulate them in an objet that will eventually call a function to return the initial thing.

Then the real fix would be to use the ChoiceType (if the previous "choices" values is not used), I could add this instead, but objectively I would let it as is now.

I'm waiting for your final call on this symfony deciders.

Copy link
Member

Choose a reason for hiding this comment

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

I am fine with how it is now. But we should indeed clarify the documentation about the differences of the choices and choice_loader options and when to use what.

));
// or
$builder->add('custom_locales', LocaleType::class, array(
'choice_loader' => new CallbackChoiceLoader(function () {
return $this->getAvailableLocales();
}),
));
```

FrameworkBundle
---------------

Expand Down
25 changes: 25 additions & 0 deletions UPGRADE-4.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,31 @@ Form
}
```

* Using the "choices" option in ``CountryType``, ``CurrencyType``, ``LanguageType``,
``LocaleType``, and ``TimezoneType`` without overriding the ``choice_loader``
option is now ignored.

Before:
```php
$builder->add('custom_locales', LocaleType::class, array(
'choices' => $availableLocales,
));
```

After:
```php
$builder->add('custom_locales', LocaleType::class, array(
'choices' => $availableLocales,
'choice_loader' => null,
));
// or
$builder->add('custom_locales', LocaleType::class, array(
'choice_loader' => new CallbackChoiceLoader(function () {
return $this->getAvailableLocales();
}),
));
```

FrameworkBundle
---------------

Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Component/Form/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ CHANGELOG
3.3.0
-----

* deprecated using "choices" option in ``CountryType``, ``CurrencyType``, ``LanguageType``, ``LocaleType``, and
``TimezoneType`` when "choice_loader" is not ``null``
* added `Symfony\Component\Form\FormErrorIterator::findByCodes()`
* added `getTypedExtensions`, `getTypes`, and `getTypeGuessers` to `Symfony\Component\Form\Test\FormIntegrationTestCase`
* added `FormPass`
Expand Down
8 changes: 7 additions & 1 deletion src/Symfony/Component/Form/Extension/Core/Type/CountryType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,13 @@ public function configureOptions(OptionsResolver $resolver)
{
$resolver->setDefaults(array(
'choice_loader' => function (Options $options) {
return $options['choices'] ? null : $this;
if ($options['choices']) {
@trigger_error(sprintf('Using the "choices" option in %s has been deprecated since version 3.3 and will be ignored in 4.0. Override the "choice_loader" option instead or set it to null.', __CLASS__), E_USER_DEPRECATED);

return null;
}

return $this;
},
'choice_translation_domain' => false,
));
Expand Down
8 changes: 7 additions & 1 deletion src/Symfony/Component/Form/Extension/Core/Type/CurrencyType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,13 @@ public function configureOptions(OptionsResolver $resolver)
{
$resolver->setDefaults(array(
'choice_loader' => function (Options $options) {
return $options['choices'] ? null : $this;
if ($options['choices']) {
@trigger_error(sprintf('Using the "choices" option in %s has been deprecated since version 3.3 and will be ignored in 4.0. Override the "choice_loader" option instead or set it to null.', __CLASS__), E_USER_DEPRECATED);

return null;
}

return $this;
},
'choice_translation_domain' => false,
));
Expand Down
8 changes: 7 additions & 1 deletion src/Symfony/Component/Form/Extension/Core/Type/LanguageType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,13 @@ public function configureOptions(OptionsResolver $resolver)
{
$resolver->setDefaults(array(
'choice_loader' => function (Options $options) {
return $options['choices'] ? null : $this;
if ($options['choices']) {
@trigger_error(sprintf('Using the "choices" option in %s has been deprecated since version 3.3 and will be ignored in 4.0. Override the "choice_loader" option instead or set it to null.', __CLASS__), E_USER_DEPRECATED);

return null;
}

return $this;
},
'choice_translation_domain' => false,
));
Expand Down
8 changes: 7 additions & 1 deletion src/Symfony/Component/Form/Extension/Core/Type/LocaleType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,13 @@ public function configureOptions(OptionsResolver $resolver)
{
$resolver->setDefaults(array(
'choice_loader' => function (Options $options) {
return $options['choices'] ? null : $this;
if ($options['choices']) {
@trigger_error(sprintf('Using the "choices" option in %s has been deprecated since version 3.3 and will be ignored in 4.0. Override the "choice_loader" option instead or set it to null.', __CLASS__), E_USER_DEPRECATED);

return null;
}

return $this;
},
'choice_translation_domain' => false,
));
Expand Down
8 changes: 7 additions & 1 deletion src/Symfony/Component/Form/Extension/Core/Type/TimezoneType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,13 @@ public function configureOptions(OptionsResolver $resolver)
{
$resolver->setDefaults(array(
'choice_loader' => function (Options $options) {
return $options['choices'] ? null : $this;
if ($options['choices']) {
@trigger_error(sprintf('Using the "choices" option in %s has been deprecated since version 3.3 and will be ignored in 4.0. Override the "choice_loader" option instead or set it to null.', __CLASS__), E_USER_DEPRECATED);

return null;
}

return $this;
},
'choice_translation_domain' => false,
));
Expand Down
42 changes: 41 additions & 1 deletion src/Symfony/Component/Form/Tests/Extension/Core/Type/ExtendedChoiceTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@
use PHPUnit\Framework\TestCase;
use Symfony\Component\Form\Forms;
use Symfony\Component\Form\Tests\Fixtures\ChoiceTypeExtension;
use Symfony\Component\Form\Tests\Fixtures\LazyChoiceTypeExtension;

class ExtendedChoiceTypeTest extends TestCase
{
/**
* @group legacy
* @dataProvider provideTestedTypes
*/
public function testChoicesAreOverridden($type)
public function testLegacyChoicesAreOverridden($type)
{
$factory = Forms::createFormFactoryBuilder()
->addTypeExtension(new ChoiceTypeExtension($type))
Expand All @@ -36,6 +38,44 @@ public function testChoicesAreOverridden($type)
$this->assertSame('b', $choices[1]->value);
}

/**
* @dataProvider provideTestedTypes
*/
public function testChoicesAreOverridden($type)
{
$factory = Forms::createFormFactoryBuilder()
->addTypeExtension(new ChoiceTypeExtension($type))
->getFormFactory()
;

$choices = $factory->create($type, null, array('choice_loader' => null))->createView()->vars['choices'];

$this->assertCount(2, $choices);
$this->assertSame('A', $choices[0]->label);
$this->assertSame('a', $choices[0]->value);
$this->assertSame('B', $choices[1]->label);
$this->assertSame('b', $choices[1]->value);
}

/**
* @dataProvider provideTestedTypes
*/
public function testChoiceLoaderIsOverridden($type)
{
$factory = Forms::createFormFactoryBuilder()
->addTypeExtension(new LazyChoiceTypeExtension($type))
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need the type extension here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would keep it like it for consistency with the test above, and because it test both overriding the option directly and doing it with an extension, so looks safer as is.

->getFormFactory()
;

$choices = $factory->create($type)->createView()->vars['choices'];

$this->assertCount(2, $choices);
$this->assertSame('Lazy A', $choices[0]->label);
$this->assertSame('lazy_a', $choices[0]->value);
$this->assertSame('Lazy B', $choices[1]->label);
$this->assertSame('lazy_b', $choices[1]->value);
}

public function provideTestedTypes()
{
yield array(CountryTypeTest::TESTED_TYPE);
Expand Down
48 changes: 48 additions & 0 deletions src/Symfony/Component/Form/Tests/Fixtures/LazyChoiceTypeExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Form\Tests\Fixtures;

use Symfony\Component\Form\AbstractTypeExtension;
use Symfony\Component\Form\ChoiceList\Loader\CallbackChoiceLoader;
use Symfony\Component\Form\Extension\Core\Type\ChoiceType;
use Symfony\Component\OptionsResolver\OptionsResolver;

class LazyChoiceTypeExtension extends AbstractTypeExtension
{
private $extendedType;

public function __construct($extendedType = ChoiceType::class)
{
$this->extendedType = $extendedType;
}

/**
* {@inheritdoc}
*/
public function configureOptions(OptionsResolver $resolver)
{
$resolver->setDefault('choice_loader', new CallbackChoiceLoader(function () {
return array(
'Lazy A' => 'lazy_a',
'Lazy B' => 'lazy_b',
);
}));
}

/**
* {@inheritdoc}
*/
public function getExtendedType()
{
return $this->extendedType;
}
}