Skip to content
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

[Validator] - Added Timezone Constraint Validator #13270

Closed
wants to merge 1 commit into from
Closed

[Validator] - Added Timezone Constraint Validator #13270

wants to merge 1 commit into from

Conversation

mickaelandrieu
Copy link
Contributor

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)

comments: was previously, #11010. Re-created because I've deleted my old fork.

ping @webmozart

@xabbuh
Copy link
Member

xabbuh commented Jan 5, 2015

The test class looks like it's just a copy of the constraint validator class.

@mickaelandrieu
Copy link
Contributor Author

@xabbuh already fixed, but well catched 💃

Should be good now :)

* @Annotation
* @Target({"PROPERTY", "METHOD", "ANNOTATION"})
*
* @author Mickaël Andrieu <mickael.andrieu@sensiolabs.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was from SL when I did this pull request.
Extracted from a project with project manager agreement.

edit: I've updated, I'm not attached to my "old" email ;)

@mickaelandrieu
Copy link
Contributor Author

@hhamon @xabbuh thank for this final review.

ping @webmozart : now this is realy done i guess 🐱

*/
class Timezone extends Constraint
{
public $message = 'The value "{{ value }}" is not a valid timezone';
Copy link
Contributor

Choose a reason for hiding this comment

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

This message should be also added to the translations file (at least the English one).

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, how about changing the message to "{{ value }}" is not a valid timezone?

The value "Foo/Bar" is not a valid timezone doesn't sound right to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @jakzal, you're right, I've updated.

@stof
Copy link
Member

stof commented Jan 18, 2015

Please also add test cases against the other validation APIs

@mickaelandrieu
Copy link
Contributor Author

@stof need help here as I don't know what I need to do, did you have an example ?

@@ -306,6 +306,10 @@
<source>The host could not be resolved.</source>
<target>The host could not be resolved.</target>
</trans-unit>
<trans-unit id="80">
Copy link
Member

Choose a reason for hiding this comment

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

The id will need to become 81.

@xabbuh
Copy link
Member

xabbuh commented Mar 7, 2015

@mickaelandrieu You can have a look at the Legacy*ValidatorLegacyApiTest classes.

@mickaelandrieu
Copy link
Contributor Author

After 12 months I think ultimately this constraint does not need to be part of the core . If it had been, it would have been merged long time ago (was validated for 2.4, then 2.5, then 2.6, now 2.7 or 3.0 ?).

I'm sorry for all who have reviewed this one, but I don't want to "waste" time on this (and I don't want others waste time on this too), because this is not so useful and it's better at this point to just close this.

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

Successfully merging this pull request may close these issues.

None yet

6 participants