Skip to content

Update DateValidator.php #17751

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

Update DateValidator.php #17751

wants to merge 1 commit into from

Conversation

mrsnowin
Copy link

With this change DateValidator can be extended and PATTERN checkDate changed for another date format.

@xabbuh
Copy link
Member

xabbuh commented Feb 10, 2016

I do not understand this change. The checkDate() method is not related to the pattern being applied to the input string.

@Tobion
Copy link
Contributor

Tobion commented Feb 10, 2016

Also checkDate is marked as @internal so should not be overwritten anyway. 👎

@mrsnowin
Copy link
Author

@xabbuh
yyyy-mm-dd (original)
const PATTERN = '/^(\d{4})-(\d{2})-(\d{2})$/';
public static function checkDate($year, $month, $day)

dd.mm.yyyy (my)
const PATTERN = '/^(\d{2}).(\d{2}).(\d{4})$/';
public static function checkDate($day, $month, $year)

Different pattern - different order in income params of checkDate function. Original DateValidator always calls own checkDate, not extended. Changing self to $this will salve this problem.

@Tobion
I want to extend Constraints\Date and Constraints\DateValidator to use in my validation. Don't want to add additional date format changing to whole project. Why i cant extend checkDate()?

@xabbuh
Copy link
Member

xabbuh commented Feb 10, 2016

Imo the right approach for your use case would be to introduce a format option like it was proposed for the DateTime constraint in #17553.

@mrsnowin
Copy link
Author

@xabbuh so, datetime validator with format option will solve my problem :)

@jakzal
Copy link
Contributor

jakzal commented Feb 11, 2016

Ok, let's close this one then.

@jakzal jakzal closed this Feb 11, 2016
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