Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Validator] Restore default locale in ConstraintValidatorTestCase#44473
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedDec 6, 2021
It looks like you unchecked the "Allow edits from maintainer" box. That is fine, but please note that if you have multiple commits, you'll need to squash your commits into one before this can be merged. Or, you can check the "Allow edits from maintainers" box and the maintainer can squash for you. Cheers! Carsonbot |
src/Symfony/Component/Validator/Test/ConstraintValidatorTestCase.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| protected$propertyPath; | ||
| protected$constraint; | ||
| protected$defaultTimezone; | ||
| protectedstring$locale; |
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.
the property should be defined asprotected ?string $locale = null, or the check line 124 needs to be changed
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 agree, thanks.
fancyweb commentedDec 6, 2021
I think it's a bug and you should target 4.4. You can take inspiration fromhttps://github.com/symfony/symfony/pull/40932/files. Also, I'm not sure we need protected methods for that. |
rodnaph commentedDec 6, 2021
Happy to rebase if that's needed, let me know. I added the protected methods to follow the same pattern as used to manage the timezone, can change that if it's overkill though. |
src/Symfony/Component/Validator/Test/ConstraintValidatorTestCase.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
xabbuh commentedDec 13, 2021
I agree with@fancyweb about this being a bugfix. |
derrabus commentedDec 13, 2021
Let's merge to 4.4. 👍🏻 |
… configured valuePreviously this change was not resetting the locale after changing it to'en' - which affected other tests which relied on this value being theconfigured value (however it was configured).This mirrors the pattern used for the timezone, storing it to be reseton tearDown.
rodnaph commentedDec 15, 2021
Rebased on 4.4, withsuggested updates. |
fancyweb commentedDec 17, 2021
Thank you@rodnaph. |
Previously this code was not resetting the locale after changing it to
en- which affected other tests which relied on this value being the configured value (however it was configured).This mirrors the pattern used for the timezone, storing it to be reset on tearDown.
I've based this on 6.1. If it's valid, I'm unsure if it's classed a bug, or needs UPGRADE notes?