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] Validate time without seconds #48907

Merged
merged 1 commit into from Aug 1, 2023

Conversation

xepozz
Copy link
Contributor

@xepozz xepozz commented Jan 7, 2023

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
License MIT
Doc PR symfony/symfony-docs#...

Adds ability to validate time with the Time constraint without having the last part of the regexp that's responsible for seconds.
I don't want to override the Time and create a new one with custom logic because I think that it maybe helpful for others.
Tell me if I need to touch the docs

@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "6.3" but it seems your PR description refers to branch "6.3 for features".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@carsonbot
Copy link

Hey!

I think @alexandre-daubois has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@carsonbot carsonbot changed the title Validate time without seconds [Validator] Validate time without seconds Jan 9, 2023
Copy link
Contributor Author

@xepozz xepozz left a comment

Choose a reason for hiding this comment

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

Let's decide are we using typed properties and generators in tests and then I'll finish the rest remarks.

I'd looked at others constraints when I changed the Time constraint. AFAIR there weren't typed properties, I suppose because of the internal property accessor inside the \Symfony\Component\Validator\Constraint.
Also about tests. The generators haven't been used in TimeTest at least.
If you suggest me to do a chore, I'll better do it in another PR.

@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@fabpot
Copy link
Member

fabpot commented Aug 1, 2023

@xepozz Can you rebase on 6.4 to get rid of the merge commit? Thank you.

@nicolas-grekas
Copy link
Member

Thank you @xepozz.

@nicolas-grekas nicolas-grekas merged commit 268a359 into symfony:6.4 Aug 1, 2023
3 of 9 checks passed
@xepozz xepozz deleted the feature/time-without-seconds branch August 1, 2023 15:39
@xepozz
Copy link
Contributor Author

xepozz commented Aug 1, 2023

Oh I just wanted to do rebase, but you were first 😀
Ok I'l help with docs

This was referenced Oct 21, 2023
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

5 participants