NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Validator] Add`TimezoneValidator`#22262

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

phansys wants to merge14 commits intosymfony:masterfromphansys:tz_validator

Closed

[Validator] Add`TimezoneValidator`#22262

phansys wants to merge14 commits intosymfony:masterfromphansys:tz_validator

Conversation

Copy link

Contributor

phansys commentedApr 4, 2017•
edited
Loading

Q	A
Branch	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	n/a (yet)

AddTimezoneValidator in order to validate timezones against\DateTimeZone::listIdentifiers().

TODO:

Add support for constraints with\DateTimeZone::PER_COUNTRY argument
Improve the validation message

carsonbot added Status: Needs Review Feature labels

Apr 4, 2017

phansys changed the title~~AddTimeZoneValidator~~[Validator] AddTimeZoneValidator

Apr 4, 2017

phansys force-pushed thetz_validator branch fromc82febb to5e8a99fCompare

April 4, 2017 04:27

phansys changed the title~~[Validator] AddTimeZoneValidator~~[Validator] [WIP] AddTimeZoneValidator

Apr 4, 2017

phansys force-pushed thetz_validator branch from5e8a99f to171e4b9Compare

April 4, 2017 04:30

Copy link

Member

nicolas-grekas commentedApr 4, 2017

should be with a lowercase "z", see#21875

nicolas-grekas added this to the3.4 milestone

Apr 4, 2017

phansys force-pushed thetz_validator branch 2 times, most recently fromdbe965b to0328eb0Compare

April 4, 2017 16:08

Copy link

ContributorAuthor

phansys commentedApr 4, 2017

Simple question: sinceDateTimeZone::listIdentifiers() accepts a string with "a two-letter ISO 3166-1 compatible country code" as second argument, could we useCountry validator atTimezone constructor in order to validate thecountryCode argument?

phansys changed the title~~[Validator] [WIP] AddTimeZoneValidator~~[Validator] [WIP] AddTimezoneValidator

Apr 4, 2017

phansys force-pushed thetz_validator branch from7ae8f65 to4d15750Compare

April 4, 2017 19:48

phansys changed the title~~[Validator] [WIP] AddTimezoneValidator~~[Validator] AddTimezoneValidator

Apr 4, 2017

phansys force-pushed thetz_validator branch 2 times, most recently frombbc03de toe5b599fCompare

April 9, 2017 20:21

phansys force-pushed thetz_validator branch frome5b599f to00eb8b7Compare

April 19, 2017 02:07

ro0NL reviewed

Jul 14, 2017

View reviewed changes

src/Symfony/Component/Validator/Constraints/TimezoneValidator.php Outdated

		}

		$this->context->buildViolation($constraint->message)
		->setParameter('{{ extra_info }}', $this->formatExtraInfo($constraint->zone, $constraint->countryCode))

Copy link

Contributor

ro0NLJul 14, 2017

Choose a reason for hiding this comment

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

This is a weird parameter, and binds localized text to this constraint validator... 👎 for that.

What about specialized params instead likezone_id /zone_name or so?

src/Symfony/Component/Validator/Constraints/Timezone.php Outdated

		*/
		public function __construct($options = null)
		{
		if (isset($options['zone'])) {

Copy link

Contributor

ro0NLJul 14, 2017•
edited
Loading

Choose a reason for hiding this comment

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

What about\DateTimeZone::ALL as a default, or, ifcountryCode is given default to\DateTimeZone::PER_COUNTRY instead. So we dont have to expectnull :)

src/Symfony/Component/Validator/Constraints/Timezone.php

		public $message = 'This value is not a valid timezone{{ extra_info }}.';

		protected static $errorNames = array(
		self::NO_SUCH_TIMEZONE_ERROR => 'NO_SUCH_TIMEZONE_ERROR',

Copy link

Contributor

ro0NLJul 14, 2017

Choose a reason for hiding this comment

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

missing few codes here

src/Symfony/Component/Validator/Constraints/Timezone.php Outdated

		const NO_SUCH_TIMEZONE_IN_COUNTRY_ERROR = 'c4a22222-dc92-4fc0-abb0-d95b268c7d0b';

		public $zone;

Copy link

Contributor

ro0NLJul 14, 2017

Choose a reason for hiding this comment

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

newlines between public props can be removed

src/Symfony/Component/Validator/Constraints/TimezoneValidator.php Outdated


		$value = (string) $value;
		$zone = null !== $constraint->zone ? $constraint->zone : \DateTimeZone::ALL;
		$timezoneIds = \DateTimeZone::listIdentifiers($zone, $constraint->countryCode);

Copy link

Contributor

ro0NLJul 14, 2017

Choose a reason for hiding this comment

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

listIdentifiers actually allows zones like\DateTimeZone::AMERICA | \DateTimeZone::EUROPE.. thats cool 👍

Copy link

Member

nicolas-grekas commentedOct 8, 2017

Moving to 4.1. Rebase on master might be needed, where PHP 7.1 features can be used btw.

nicolas-grekas modified the milestones:3.4,4.1

Oct 8, 2017

phansys force-pushed thetz_validator branch 2 times, most recently from58de37e tob8cce71Compare

February 6, 2018 17:50

Copy link

ContributorAuthor

phansys commentedFeb 6, 2018•
edited
Loading

Rebased. I'll be waiting for feedback about the@ro0NL's proposal to useDateTimeZone::ALL as default zone. The other comments were already attended.

phansys force-pushed thetz_validator branch 2 times, most recently from386a46c to72505afCompare

February 26, 2018 03:03

nicolas-grekas modified the milestones:4.1,next

Apr 20, 2018

ro0NL reviewed

Sep 30, 2018

View reviewed changes

src/Symfony/Component/Validator/Constraints/Timezone.php Outdated

		*/
		public function __construct(array $options = null)
		{
		$this->zone = $options['zone'] ?? null;

Copy link

Contributor

ro0NLSep 30, 2018

Choose a reason for hiding this comment

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

calling the parent constructor actually sets the options on properties, so i think we should do the validation step after based on the property values set.

src/Symfony/Component/Validator/Constraints/TimezoneValidator.php Outdated

		*
		* @return array
		*/
		private function generateValidationMessage(int $zone = null, string $countryCode = null): array

Copy link

Contributor

ro0NLSep 30, 2018•
edited
Loading

Choose a reason for hiding this comment

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

i'd prefer not to have this method, the generated messages are also not translator friendly. Instead i would always set the (standard){{ value }} parameter (being the zone ID in this case) see e.g.https://github.com/symfony/symfony/blob/v4.1.5/src/Symfony/Component/Validator/Constraints/CurrencyValidator.php#L48

src/Symfony/Component/Validator/Constraints/Timezone.php Outdated


		public $zone;
		public $countryCode;
		public $message = 'This value is not a valid timezone{{ zone_message }}{{ country_code_message }}.';

Copy link

Contributor

ro0NLSep 30, 2018•
edited
Loading

Choose a reason for hiding this comment

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

i'd sayThis value is not a valid timezone.

the standard seems to not include the value in the message (which in this case cannot be translated), see e.g.https://github.com/symfony/symfony/blob/v4.1.5/src/Symfony/Component/Validator/Constraints/Currency.php#L31

also the message should be added toovalidators.en.xml + any language you know

ro0NL mentioned this pull request

Oct 11, 2018

[Form] Intl and TimezoneType are not interchangeable#22302

Closed

phansys added12 commits

November 29, 2018 09:04

AddTimeZoneValidator

4b87fb7

Add support for\DateTimeZone::PER_COUNTRY

6cceda0

Update constraint message

6124ef6

Renamed "timezone" group to "zone"

49e4b51

Apply CS fixes fromfabbot

288a413

Update method visibility atTimezoneValidator

7545275

Update validation messages

de78802

Update way of handlingzone option

dfa4f98

Add specific errors to be used whenzone orcountryCode are provided

6be950a

Add support for deprecated timezones

6d23035

Remove surplus quotes in validation message

13bc629

Update validation message

0491875

phansys force-pushed thetz_validator branch from72505af toe1831c4Compare

November 29, 2018 13:15

UpdateTimezoneValidator

14689f7

phansys force-pushed thetz_validator branch frome1831c4 to14689f7Compare

November 29, 2018 13:17

ro0NL reviewed

Nov 29, 2018

View reviewed changes

Copy link

Contributor

ro0NL left a comment

Choose a reason for hiding this comment

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

see also#28831, as we have to decide wether we use PHP or Intl for validating.

Add simple assertion atTimezoneTest::testValidTimezoneConstraints()

c3dce5c

Copy link

Contributor

hhamon commentedApr 6, 2019•
edited
Loading

Hey!

What's the status of this PR please? Can I try to finish it?

nicolas-grekas added the ⭐️ EUFOSSA Hackathon label

Apr 6, 2019

hhamon mentioned this pull request

Apr 6, 2019

[Validator] add newTimezone validation constraint#30900

Merged

Copy link

Contributor

hhamon commentedApr 6, 2019

This PR has been reworked here#30900.

Copy link

Member

fabpot commentedApr 6, 2019

Closing in favor of#30900.

fabpot closed this

Apr 6, 2019

phansys deleted the tz_validator branch

April 6, 2019 17:57

fabpot added a commit that referenced this pull request

Apr 6, 2019

feature#30900[Validator] add newTimezonevalidation constraint (…

c73fd10

…phansys)This PR was merged into the 4.3-dev branch.Discussion----------[Validator] add new `Timezone` validation constraint| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| License       | MIT| Doc PR        |symfony/symfony-docs#11317Rework of#22262.Commits-------536e53f [Validator] add new `Timezone` validation constraint.

nicolas-grekas modified the milestones:next,4.3

Apr 30, 2019

Labels

Feature Status: Needs Review

Movatterモバイル変換

Uh oh!

[Validator] AddTimezoneValidator#22262

[Validator] AddTimezoneValidator#22262

Uh oh!

Conversation

phansys commentedApr 4, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commentedApr 4, 2017

Uh oh!

phansys commentedApr 4, 2017

Uh oh!

ro0NLJul 14, 2017

Choose a reason for hiding this comment

Uh oh!

ro0NLJul 14, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NLJul 14, 2017

Choose a reason for hiding this comment

Uh oh!

ro0NLJul 14, 2017

Choose a reason for hiding this comment

Uh oh!

ro0NLJul 14, 2017

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commentedOct 8, 2017

Uh oh!

phansys commentedFeb 6, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

ro0NLSep 30, 2018

Choose a reason for hiding this comment

Uh oh!

ro0NLSep 30, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NLSep 30, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NL left a comment

Choose a reason for hiding this comment

Uh oh!

hhamon commentedApr 6, 2019• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

hhamon commentedApr 6, 2019

Uh oh!

fabpot commentedApr 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[Validator] Add`TimezoneValidator`#22262

[Validator] Add`TimezoneValidator`#22262

phansys commentedApr 4, 2017•
edited
Loading

ro0NLJul 14, 2017•
edited
Loading

phansys commentedFeb 6, 2018•
edited
Loading

ro0NLSep 30, 2018•
edited
Loading

ro0NLSep 30, 2018•
edited
Loading

hhamon commentedApr 6, 2019•
edited
Loading