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
Appearance settings

[Form] Remove deprecated a callable empty_data in ChoiceType #25925

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

Closed
wants to merge 6 commits into from
Closed

[Form] Remove deprecated a callable empty_data in ChoiceType #25925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
96e194c
Merge pull request #1 from symfony/master
peter-gribanov Jan 23, 2018
b8ac58c
mark usage of callable empty_data not instance of \Closure is deprecated
peter-gribanov Jan 25, 2018
f8cc144
allow use array as handler
peter-gribanov Jan 25, 2018
62d2c14
add info in UPGRADE and CHANGELOG. Add callable empty_data tests
peter-gribanov Jan 25, 2018
da3c6ac
fix CS
peter-gribanov Jan 25, 2018
eb95111
remove deprecated feature
peter-gribanov Jan 25, 2018
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
19 changes: 19 additions & 0 deletions 19 UPGRADE-4.1.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,25 @@ Validator
* Calling `EmailValidator::__construct()` method with a boolean parameter is deprecated and will be removed in 5.0, use `EmailValidator("strict")` instead.
* Deprecated the `checkDNS` and `dnsMessage` options of the `Url` constraint. They will be removed in 5.0.

Form
----

* Using callable strings as `empty_data` in `ChoiceType` has been deprecated in Symfony 4.1 use a `\Closure` instead.

Before:

```php
'empty_data' => 'SomeValueObject::getDefaultValue',
```

After:

```php
'empty_data' => function (FormInterface $form, $data) {
return SomeValueObject::getDefaultValue();
},
```

Workflow
--------

Expand Down
19 changes: 19 additions & 0 deletions 19 UPGRADE-5.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,25 @@ Validator
* Calling `EmailValidator::__construct()` method with a boolean parameter has been removed, use `EmailValidator("strict")` instead.
* Removed the `checkDNS` and `dnsMessage` options from the `Url` constraint.

Form
----

* Using callable strings as `empty_data` in `ChoiceType` has been deprecated in Symfony 4.1 use a `\Closure` instead.

Before:

```php
'empty_data' => 'SomeValueObject::getDefaultValue',
```

After:

```php
'empty_data' => function (FormInterface $form, $data) {
return SomeValueObject::getDefaultValue();
},
```

Workflow
--------

Expand Down
5 changes: 5 additions & 0 deletions 5 src/Symfony/Component/Form/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
CHANGELOG
=========

4.1.0
-----

* Using callable strings as `empty_data` in `ChoiceType` has been deprecated in Symfony 4.1 use a `\Closure` instead.

4.0.0
-----

Expand Down
6 changes: 5 additions & 1 deletion 6 src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,11 @@ public function buildForm(FormBuilderInterface $builder, array $options)
$emptyData = $form->getConfig()->getEmptyData();

if (false === FormUtil::isEmpty($emptyData) && array() !== $emptyData) {
$data = is_callable($emptyData) ? call_user_func($emptyData, $form, $data) : $emptyData;
if (is_callable($emptyData) && !is_string($emptyData)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You marked the behavior as deprecated, but doesn't this check completely remove the feature? I think you should still support this case and throw a deprecation instead, otherwise people can't upgrade.

This also means your branch can target 4.1 instead of 5.0. To remove it in 5.0, a new patch is required when 4.3 is released in 1.5 year (but not your responsibility).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR for 5.0. For 4.1 see #25924

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how useful it is to make this PR right now, because that means we'll have an open PR for about 1.5 year and then someone else might make a second PR to do the same. If every BC break got a PR open, the amount of open PRs will be tremendous, hence it's custom to open them after 4.3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This #25896 bug, too, will not be resolved for 1.5 years and so what?

I'm a author of this deprecated code and nobody except me knows how to cut it better.
After 1.5 years, everyone will forget what and how. And so i propose to solve a specific problem now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What prevents to create Mileston 5.0 and forget about this PR before release of 4.3 and creation of branch 5.0?

Copy link
Contributor

Choose a reason for hiding this comment

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

will not be resolved for 1.5 years and so what?

It will create a lot of open PRs, which makes it really hard to distinguish between PRs that are just waiting and PRs that could be merged (in theory).

I'm a author of this deprecated code and nobody except me knows how to cut it better.
After 1.5 years, everyone will forget what and how.

The deprecation should make clear what the intended behavior is and just the deprecated branch should be removed. This is how has been all along. In case people working on the removal can't figure it out, they can always ping the original author of the deprecation.

What prevents to create Mileston 5.0 and forget about this PR before release of 4.3 and creation of branch 5.0?

Nothing really, but keeping a PR for that long, is bound to cause issues. As I've mentioned before, it would cause a massive amount of waiting PRs, clogging the PR list. Next to that, it will cause a ton of merge conflicts in the upgrade files, meaning something has to be done eventually anyway. Sometimes, a deprecation gets un-deprecated, that means that every time this happens, the person doing so, has to make sure that the other PR is closed as well. This means extra overhead for the core developers and PRs roaming around that might not go anywhere, ever.

$data = call_user_func($emptyData, $form, $data);
} else {
$data = $emptyData;
}
}
}

Expand Down
16 changes: 16 additions & 0 deletions 16 src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,6 +640,22 @@ public function testSubmitMultipleChoiceExpandedWithEmptyData()
$this->assertSame(array('test'), $form->getData());
}

public function testSubmitSingleChoiceWithClosureEmptyData()
{
$form = $this->factory->create(static::TESTED_TYPE, null, array(
'multiple' => false,
'expanded' => true,
'choices' => array('test'),
'empty_data' => function () {
return 'test';
},
));

$form->submit(null);

$this->assertSame('test', $form->getData());
}

public function testSubmitMultipleNonExpanded()
{
$form = $this->factory->create(static::TESTED_TYPE, null, array(
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.