-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Validator] Add Week
constraint
#57908
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
Conversation
3a001a1
to
049cf51
Compare
049cf51
to
a933560
Compare
What I wonder: Do we really need the |
ISO8601 only defines that week number must be between 1 and 53 it seems, and the week number is not strictly validated depending on the year. To me, it is useful depending on if you only want to stick to ISO or if you want a little bit of additional check. |
I think I agree with @xabbuh here. In the equivalent So, I think we should remove the An additional comment: should we add the |
a933560
to
bcec64d
Compare
Thank you Javier for your super relevant feedback! I think @xabbuh and you are right about |
bcec64d
to
cbc3c4c
Compare
throw new UnexpectedValueException($value, 'string'); | ||
} | ||
|
||
if (!preg_match('/^\d{4}-W(0[1-9]|[1-4][0-9]|5[0-3])$/', $value)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something we might want to do on all validators:
if (!preg_match('/^\d{4}-W(0[1-9]|[1-4][0-9]|5[0-3])$/', $value)) { | |
if (!preg_match('/^\d{4}-W(0[1-9]|[1-4][0-9]|5[0-3])$/D', $value)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware of this modifier, thanks. I added it, I'll have a look at a follow-up PR to add it where it could be relevant. Would it be considered a bugfix or new feature? Not sure on this one
3fe0b10
to
9328a5c
Compare
src/Symfony/Component/Validator/Tests/Constraints/WeekValidatorTest.php
Outdated
Show resolved
Hide resolved
9328a5c
to
4079ab5
Compare
Thank you Alexandre. |
…(alexandre-daubois) This PR was merged into the 5.4 branch. Discussion ---------- [Validator] Add `D` regex modifier in relevant validators | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | - | License | MIT After #57908 (comment). I'm not sure if this should be considered a bugfix or a feature. Commits ------- 7d1032b [Validator] Add `D` regex modifier in relevant validators
…(alexandre-daubois) This PR was merged into the 5.4 branch. Discussion ---------- [Validator] Add `D` regex modifier in relevant validators | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | - | License | MIT After symfony/symfony#57908 (comment). I'm not sure if this should be considered a bugfix or a feature. Commits ------- 7d1032bbea [Validator] Add `D` regex modifier in relevant validators
Validates weeks in the format
2024-W32
. You can also passmin
andmax
weeks to validate and range of weeks.