[Validator] Added Timezone Constraint Validator #11010

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
10 participants
Contributor

mickaelandrieu commented May 28, 2014

Q A
Bug fix? [no]
New feature? [yes]
BC breaks? [no]
Deprecations? [no]
Tests pass? [yes]
Fixed tickets []
License MIT
Doc PR yes (symfony/symfony-docs#3907)

jakzal added the Validator label May 28, 2014

mickaelandrieu changed the title from Added Timezone Constraint Validator to [Validator] Added Timezone Constraint Validator May 29, 2014

Contributor

Koc commented Jun 2, 2014

Why not just use Choice constraint http://symfony.com/doc/current/reference/constraints/Choice.html instead of creating new one?

Member

jakzal commented Jun 2, 2014

re #11028

Contributor

mickaelandrieu commented Jun 2, 2014

@Koc well, I didn't think about it. What did you suggest ?
@jakzal IMO the classes are correctly named here, as I consider Timezone as a word and should not be "CamelCased" isn't it ?

@stof stof and 1 other commented on an outdated diff Jun 2, 2014

...Validator/Tests/Constraints/TimezoneValidatorTest.php
@@ -0,0 +1,69 @@
+<?php
+namespace ReflectGen\Bundle\Tests\Constraints;
@stof

stof Jun 2, 2014

Member

wrong namespace

@mickaelandrieu

mickaelandrieu Jun 2, 2014

Contributor

O_O wtf! fixed (and sorry btw)

@stof stof commented on an outdated diff Jun 2, 2014

...Validator/Tests/Constraints/TimezoneValidatorTest.php
@@ -0,0 +1,69 @@
+<?php
+namespace ReflectGen\Bundle\Tests\Constraints;
+
+use Symfony\Component\Validator\Constraints\Timezone;
+use Symfony\Component\Validator\Constraints\TimezoneValidator;
+
+/**
+ * @author Mickaël Andrieu <mickael.andrieu@sensiolabs.com>
+ */
+class TimezoneValidatorTest extends \PHPUnit_Framework_TestCase
+{
+ protected $context;
+ protected $validator;
@stof

stof Jun 2, 2014

Member

should be private

Contributor

mickaelandrieu commented Jun 2, 2014

Thanks for the review @stof

mickaelandrieu referenced this pull request in symfony/symfony-docs Jun 5, 2014

Closed

[WCM] Added docs for new @Timezone constraint #3907

Contributor

mickaelandrieu commented Jun 5, 2014

Hi,

I've added docs too.

Regards,

Member

romainneutron commented Jun 17, 2014

Given that PHP versions do not return the same timezone list (see http://3v4l.org/tELho), I'm not much in favor of such constraint.
If you really want a form constraint, It could be very easily done with a choice that would be generated on the same server that validates

Contributor

mickaelandrieu commented Jun 17, 2014

"If you really want a form constraint, It could be very easily done with a choice that would be generated on the same server that validates"
=> +1, it's the way you make me think we need a new annotation for this kind of validation, which use the same function used to generate the Timezone choice FormType from Form Component.

The annotation should be used in the same server that validates, to avoid the inconsistency of DateTimeZone::listIdentifiers() (thanks for the link, I didn't know this strange behaviors :) )

@webmozart webmozart commented on an outdated diff Sep 26, 2014

src/Symfony/Component/Validator/Constraints/Timezone.php
+* (c) Fabien Potencier <fabien@symfony.com>
+*
+* For the full copyright and license information, please view the LICENSE
+* file that was distributed with this source code.
+*/
+
+namespace Symfony\Component\Validator\Constraints;
+
+use Symfony\Component\Validator\Constraint;
+
+/**
+ * @author Mickaël Andrieu <mickael.andrieu@sensiolabs.com>
+ */
+class Timezone extends Constraint
+{
+ public $message = 'The string "%string%" is not a valid timezone';
@webmozart

webmozart Sep 26, 2014

Contributor

Should be
The value "{{ value }}" is not a valid time zone.

@webmozart webmozart commented on an outdated diff Sep 26, 2014

...Component/Validator/Constraints/TimezoneValidator.php
+ */
+
+namespace Symfony\Component\Validator\Constraints;
+
+use Symfony\Component\Validator\Constraint;
+use Symfony\Component\Validator\ConstraintValidator;
+
+/**
+ * @author Mickaël Andrieu <mickael.andrieu@sensiolabs.com>
+ */
+class TimezoneValidator extends ConstraintValidator
+{
+ public function validate($value, Constraint $constraint)
+ {
+ if (!in_array($value, \DateTimeZone::listIdentifiers(), true)) {
+ $this->context->addViolation(
@webmozart

webmozart Sep 26, 2014

Contributor

This should use the buildViolation() API merged recently:

$this->buildViolation($constraint->message)
    ->setParameter('{{ value }}', $this->formatValue($value))
    ->addViolation();

@webmozart webmozart commented on an outdated diff Sep 26, 2014

...Component/Validator/Constraints/TimezoneValidator.php
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Validator\Constraints;
+
+use Symfony\Component\Validator\Constraint;
+use Symfony\Component\Validator\ConstraintValidator;
+
+/**
+ * @author Mickaël Andrieu <mickael.andrieu@sensiolabs.com>
+ */
+class TimezoneValidator extends ConstraintValidator
+{
+ public function validate($value, Constraint $constraint)
+ {
@webmozart

webmozart Sep 26, 2014

Contributor

This is missing input validation and ignoring of null/"" values. Check LengthValidator for an example.

Contributor

webmozart commented Sep 26, 2014

Could you fix the errors I mentioned? Please don't forget to add tests for the missing cases (null, empty string, invalid input). After that, this should be good to merge. Thanks! :)

Contributor

mickaelandrieu commented Oct 14, 2014

hi @webmozart, I'm on it.

Thanks for your review and for this useful Validator component :)

edit (15/10/14): updated for API 2.5

@mandrieu @mickaelandrieu mandrieu Added Timezone Constraint Validator
3ba897f
Contributor

mickaelandrieu commented Oct 15, 2014

Travis failed, complaining about Memcache. Should I fix it in this pull request ?

Member

jakzal commented Oct 16, 2014

@mickaelandrieu no, memcache issues are not your fault (see php-memcached-dev/php-memcached#126).

Contributor

mickaelandrieu commented Oct 17, 2014

@jakzal thanks for information.

I suppose my work is done now, let me know if this need improvements 😄

Contributor

mickaelandrieu commented Oct 30, 2014

@webmozart is it good for you ?

Contributor

webmozart commented Oct 31, 2014

@mickaelandrieu I currently put this on hold, since 2.6 is already feature frozen. I'll look at this again when the development for 2.7 starts, but so far this looks good! Thanks!

Member

wouterj commented Dec 28, 2014

ping @webmozart

Contributor

mickaelandrieu commented Jan 5, 2015

@webmozart is it good enough for 2.7?
we don't need for 3.x release to add - at least - only 1 validation constraint imo ^^

Travis only complain about 1 failed test:

Declaration of Mock_Memcached_4b9cd8c6::get() should be compatible with that of Memcached::get()

not related to this current pull request /c @jakzal

When merged, I'll update docs (symfony/symfony-docs#3907)

Member

xabbuh commented Jan 5, 2015

@mickaelandrieu Rebasing should make that failure go away as it was fixed a while ago. By the way, IIRC merging will become an issue (at least with the tools the mergers use) given that you deleted your Symfony fork.

Contributor

mickaelandrieu commented Jan 5, 2015

@xabbuh yes.

I was a little bored to always rebase my fork: it's been 8 months I maintain "a fork" of Symfony.

Maybe a cherry pick of commit (3ba897f) can be more easy than a total rebase ? /c @webmozart

Owner

fabpot commented Jan 5, 2015

@mickaelandrieu Cherry-picking is not magic and won't solve merge conflicts; so cherry-picking or rebasing involves the same amount of work.... resolving the conflicts.

Contributor

mickaelandrieu commented Jan 5, 2015

@fabpot it's better at this point to make a new PR, I will spend more time for rebase than actualy copy this code (only 3 classes). What do you think ?

edit: done => closed and updated at #13270

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment