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

Jump to bottom

[Validator] Restore default locale in ConstraintValidatorTestCase #44473

Merged
merged 1 commit into from Dec 17, 2021
Merged

[Validator] Restore default locale in ConstraintValidatorTestCase #44473

merged 1 commit into from Dec 17, 2021

Conversation

@rodnaph
Copy link
Contributor

@rodnaph rodnaph commented Dec 6, 2021

Q A
Branch? 6.1
Bug fix? yes
New feature? no
Deprecations? no
Tickets N/A
License MIT
Doc PR N/A

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?
@carsonbot carsonbot added this to the 6.1 milestone Dec 6, 2021
@carsonbot
Copy link

@carsonbot carsonbot commented Dec 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

stof
stof requested changes Dec 6, 2021
View changes
src/Symfony/Component/Validator/Test/ConstraintValidatorTestCase.php Outdated Show resolved Hide resolved
src/Symfony/Component/Validator/Test/ConstraintValidatorTestCase.php Outdated
@@ -60,6 +60,7 @@ abstract class ConstraintValidatorTestCase extends TestCase
protected $propertyPath;
protected $constraint;
protected $defaultTimezone;
protected string $locale;
Copy link
Member

@stof stof Dec 6, 2021

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 as protected ?string $locale = null, or the check line 124 needs to be changed

Copy link
Contributor Author

@rodnaph rodnaph Dec 6, 2021

Choose a reason for hiding this comment

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

I agree, thanks.

@fancyweb
Copy link
Contributor

@fancyweb fancyweb commented Dec 6, 2021

I think it's a bug and you should target 4.4. You can take inspiration from https://github.com/symfony/symfony/pull/40932/files. Also, I'm not sure we need protected methods for that.

@rodnaph rodnaph force-pushed the validator-locale branch from 1d07290 to 8f204b0 Dec 6, 2021
@rodnaph
Copy link
Contributor Author

@rodnaph rodnaph commented Dec 6, 2021

I think it's a bug and you should target 4.4. You can take inspiration from https://github.com/symfony/symfony/pull/40932/files. Also, I'm not sure we need protected methods for that.

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.

@rodnaph rodnaph requested a review from stof Dec 6, 2021
@rodnaph rodnaph force-pushed the validator-locale branch 4 times, most recently from e007660 to a85a33a Dec 13, 2021
@xabbuh xabbuh removed this from the 6.1 milestone Dec 13, 2021
@xabbuh xabbuh added this to the 4.4 milestone Dec 13, 2021
@xabbuh
Copy link
Member

@xabbuh xabbuh commented Dec 13, 2021

I agree with @fancyweb about this being a bugfix.

@carsonbot carsonbot changed the title Reset Validator Locale [Validator] Reset Validator Locale Dec 13, 2021
@derrabus
Copy link
Member

@derrabus derrabus commented Dec 13, 2021

Let's merge to 4.4. 👍🏻

@rodnaph rodnaph force-pushed the validator-locale branch from a85a33a to 3e0af6f Dec 15, 2021
@rodnaph rodnaph changed the base branch from 6.1 to 4.4 Dec 15, 2021
@rodnaph
restore the overriden locale on tearDown - avoid interfering with any…
77267c0 
… configured value

Previously this change 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.
@rodnaph rodnaph force-pushed the validator-locale branch from 3e0af6f to 77267c0 Dec 15, 2021
@rodnaph
Copy link
Contributor Author

@rodnaph rodnaph commented Dec 15, 2021

Rebased on 4.4, with suggested updates.

xabbuh
xabbuh approved these changes Dec 16, 2021
View changes
@fancyweb fancyweb changed the title [Validator] Reset Validator Locale [Validator] Restore default locale in ConstraintValidatorTestCase Dec 17, 2021
@fancyweb
Copy link
Contributor

@fancyweb fancyweb commented Dec 17, 2021

Thank you @rodnaph.

@fancyweb fancyweb merged commit f91c40a into symfony:4.4 Dec 17, 2021
10 of 11 checks passed
@fabpot fabpot mentioned this pull request Dec 29, 2021
Merged
@fabpot fabpot mentioned this pull request Dec 29, 2021
Merged
@fabpot fabpot mentioned this pull request Dec 29, 2021
Merged
@fabpot fabpot mentioned this pull request Dec 29, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yceruto yceruto

@xabbuh xabbuh

@fancyweb fancyweb

@stof stof

Assignees
No one assigned
Labels
Bug Status: Reviewed Validator
Projects
None yet
Milestone
4.4
Linked issues

Successfully merging this pull request may close these issues.

None yet
7 participants
@rodnaph @carsonbot @fancyweb @xabbuh @derrabus @stof @yceruto