Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Validator] Deprecate use ofLocale validation constraint without setting "canonicalize" option totrue#26075

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

Merged
fabpot merged 1 commit intosymfony:masterfromphansys:locale_validator
Feb 20, 2018

Conversation

@phansys
Copy link
Contributor

@phansysphansys commentedFeb 7, 2018•
edited
Loading

QA
Branchmaster
Bug fix?no
New feature?no
BC breaks?no
Deprecations?yes
Tests pass?yes
Fixed tickets#22353
LicenseMIT
Doc PRsymfony/symfony-docs#9248

See#22353 (comment).

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

Choose a reason for hiding this comment

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

The .... Set it totrue instead.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

{
if (!$constraintinstanceof Locale) {
thrownewUnexpectedTypeException($constraint,__NAMESPACE__.'\Locale');
thrownewUnexpectedTypeException($constraint, Locale::class);

Choose a reason for hiding this comment

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

unrelated, should be reverted to ease merger's job

* @dataProvider getInvalidLocales
*/
publicfunctiontestInvalidLocales($locale)
publicfunctiontestInvalidLocales(string$locale)

Choose a reason for hiding this comment

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

should be reverted to ease merger's job

@phansysphansysforce-pushed thelocale_validator branch 2 times, most recently from3d1d2d2 tod465dbdCompareFebruary 11, 2018 12:12
@phansys
Copy link
ContributorAuthor

Comments addressed.

@phansys
Copy link
ContributorAuthor

@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
Copy link
Member

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

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

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

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.

phansys reacted with thumbs up emoji

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

Choose a reason for hiding this comment

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

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?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Sure! Done ;)

Copy link
Member

@fabpotfabpot left a comment

Choose a reason for hiding this comment

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

made some minor comments

publicfunction__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);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Message updated. Thank you.

publicfunctiontestNullIsValid()
/**
* @group legacy
* @expectedDeprecation The "canonicalize" option with value `false` is deprecated since Symfony 4.1. Set it to `true` instead.
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to change those expectations.

Copy link
Member

@fabpotfabpot left a comment

Choose a reason for hiding this comment

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

after the double-dots are removed

publicfunction__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);
Copy link
Member

Choose a reason for hiding this comment

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

double-dot :)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Sorry. Fixed.

@fabpot
Copy link
Member

Thank you@phansys.

phansys reacted with thumbs up emoji

@fabpotfabpot merged commit1572540 intosymfony:masterFeb 20, 2018
fabpot added a commit that referenced this pull requestFeb 20, 2018
…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`
@phansysphansys deleted the locale_validator branchFebruary 20, 2018 19:53
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull requestFeb 23, 2018
…` validation constraint (phansys, javiereguiluz)This PR was merged into the master branch.Discussion----------[Validator] Add docs for "canonicalize" option at `Locale` validation constraintRelated tosymfony/symfony#22353,symfony/symfony#26075.Closes#7660.Commits-------14f10bc Final changes4c28e1f Minor changes0cc4ee8 Add docs for "canonicalize" option at `Locale` validation constraint
@emodric
Copy link
Contributor

Maybe I'm missing something, but how can settingcanonicalize tofalse 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
Copy link
Member

stof commentedApr 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
Copy link
ContributorAuthor

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

@emodric
Copy link
Contributor

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

@stof
Copy link
Member

stof commentedApr 4, 2018

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

@phansys
Copy link
ContributorAuthor

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

@emodric
Copy link
Contributor

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

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

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

Thanks for your help!

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

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@fabpotfabpotfabpot approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

6 participants

@phansys@nicolas-grekas@fabpot@emodric@stof@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp