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.

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

Jump to bottom

[Form] Add upgrade path for empty_data option in TextType #41516

Open
wants to merge 10 commits into
base: 7.3
Choose a base branch
from
Open

[Form] Add upgrade path for empty_data option in TextType #41516

wants to merge 10 commits into from

Conversation

peter-gribanov
Copy link
Contributor

@peter-gribanov peter-gribanov commented Jun 2, 2021 •
edited
Loading

Q A
Branch? 6.2
Bug fix? no
New feature? no
Deprecations? yes
License MIT
Doc PR symfony/symfony-docs#15404

Upgrade path for #41514

@peter-gribanov peter-gribanov mentioned this pull request Jun 2, 2021
Closed
@peter-gribanov peter-gribanov force-pushed the form_text_type_stringable_upgrade branch 3 times, most recently from 748c37b to 1b38c73 Compare June 3, 2021 08:02
nicolas-grekas
nicolas-grekas reviewed Jun 3, 2021
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Having all those test cases marked as legacy looks suspicious to me. Should they all really be removed in 6.0? If not, how should they look like then?

@peter-gribanov peter-gribanov force-pushed the form_text_type_stringable_upgrade branch from e103ebb to 4a44fb6 Compare June 3, 2021 09:34
@peter-gribanov
Copy link
Contributor Author

Having all those test cases marked as legacy looks suspicious to me. Should they all really be removed in 6.0? If not, how should they look like then?

@nicolas-grekas I am trying to solve the problem in tests. What is the best way to do this?

Remaining self deprecation notices (575)

@peter-gribanov peter-gribanov mentioned this pull request Jun 3, 2021
Open
@xabbuh
Copy link
Member

xabbuh commented Jun 6, 2021

Marking all these tests as legacy is not the way to go. Instead all the tests should be updated the same way an Symfony user would have to update their code to keep the existing behavior when this PR is merged.

This will allow us to let the tests show us the impact the proposed change has on our users and we can use it as a base for a discussion about whether or not this is worth it.

@nicolas-grekas nicolas-grekas added this to the 5.4 milestone Jun 7, 2021
@peter-gribanov peter-gribanov force-pushed the form_text_type_stringable_upgrade branch from 57ea40c to 7d9556e Compare June 7, 2021 15:55
peter-gribanov
peter-gribanov commented Jun 7, 2021
View reviewed changes
src/Symfony/Component/Form/Extension/Core/Type/TextType.php Outdated
@@ -27,6 +27,9 @@ public function buildForm(FormBuilderInterface $builder, array $options)
// See https://github.com/symfony/symfony/issues/5906#issuecomment-203189375
if ('' === $options['empty_data']) {
$builder->addViewTransformer($this);
} elseif (FormType::$emptyData === $options['empty_data']) {
// "empty_data" option is the default value from FormType::configureOptions()
trigger_deprecation('symfony/form', '5.4', 'The default value of "empty_data" option in "%s" will be changed to empty string. Declare "NULL" as value for "empty_data" if you still want use "NULL" as data.', __CLASS__);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To ensure that this is the default closure and not the user-specified one, we must compare the provided closure with the default closure. To do this, the default closure must be created once and be accessible from the outside.

@peter-gribanov peter-gribanov force-pushed the form_text_type_stringable_upgrade branch from 31f105e to 52279c4 Compare June 7, 2021 16:12
@peter-gribanov
Copy link
Contributor Author

@xabbuh If we want to keep the old behavior, then we must pass option empty_data => NULL. In the major version #41514, it would be better to rewrite the behavior.

@peter-gribanov peter-gribanov force-pushed the form_text_type_stringable_upgrade branch from 52279c4 to ed68472 Compare June 9, 2021 07:30
@peter-gribanov peter-gribanov force-pushed the form_text_type_stringable_upgrade branch from ed68472 to 2306c3a Compare July 15, 2021 10:04
@fabpot
Copy link
Member

fabpot commented Oct 30, 2021

What's the status here?

@fabpot fabpot modified the milestones: 5.4, 6.1 Nov 16, 2021
@peter-gribanov peter-gribanov force-pushed the form_text_type_stringable_upgrade branch 3 times, most recently from 643286f to f1c43f9 Compare November 18, 2021 15:59
@peter-gribanov
Copy link
Contributor Author

What's the status here?

@fabpot needs review

@peter-gribanov peter-gribanov force-pushed the form_text_type_stringable_upgrade branch from d3d2aae to 02257d8 Compare December 28, 2021 14:50
@peter-gribanov peter-gribanov force-pushed the form_text_type_stringable_upgrade branch from 4568571 to 392b761 Compare January 24, 2022 11:46
@fabpot fabpot modified the milestones: 6.1, 6.2 May 20, 2022
@peter-gribanov peter-gribanov force-pushed the form_text_type_stringable_upgrade branch 3 times, most recently from a1be371 to 2d9849f Compare September 27, 2022 13:28
@peter-gribanov peter-gribanov force-pushed the form_text_type_stringable_upgrade branch from 2d9849f to fa0328d Compare September 27, 2022 13:32
@nicolas-grekas nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022
@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@xabbuh xabbuh modified the milestones: 7.1, 7.2 May 15, 2024
@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas

@xabbuh xabbuh

@yceruto yceruto

@chalasr chalasr

@wouterj wouterj

@stof stof

Assignees
No one assigned
Labels
Deprecation Form Status: Needs Review
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.
6 participants
@peter-gribanov @xabbuh @fabpot @nicolas-grekas @stof @carsonbot