[Validator] Deprecate use of `Locale` validation constraint without setting "canonicalize" option to `true` #26075

phansys · 2018-02-07T14:14:49Z

Q	A
Branch	master
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#22353
License	MIT
Doc PR	symfony/symfony-docs#9248

See #22353 (comment).

nicolas-grekas · 2018-02-11T10:37:31Z

src/Symfony/Component/Validator/Constraints/Locale.php

+    public function __construct($options = null)
+    {
+        if (!($options['canonicalize'] ?? false)) {
+            @trigger_error('"canonicalize" option with value `false` is deprecated since Symfony 4.1 and will be removed in 5.0. Use "canonicalize"=>true instead.', E_USER_DEPRECATED);


The .... Set it to true instead.

I took as example the syntax at Email::__construct(). Updated.

nicolas-grekas · 2018-02-11T10:38:08Z

src/Symfony/Component/Validator/Constraints/LocaleValidator.php

@@ -30,7 +30,7 @@ class LocaleValidator extends ConstraintValidator
    public function validate($value, Constraint $constraint)
    {
        if (!$constraint instanceof Locale) {
-            throw new UnexpectedTypeException($constraint, __NAMESPACE__.'\Locale');
+            throw new UnexpectedTypeException($constraint, Locale::class);


unrelated, should be reverted to ease merger's job

nicolas-grekas · 2018-02-11T10:38:25Z

src/Symfony/Component/Validator/Tests/Constraints/LocaleValidatorTest.php

        );
    }

    /**
     * @dataProvider getInvalidLocales
     */
-    public function testInvalidLocales($locale)
+    public function testInvalidLocales(string $locale)


should be reverted to ease merger's job

phansys · 2018-02-11T12:12:32Z

Comments addressed.

phansys · 2018-02-12T15:48:00Z

@fabpot, @nicolas-grekas; should we consider valid a locale including variants? "it_IT_NEDIS_ROJAZ_1901" by instance: http://php.net/manual/en/locale.getallvariants.php

nicolas-grekas · 2018-02-12T16:28:35Z

should we consider valid a locale including variants?

Only if a real world app needs it. Let's not spend time on theoretical improvements.

phansys · 2018-02-12T19:51:08Z

I can't imagine a practical use case using variants right now, but I think we should left a note about the situation in docs, explaining that locales including variants will not be considered valid. WDYT?

phansys · 2018-02-16T02:45:06Z

Do you know how we could avoid having different results per platform? AppVeyor is reading "en-us.utf8@VARIANT" as a valid locale, while in Travis it is invalid. Shouldn't both environments be using the same bundled ICU data?

nicolas-grekas · 2018-02-19T10:46:26Z

Shouldn't both environments be using the same bundled ICU data?

let's make the test case independent from the data version instead, we're not testing the data set here.

nicolas-grekas · 2018-02-19T20:42:27Z

src/Symfony/Component/Validator/Constraints/Locale.php

+    public function __construct($options = null)
+    {
+        if (!($options['canonicalize'] ?? false)) {
+            @trigger_error('The "canonicalize" option with value `false` is deprecated since Symfony 4.1 and will be removed in 5.0. Set it to `true` instead.', E_USER_DEPRECATED);


we just merged a PR that removes all occurences of "and will be removed in 5.0."
would you mind removing them from this PR please?

Sure! Done ;)

fabpot

made some minor comments

fabpot · 2018-02-20T06:37:17Z

src/Symfony/Component/Validator/Constraints/Locale.php

+    public function __construct($options = null)
+    {
+        if (!($options['canonicalize'] ?? false)) {
+            @trigger_error('The "canonicalize" option with value `false` is deprecated since Symfony 4.1. Set it to `true` instead.', E_USER_DEPRECATED);


Instead of backticks, you should use ", and make one sentence;

The "canonicalize" option with value "false" is deprecated since Symfony 4.1, set it to "true" instead.

Message updated. Thank you.

fabpot · 2018-02-20T06:37:38Z

src/Symfony/Component/Validator/Tests/Constraints/LocaleValidatorTest.php

-    public function testNullIsValid()
+    /**
+     * @group legacy
+     * @expectedDeprecation The "canonicalize" option with value `false` is deprecated since Symfony 4.1. Set it to `true` instead.


Don't forget to change those expectations.

fabpot

after the double-dots are removed

fabpot · 2018-02-20T13:21:22Z

src/Symfony/Component/Validator/Constraints/Locale.php

+    public function __construct($options = null)
+    {
+        if (!($options['canonicalize'] ?? false)) {
+            @trigger_error('The "canonicalize" option with value "false" is deprecated since Symfony 4.1, set it to "true" instead..', E_USER_DEPRECATED);


double-dot :)

Sorry. Fixed.

…nicalize" option to `true`

fabpot · 2018-02-20T19:48:35Z

Thank you @phansys.

…raint without setting "canonicalize" option to `true` (phansys) This PR was merged into the 4.1-dev branch. Discussion ---------- [Validator] Deprecate use of `Locale` validation constraint without setting "canonicalize" option to `true` |Q |A | |--- |--- | |Branch |master | |Bug fix? |no | |New feature? |no | |BC breaks? |no | |Deprecations?|yes | |Tests pass? |yes | |Fixed tickets|#22353 | |License |MIT | |Doc PR |symfony/symfony-docs#9248| See #22353 (comment). Commits ------- 1572540 Deprecate use of `Locale` validation constraint without setting "canonicalize" option to `true`

…` validation constraint (phansys, javiereguiluz) This PR was merged into the master branch. Discussion ---------- [Validator] Add docs for "canonicalize" option at `Locale` validation constraint Related to symfony/symfony#22353, symfony/symfony#26075. Closes #7660. Commits ------- 14f10bc Final changes 4c28e1f Minor changes 0cc4ee8 Add docs for "canonicalize" option at `Locale` validation constraint

emodric · 2018-04-04T16:58:15Z

Maybe I'm missing something, but how can setting canonicalize to false be deprecated in 4.1, when that option didn't even exist in 4.0?

https://github.com/symfony/symfony/blob/4.0/src/Symfony/Component/Validator/Constraints/Locale.php
https://github.com/symfony/symfony/blob/4.0/src/Symfony/Component/Validator/Constraints/LocaleValidator.php

stof · 2018-04-04T17:00:36Z

hmm, deprecating a value of the option at the same time it is added looks indeed weird, as it means we cannot write code running both on 4.0 and on 4.1 without deprecation

phansys · 2018-04-04T17:52:05Z

Because the deprecation is not about using the option itself, but about passing false as value for it (which is allowed to keep BC with the default behavior present until 4.1).

emodric · 2018-04-04T18:04:06Z

@phansys But the option didn't exist before 4.1, therefore you couldn't even pass false to it?

stof · 2018-04-04T18:07:26Z

@emodric but omitting it is equivalent to passing false as this is the default in 4.x (for BC reasons)

phansys · 2018-04-04T18:07:55Z

Right @emodric, this option will be introduced in 4.1, and the deprecation will be thrown if you omit it or if you pass false as value for it.

emodric · 2018-04-04T18:11:51Z

Okay, I understand, but:

as it means we cannot write code running both on 4.0 and on 4.1 without deprecation

How to solve this case that @stof mentions, or should we check for kernel version to avoid the deprecation?

phansys · 2018-04-04T18:49:44Z

Except a build matrix executing same build for both versions, I can't imagine which scenario could require avoid the deprecation without changing the codebase triggering it. But I think there is no way to use this validator without deprecation in ^4.1 if you don't follow the upgrade path. ^4.0.0 will not trigger any deprecation at all, since this feature will not be backported at all (AFAIK).

emodric · 2018-04-04T20:35:36Z

Build matrix discovered this deprecation for me, yes. No worries, I will add a @legacy annotation to the test :)

Thanks for your help!

carsonbot added Status: Needs Review Validator Deprecation labels Feb 7, 2018

phansys force-pushed the locale_validator branch from b9af78c to 2229cec Compare February 7, 2018 14:16

nicolas-grekas added this to the 4.1 milestone Feb 11, 2018

nicolas-grekas reviewed Feb 11, 2018

View reviewed changes

phansys force-pushed the locale_validator branch 2 times, most recently from 3d1d2d2 to d465dbd Compare February 11, 2018 12:12

carsonbot removed the Status: Needs Review label Feb 11, 2018

nicolas-grekas approved these changes Feb 11, 2018

View reviewed changes

carsonbot added the Status: Reviewed label Feb 11, 2018

phansys mentioned this pull request Feb 12, 2018

[Validator] Add docs for "canonicalize" option at Locale validation constraint symfony/symfony-docs#9248

Merged

phansys force-pushed the locale_validator branch from d465dbd to e5c4c2c Compare February 12, 2018 13:02

phansys force-pushed the locale_validator branch from e5c4c2c to 3e3d565 Compare February 15, 2018 15:37

phansys force-pushed the locale_validator branch from 3e3d565 to 7a47aef Compare February 19, 2018 16:18

nicolas-grekas reviewed Feb 19, 2018

View reviewed changes

phansys force-pushed the locale_validator branch from 7a47aef to 8a8d1ec Compare February 19, 2018 20:51

fabpot approved these changes Feb 20, 2018

View reviewed changes

phansys force-pushed the locale_validator branch from 8a8d1ec to 4f08f20 Compare February 20, 2018 09:33

fabpot approved these changes Feb 20, 2018

View reviewed changes

Deprecate use of Locale validation constraint without setting "cano…

1572540

…nicalize" option to `true`

phansys force-pushed the locale_validator branch from 4f08f20 to 1572540 Compare February 20, 2018 14:34

fabpot merged commit 1572540 into symfony:master Feb 20, 2018

phansys deleted the locale_validator branch February 20, 2018 19:53

javiereguiluz mentioned this pull request Mar 20, 2018

[Validator] Deprecate use of Locale validation constraint without setting "canonicalize" option to true symfony/symfony-docs#9453

Closed

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

wouterj mentioned this pull request Feb 9, 2020

Add Canonicalize option to Locale Constraint symfony/symfony-docs#12984

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Validator] Deprecate use of `Locale` validation constraint without setting "canonicalize" option to `true` #26075

[Validator] Deprecate use of `Locale` validation constraint without setting "canonicalize" option to `true` #26075

phansys commented Feb 7, 2018 •

edited

Loading

nicolas-grekas Feb 11, 2018

phansys Feb 11, 2018

nicolas-grekas Feb 11, 2018

nicolas-grekas Feb 11, 2018

phansys commented Feb 11, 2018

phansys commented Feb 12, 2018

nicolas-grekas commented Feb 12, 2018

phansys commented Feb 12, 2018

phansys commented Feb 16, 2018

nicolas-grekas commented Feb 19, 2018

nicolas-grekas Feb 19, 2018

phansys Feb 19, 2018

fabpot left a comment

fabpot Feb 20, 2018

phansys Feb 20, 2018

fabpot Feb 20, 2018

fabpot left a comment

fabpot Feb 20, 2018

phansys Feb 20, 2018

fabpot commented Feb 20, 2018

emodric commented Apr 4, 2018

stof commented Apr 4, 2018

phansys commented Apr 4, 2018

emodric commented Apr 4, 2018

stof commented Apr 4, 2018

phansys commented Apr 4, 2018

emodric commented Apr 4, 2018

phansys commented Apr 4, 2018

emodric commented Apr 4, 2018

[Validator] Deprecate use of Locale validation constraint without setting "canonicalize" option to true #26075

[Validator] Deprecate use of Locale validation constraint without setting "canonicalize" option to true #26075

Conversation

phansys commented Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phansys commented Feb 11, 2018

phansys commented Feb 12, 2018

nicolas-grekas commented Feb 12, 2018

phansys commented Feb 12, 2018

phansys commented Feb 16, 2018

nicolas-grekas commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Feb 20, 2018

emodric commented Apr 4, 2018

stof commented Apr 4, 2018

phansys commented Apr 4, 2018

emodric commented Apr 4, 2018

stof commented Apr 4, 2018

phansys commented Apr 4, 2018

emodric commented Apr 4, 2018

phansys commented Apr 4, 2018

emodric commented Apr 4, 2018

[Validator] Deprecate use of `Locale` validation constraint without setting "canonicalize" option to `true` #26075

[Validator] Deprecate use of `Locale` validation constraint without setting "canonicalize" option to `true` #26075

phansys commented Feb 7, 2018 •

edited

Loading