Skip to content

[Validator] Added a format option to the DateTime constraint. #14531

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

Closed
wants to merge 1 commit into from
Closed

[Validator] Added a format option to the DateTime constraint. #14531

wants to merge 1 commit into from

Conversation

dosten
Copy link
Contributor

@dosten dosten commented May 3, 2015

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

This PR adds a new format option to the DateTime constraint and deprecate the Date and Time constraints in favor of this new option.

  • update docs

@dosten dosten changed the title Added a format option to the DateTime constraint. [Validator] Added a format option to the DateTime constraint. May 3, 2015
@dosten
Copy link
Contributor Author

dosten commented May 4, 2015

Some feedback?

@webmozart
Copy link
Contributor

Thank you for the PR @dosten! As I said in the ticket, I don't want to deprecate Date and Time. Apart from that, this PR looks good.

@dosten
Copy link
Contributor Author

dosten commented May 5, 2015

The UPGRADE-2.8.md need to be removed too?

@dosten
Copy link
Contributor Author

dosten commented May 5, 2015

@webmozart done, if a want to add a violation to a validator is this considered a BC break? Ex: DateTime::getLastErrors() returns an error when the timezone is invalid, in this PR is mapped as a INVALID_FORMAT_ERROR, but i want to submit another PR to add a INVALID_TIMEZONE_ERROR.

@dosten
Copy link
Contributor Author

dosten commented May 22, 2015

ping @webmozart

@xabbuh xabbuh added the Feature label May 23, 2015
@dosten
Copy link
Contributor Author

dosten commented Jun 1, 2015

ping @webmozart

@dosten
Copy link
Contributor Author

dosten commented Jun 15, 2015

Any feedback on this?

@xabbuh
Copy link
Member

xabbuh commented Jun 16, 2015

Looks good to me. But doesn't it make sense to also have the same option in the Date constraint?

@dosten
Copy link
Contributor Author

dosten commented Jun 16, 2015

@xabbuh My first idea was to deprecate the Date and Time constraints because we can do the same with this constraint now. If i add this to the Date (or Time) constraint IMO we would have two constraints that can do the same.

use Symfony\Component\Validator\Constraints\Date;
use Symfony\Component\Validator\Constraints\DateTime;
use Symfony\Component\Validator\Constraints\Time;

// This constraints are equivalent.
$constraint = new DateTime(['format' => 'Y-m-d']);
$constraint = new Date();

// This constraints are equivalent.
$constraint = new DateTime(['format' => 'H:i:s']);
$constraint = new Time();

@xabbuh
Copy link
Member

xabbuh commented Jun 26, 2015

@dosten What you describe will actually be the case right now with only having the format option in the DateTime constraint.

Edit: I mean, we could now indeed use the DateTime constraint to check for only the date or the time, but if we don't deprecate the Date and Time constraints it feels a bit awkward that one would have to use the DateTime constraint if they want to have a custom date format while they could use the Date constraint if the format doesn't need to be customised.

@SoboLAN
Copy link

SoboLAN commented Jun 26, 2015

I agree with @xabbuh on the deprecation of Date and Time. It's probably best instead of having multiple ways of validating the same type of value.

@dosten
Copy link
Contributor Author

dosten commented Jun 26, 2015

I agree with you, IMO we should deprecate these constraints.

@dosten
Copy link
Contributor Author

dosten commented Aug 5, 2015

Can you check this @webmozart ? We can merge this one and discuss the deprecation of the other constraints in another issue?

@dosten
Copy link
Contributor Author

dosten commented Nov 7, 2015

ping someone

@dosten
Copy link
Contributor Author

dosten commented Jan 27, 2016

Closed in favor of #17553

@dosten dosten closed this Jan 27, 2016
@dosten dosten deleted the deprecate-date-constraints branch January 27, 2016 00:39
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.

5 participants