8000 [Form] Deprecate not configuring the "widget" option of date/time form types by MrYamous · Pull Request #49588 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] Deprecate not configuring the "widget" option of date/time form types#49588

Merged
nicolas-grekas merged 2 commits intosymfony:6.3from
MrYamous:form/add-widget-html5-option
Apr 19, 2023
Merged

[Form] Deprecate not configuring the "widget" option of date/time form types#49588
nicolas-grekas merged 2 commits intosymfony:6.3from
MrYamous:form/add-widget-html5-option

Conversation

@MrYamous
Copy link
Contributor
@MrYamous MrYamous commented Mar 3, 2023 •
edited by nicolas-grekas
Loading
Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets Related to a suggestion made on symfony/symfony-docs#16600
License MIT
Doc PR TODO

This PR proposes to change the default value of the widget option of date/time form types to single_text, so that the corresponding input fields are rendered using native HTML5 widgets by default.

It does so by deprecating not setting the widget option, so that we can change the default in Symfony 7.

@MrYamous MrYamous requested review from xabbuh and yceruto as code owners March 3, 2023 09:59
@carsonbot carsonbot changed the title [WIP] [Form] Add new html5 widget option - DateType / DateTimeType / TimeType [WIP] Add new html5 widget option - DateType / DateTimeType / TimeType Mar 3, 2023
@carsonbot carsonbot added this to the 6.3 milestone Mar 3, 2023
@carsonbot carsonbot changed the title [WIP] Add new html5 widget option - DateType / DateTimeType / TimeType [Form] [WIP] Add new html5 widget option - DateType / DateTimeType / TimeType Mar 3, 2023
@MrYamous MrYamous changed the title [Form] [WIP] Add new html5 widget option - DateType / DateTimeType / TimeType [Form] Add new html5 widget option - DateType / DateTimeType / TimeType Mar 3, 2023
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Mar 6, 2023 •
edited
Loading

Adding a new value for the widget option + planning to deprecate might make it harder for ppl to upgrade if they need to support many versions of Symfony with the same codebase.

Instead, what about deprecating not setting the widget option and making it default to single_text in v7?

@MrYamous
Copy link
Contributor Author
MrYamous commented Mar 6, 2023

Adding a new value for the widget option + planning to deprecate might make it harder for ppl to upgrade if they need to support many versions of Symfony with the same codebase.

Instead, what about deprecating not setting the widget option and making it default to single_text in v7?

I got your point and this sounds good to achieve the same result of simplification.
However, DateTimeType already has a other default value (choice), won't that cause the same problem for supporting many versions ?

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Mar 6, 2023

We did follow a similar approach in the past. By asking ppl to be explicit in v6, we can then change the default in v7. To support many versions, one should be explicit about the behavior they want in all versions. They can do so immediately.

@MrYamous
Copy link
Contributor Author
MrYamous commented Mar 6, 2023

We did follow a similar approach in the past. By asking ppl to be explicit in v6, we can then change the default in v7. To support many versions, one should be explicit about the behavior they want in all versions. They can do so immediately.

Thanks for explanation
I'll update pr with this approach

@MrYamous MrYamous force-pushed the form/add-widget-html5-option branch from 43635bb to 67893b3 Compare March 11, 2023 23:50
@nicolas-grekas nicolas-grekas force-pushed the form/add-widget-html5-option branch from 67893b3 to 04fac82 Compare April 18, 2023 21:23
@nicolas-grekas nicolas-grekas changed the title [Form] Add new html5 widget option - DateType / DateTimeType / TimeType [Form] Deprecate not configuring the "widget" option of date/time form types Apr 19, 2023
@nicolas-grekas nicolas-grekas force-pushed the form/add-widget-html5-option branch from 04fac82 to a1e3dea Compare April 19, 2023 06:38
nicolas-grekas
nicolas-grekas approved these changes Apr 19, 2023
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.

I added a 2nd commit that updates the codebase to always set the widget option, so that we don't trigger the deprecation ourselves.

@nicolas-grekas nicolas-grekas force-pushed the form/add-widget-html5-option branch from a1e3dea to 31c3db9 Compare April 19, 2023 06:46
@nicolas-grekas nicolas-grekas force-pushed the form/add-widget-html5-option branch from 31c3db9 to 138be68 Compare April 19, 2023 11:28
@nicolas-grekas nicolas-grekas force-pushed the form/add-widget-html5-option branch from 138be68 to 1ca33e7 Compare April 19, 2023 11:52
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Apr 19, 2023

Thank you @MrYamous.

@nicolas-grekas nicolas-grekas merged commit bfd34b3 into symfony:6.3 Apr 19, 2023
@nicolas-grekas nicolas-grekas mentioned this pull request Apr 19, 2023
Closed
@faizanakram99
Copy link
Contributor
faizanakram99 commented Jul 13, 2023

This is a breaking change btw, the combination [html5 => false, widget => single_text] is not possible anymore, widget => single_text causes html5 to be true regardless of the given option

@xabbuh
Copy link
Member
xabbuh commented Jul 20, 2023

@faizanakram99 Please open a new issue. Comments on merged pull requests are likely to get lost.

@faizanakram99 faizanakram99 mentioned this pull request Jul 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@xabbuh xabbuh Awaiting requested review from xabbuh xabbuh is a code owner

@yceruto yceruto Awaiting requested review from yceruto yceruto is a code owner

Assignees

No one assigned

Labels

Feature Form Status: Reviewed

Projects

None yet

Milestone

6.3

Development

Successfully merging this pull request may close these issues.

5 participants

@MrYamous @nicolas-grekas @faizanakram99 @xabbuh @carsonbot
0