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] Deprecate use of `Locale` validation constraint without setting "canonicalize" option to `true` #26075

Merged
merged 1 commit into from Feb 20, 2018

Conversation

Projects
None yet
6 participants
@phansys
Contributor

phansys commented Feb 7, 2018

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).

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);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 11, 2018

Member

The .... Set it to true instead.

This comment has been minimized.

@phansys

phansys Feb 11, 2018

Contributor

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

@@ -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);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 11, 2018

Member

unrelated, should be reverted to ease merger's job

);
}
/**
* @dataProvider getInvalidLocales
*/
public function testInvalidLocales($locale)
public function testInvalidLocales(string $locale)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 11, 2018

Member

should be reverted to ease merger's job

@phansys

This comment has been minimized.

Contributor

phansys commented Feb 11, 2018

Comments addressed.

@phansys

This comment has been minimized.

Contributor

phansys commented Feb 12, 2018

@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

This comment has been minimized.

Member

nicolas-grekas commented Feb 12, 2018

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

This comment has been minimized.

Contributor

phansys commented Feb 12, 2018

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

This comment has been minimized.

Contributor

phansys commented Feb 16, 2018

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

This comment has been minimized.

Member

nicolas-grekas commented Feb 19, 2018

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.

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);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 19, 2018

Member

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?

This comment has been minimized.

@phansys

phansys Feb 19, 2018

Contributor

Sure! Done ;)

@fabpot

fabpot approved these changes Feb 20, 2018

made some minor comments

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);

This comment has been minimized.

@fabpot

fabpot Feb 20, 2018

Member

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.

This comment has been minimized.

@phansys

phansys Feb 20, 2018

Contributor

Message updated. Thank you.

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

This comment has been minimized.

@fabpot

fabpot Feb 20, 2018

Member

Don't forget to change those expectations.

@fabpot

fabpot approved these changes Feb 20, 2018

after the double-dots are removed

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);

This comment has been minimized.

@fabpot

fabpot Feb 20, 2018

Member

double-dot :)

This comment has been minimized.

@phansys

phansys Feb 20, 2018

Contributor

Sorry. Fixed.

@fabpot

This comment has been minimized.

Member

fabpot commented Feb 20, 2018

Thank you @phansys.

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Feb 20, 2018

feature #26075 [Validator] Deprecate use of `Locale` validation const…
…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`

@phansys phansys deleted the phansys:locale_validator branch Feb 20, 2018

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Feb 23, 2018

minor #9248 [Validator] Add docs for "canonicalize" option at `Locale…
…` 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

This comment has been minimized.

Contributor

emodric commented Apr 4, 2018

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

This comment has been minimized.

Member

stof commented Apr 4, 2018

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

This comment has been minimized.

Contributor

phansys commented Apr 4, 2018

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

This comment has been minimized.

Contributor

emodric commented Apr 4, 2018

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

@stof

This comment has been minimized.

Member

stof commented Apr 4, 2018

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

@phansys

This comment has been minimized.

Contributor

phansys commented Apr 4, 2018

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

This comment has been minimized.

Contributor

emodric commented Apr 4, 2018

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

This comment has been minimized.

Contributor

phansys commented Apr 4, 2018

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

This comment has been minimized.

Contributor

emodric commented Apr 4, 2018

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

Thanks for your help!

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment