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] 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
Copy link

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

@@ -60,6 +60,7 @@ abstract class ConstraintValidatorTestCase extends TestCase
protected $propertyPath;
protected $constraint;
protected $defaultTimezone;
protected string $locale;
Copy link
Member

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

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 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
Copy link
Contributor Author

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.

@xabbuh
Copy link
Member

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

Let's merge to 4.4. 👍🏻

@rodnaph rodnaph changed the base branch from 6.1 to 4.4 December 15, 2021 23:01
… 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
Copy link
Contributor Author

rodnaph commented Dec 15, 2021

Rebased on 4.4, with suggested updates.

@fancyweb fancyweb changed the title [Validator] Reset Validator Locale [Validator] Restore default locale in ConstraintValidatorTestCase Dec 17, 2021
@fancyweb
Copy link
Contributor

Thank you @rodnaph.

@fancyweb fancyweb merged commit f91c40a into symfony:4.4 Dec 17, 2021
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.

7 participants