[Form] allow to pass the \DateTimeZone::PER_COUNTRY flag to list identifier per country #25165
Conversation
| { | ||
| $timezones = array(); | ||
|
|
||
| foreach (\DateTimeZone::listIdentifiers($regions) as $timezone) { | ||
| foreach (\DateTimeZone::listIdentifiers($regions, $perCountry) as $timezone) { |
There was a problem hiding this comment.
Hm, this is not how listIdentifiers works; you'd pass regions=PER_COUNTRY, country=US
There was a problem hiding this comment.
Ow, OK, ill change the test
54633ce to
493c5f6
Compare
| return new CallbackChoiceLoader(function () use ($regions) { | ||
| return self::getTimezones($regions); | ||
| return new CallbackChoiceLoader(function () use ($regions, $country) { | ||
| return self::getTimezones($regions, $country); | ||
| }); | ||
| }, | ||
| 'choice_translation_domain' => false, | ||
| 'input' => 'string', | ||
| 'regions' => \DateTimeZone::ALL, |
There was a problem hiding this comment.
i think to get the "right" DX we should determine a default (ALL | PER_COUNTRY) based on country is set yes/no.
633042d to
6fadaf2
Compare
|
I think the travis failure could be linked but I don't understand why |
| }); | ||
| }, | ||
| 'choice_translation_domain' => false, | ||
| 'input' => 'string', | ||
| 'regions' => \DateTimeZone::ALL, | ||
| 'regions' => isset($options['country']) ? \DateTimeZone::PER_COUNTRY :\DateTimeZone::ALL, |
There was a problem hiding this comment.
- 'regions' => isset($options['country']) ? \DateTimeZone::PER_COUNTRY :\DateTimeZone::ALL,
+ 'regions' => isset($options['country']) ? \DateTimeZone::PER_COUNTRY : \DateTimeZone::ALL,| )); | ||
|
|
||
| $resolver->setAllowedValues('input', array('string', 'datetimezone')); | ||
|
|
||
| $resolver->setAllowedTypes('regions', 'int'); | ||
| $resolver->setAllowedTypes('country', array('string', null)); |
There was a problem hiding this comment.
- $resolver->setAllowedTypes('country', array('string', null));
+ $resolver->setAllowedTypes('country', array('string', 'null'));?
6fadaf2 to
62fbd44
Compare
|
@ogizanagi @yceruto thanks for the review, PR fixed. |
| @@ -72,11 +75,11 @@ public function getBlockPrefix() | |||
| /** | |||
| * Returns a normalized array of timezone choices. | |||
| */ | |||
| private static function getTimezones(int $regions): array | |||
| private static function getTimezones(int $regions, ?string $country = null): array | |||
| { | |||
| $timezones = array(); | |||
There was a problem hiding this comment.
wondering.. should we throw if country is set and regions is not PER_COUNTRY? Or perhaps even better, let OptionsResolver forbid such a pair.
There was a problem hiding this comment.
Idk, do we really want to enforce this ? I guess if it's not passed then it will not work as expected. OptionsResolver seems a good idea to me.
There was a problem hiding this comment.
@ogizanagi WDYT ?
There was a problem hiding this comment.
If it can be easily achieved using the OptionsResolver (and it probably is through normalizers), that would be a better DX indeed :)
| @@ -72,11 +75,11 @@ public function getBlockPrefix() | |||
| /** | |||
| * Returns a normalized array of timezone choices. | |||
| */ | |||
| private static function getTimezones(int $regions): array | |||
| private static function getTimezones(int $regions, ?string $country = null): array | |||
There was a problem hiding this comment.
no big fan of ? when nulled by default :)
…tifier per country
62fbd44 to
3cc50de
Compare
| }); | ||
| }, | ||
| 'choice_translation_domain' => false, | ||
| 'input' => 'string', | ||
| 'regions' => \DateTimeZone::ALL, | ||
| 'regions' => isset($options['country']) ? \DateTimeZone::PER_COUNTRY : \DateTimeZone::ALL, |
There was a problem hiding this comment.
$options doesn't exist in this context, this is always evaluated to \DateTimeZone::ALL
There was a problem hiding this comment.
Right, this option needs a lazy default to evaluate the country value. (See example)
|
@Simperfit Do you have to finish this one? |
| { | ||
| $choices = $this->factory->create(static::TESTED_TYPE, null, array('regions' => \DateTimeZone::PER_COUNTRY, 'country' => 'US')) | ||
| ->createView()->vars['choices']; | ||
| $this->assertEquals(new ChoiceView('America/Adak', 'America/Adak', 'Adak'), $choices['America']->getIterator()[0]); |
There was a problem hiding this comment.
there is strictly no guarantee that getIterator returns an object implementing ArrayAccess, so this code is very fragile (refactoring the way we build the iterator might break things)
|
Closing as there is no more activity on this PR. |
Working with a hot chocolate ;).
This PR allows passing the PER_COUNTRY flag to DateTimeZone in the TimeZoneType.