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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Form] Add input + regions options to TimezoneType #23648

Merged
merged 1 commit into from Sep 16, 2017

Conversation

Projects
None yet
7 participants
@ro0NL
Contributor

ro0NL commented Jul 24, 2017

Q A
Branch? 3.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #...
License MIT
Doc PR symfony/symfony-docs#8223

I want to use \DateTimeZone as a model format, with only european timezones in the form available. Hence the new options.

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Jul 25, 2017

Contributor

Not sure, but we might consider favoring datetimezone as default input format; so it's consistent with datetime type.

Also i need the next step basically; integrate timezone type with datetime. A with_timezone option or so.. that would save me a few steps :)

Contributor

ro0NL commented Jul 25, 2017

Not sure, but we might consider favoring datetimezone as default input format; so it's consistent with datetime type.

Also i need the next step basically; integrate timezone type with datetime. A with_timezone option or so.. that would save me a few steps :)

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Jul 25, 2017

Contributor

Updated so that ChoiceLoaderInterface implem is deprecated and uses a CallbackLoader directly. Let me know if this is OK.

Contributor

ro0NL commented Jul 25, 2017

Updated so that ChoiceLoaderInterface implem is deprecated and uses a CallbackLoader directly. Let me know if this is OK.

Show outdated Hide outdated .../Form/Extension/Core/DataTransformer/DateTimeZoneToStringTransformer.php
Show outdated Hide outdated src/Symfony/Component/Form/Extension/Core/Type/TimezoneType.php
@@ -139,6 +170,6 @@ private static function getTimezones()
$timezones[$region][str_replace('_', ' ', $name)] = $timezone;
}
return $timezones;
return 1 === count($timezones) ? reset($timezones) : $timezones;

This comment has been minimized.

@HeahDude

HeahDude Jul 25, 2017

Member

1 === count($timezones) ? $timezones : reset($timezones)?

@HeahDude

HeahDude Jul 25, 2017

Member

1 === count($timezones) ? $timezones : reset($timezones)?

This comment has been minimized.

@ro0NL

ro0NL Jul 25, 2017

Contributor

That would be weird.. :P

@ro0NL

ro0NL Jul 25, 2017

Contributor

That would be weird.. :P

This comment has been minimized.

@HeahDude

HeahDude Jul 25, 2017

Member

Ah yes sorry about that.

@HeahDude

HeahDude Jul 25, 2017

Member

Ah yes sorry about that.

This comment has been minimized.

@HeahDude

HeahDude Jul 25, 2017

Member

But we need an array all the time right? Why this change?

@HeahDude

HeahDude Jul 25, 2017

Member

But we need an array all the time right? Why this change?

This comment has been minimized.

@ro0NL

ro0NL Jul 25, 2017

Contributor

We get a flat array (1 region) or a nested one (multiple regions). Let me explain with a picture :)

image

It's just cosmetic sugar. IMHO.

@ro0NL

ro0NL Jul 25, 2017

Contributor

We get a flat array (1 region) or a nested one (multiple regions). Let me explain with a picture :)

image

It's just cosmetic sugar. IMHO.

This comment has been minimized.

@HeahDude

HeahDude Jul 25, 2017

Member

Alright! Thanks for clarifying.

@HeahDude

HeahDude Jul 25, 2017

Member

Alright! Thanks for clarifying.

Show outdated Hide outdated src/Symfony/Component/Form/CHANGELOG.md
Show outdated Hide outdated UPGRADE-3.4.md
},
'choice_translation_domain' => false,
'input' => 'string',

This comment has been minimized.

@HeahDude

HeahDude Jul 25, 2017

Member

Something else is troubling me. With this new implementation can we still use this choice field as multiple? I guess we should handle an array of timezones too to be fully BC.

@HeahDude

HeahDude Jul 25, 2017

Member

Something else is troubling me. With this new implementation can we still use this choice field as multiple? I guess we should handle an array of timezones too to be fully BC.

This comment has been minimized.

@ro0NL

ro0NL Jul 25, 2017

Contributor

Good catch. Will look into that 👍

@ro0NL

ro0NL Jul 25, 2017

Contributor

Good catch. Will look into that 👍

@ogizanagi

ogizanagi approved these changes Jul 25, 2017 edited

Nice :)

Failure unrelated.

Show outdated Hide outdated UPGRADE-4.0.md

ogizanagi added a commit that referenced this pull request Jul 25, 2017

minor #23652 [Form] Static call TimezoneType::getTimezones (ro0NL)
This PR was merged into the 3.2 branch.

Discussion
----------

[Form] Static call TimezoneType::getTimezones

| Q             | A
| ------------- | ---
| Branch?       | 3.2
| Bug fix?      | no
| New feature?  | no <!-- don't forget updating src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->

Spotted in #23648

Commits
-------

fe48ab1 [Form] Static call TimezoneType::getTimezones
@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 5, 2017

Member

small conflict to fix

Member

nicolas-grekas commented Aug 5, 2017

small conflict to fix

*/
private $choiceList;
/**
* {@inheritdoc}
*/
public function buildForm(FormBuilderInterface $builder, array $options)
{
if ('datetimezone' === $options['input']) {

This comment has been minimized.

@Tobion

Tobion Aug 6, 2017

Member

How about we name the option value DateTimeZone so people can use ['input' => \DateTimeZone::class]? This avoids typos and makes things more explicit as the value directly maps to a class.

@Tobion

Tobion Aug 6, 2017

Member

How about we name the option value DateTimeZone so people can use ['input' => \DateTimeZone::class]? This avoids typos and makes things more explicit as the value directly maps to a class.

This comment has been minimized.

@ogizanagi

ogizanagi Aug 6, 2017

Member

I like this suggestion and we could also apply it for other Date*Type having this input option.
But I'd keep datetimezone as alias for consistency, as I'm not convinced deprecating old option values for other types would be worth it and such aliases are handful too (for configuration based form generation like in EasyAdmin for instance).

@ogizanagi

ogizanagi Aug 6, 2017

Member

I like this suggestion and we could also apply it for other Date*Type having this input option.
But I'd keep datetimezone as alias for consistency, as I'm not convinced deprecating old option values for other types would be worth it and such aliases are handful too (for configuration based form generation like in EasyAdmin for instance).

This comment has been minimized.

@ro0NL

ro0NL Aug 6, 2017

Contributor

so.. strtolower? :)

@ro0NL

ro0NL Aug 6, 2017

Contributor

so.. strtolower? :)

This comment has been minimized.

@ogizanagi

ogizanagi Aug 6, 2017

Member

strtolowering the whole FQCN won't help you much ;)

@ogizanagi

ogizanagi Aug 6, 2017

Member

strtolowering the whole FQCN won't help you much ;)

This comment has been minimized.

@ro0NL

ro0NL Aug 6, 2017

Contributor

i meant if ('datetimezone' === strtolower($options['input'])) {..

then again im not sure using class-ish values for input makes sense (compared to data_class). It kinda implies it supports any class name, which it doesnt.

@ro0NL

ro0NL Aug 6, 2017

Contributor

i meant if ('datetimezone' === strtolower($options['input'])) {..

then again im not sure using class-ish values for input makes sense (compared to data_class). It kinda implies it supports any class name, which it doesnt.

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 16, 2017

Contributor

Should be good :)

Contributor

ro0NL commented Sep 16, 2017

Should be good :)

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Sep 16, 2017

Member

Thanks @ro0NL.

Member

ogizanagi commented Sep 16, 2017

Thanks @ro0NL.

@ogizanagi ogizanagi merged commit 183307b into symfony:3.4 Sep 16, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

ogizanagi added a commit that referenced this pull request Sep 16, 2017

feature #23648 [Form] Add input + regions options to TimezoneType (ro…
…0NL)

This PR was merged into the 3.4 branch.

Discussion
----------

[Form] Add input  + regions options to TimezoneType

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#8223

I want to use `\DateTimeZone` as a model format, with only european timezones in the form available. Hence the new options.

Commits
-------

183307b [Form] Add input  + regions options to TimezoneType

@ro0NL ro0NL deleted the ro0NL:form/timezone branch Sep 16, 2017

ogizanagi added a commit that referenced this pull request Sep 17, 2017

feature #24242 [Form] Remove deprecated ChoiceLoaderInterface impleme…
…ntation in TimezoneType (ogizanagi)

This PR was merged into the 4.0-dev branch.

Discussion
----------

[Form] Remove deprecated ChoiceLoaderInterface implementation in TimezoneType

| Q             | A
| ------------- | ---
| Branch?       | master <!-- see comment below -->
| Bug fix?      | no
| New feature?  | yes <!-- don't forget updating src/**/CHANGELOG.md files -->
| BC breaks?    | yes
| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets | #23648 <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A

Commits
-------

47993a8 [Form] Remove deprecated ChoiceLoaderInterface implementation in TimezoneType

This was referenced Oct 18, 2017

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jan 3, 2018

minor #8223 Documentation input+regions options in TimezoneType (ro0N…
…L, javiereguiluz)

This PR was merged into the 3.4 branch.

Discussion
----------

Documentation input+regions options in TimezoneType

See symfony/symfony#23648

Commits
-------

99f5f3e Minor typo
d6611eb Update timezone.rst
de490ca Update timezone.rst
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment