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] Expression language allow null#41329

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

Closed

Conversation

mpiot
Copy link
Contributor

@mpiotmpiot commentedMay 20, 2021•
edited by derrabus
Loading

QA
Branch?5.4
Bug fix?yes
New feature?no
Deprecations?yes
Tickets
LicenseMIT
Doc PR

Like other validators, ExpressionLanguageSyntax validator should be ignored when the value is null or receive an empty string, add NotNull or NotBlank if needed.

@mpiotmpiotforce-pushed theexpression-language-allow-null branch from16c2086 to44c0094CompareMay 20, 2021 12:40
@mpiotmpiot changed the title[Constrainst] Expression language allow null[Validator] Expression language allow nullMay 20, 2021
@mpiotmpiotforce-pushed theexpression-language-allow-null branch 3 times, most recently from45769db to18aa137CompareMay 20, 2021 12:49
@derrabusderrabus added this to the5.2 milestoneMay 20, 2021
@carsonbot
Copy link

Hey!

I think@Andrej-in-ua has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@Andrej-in-ua
Copy link
Contributor

Looks like correct fix. Thanks!

@mpiot
Copy link
ContributorAuthor

Thanks for your review 🙂

@derrabus
Copy link
Member

derrabus commentedMay 23, 2021•
edited
Loading

I agree that the validator should behave as you're suggesting. However, changing its behavior is technically a BC break, so we should probably add a config flag and a deprecation layer for your change.

@mpiot
Copy link
ContributorAuthor

mpiot commentedMay 25, 2021•
edited
Loading

@derrabus I've added anallowNullAndEmptyString option, and add a deprecation on its usage for 5.4.
Added some tests to: with and without the option.

For the deprecation I don't really know ho to add it correctly. For the moment just add a Deprecation when usingallowNullAndEmptyString but for the 5.4 version that do not exists, it trigger a Deprecation that we can't solve.

@mpiotmpiotforce-pushed theexpression-language-allow-null branch 2 times, most recently fromdf753f7 to10218cfCompareMay 25, 2021 13:39
Copy link
Member

@derrabusderrabus left a comment

Choose a reason for hiding this comment

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

Sorry for stalling here. I have a comment about the deprecation layer. We should make sure that the deprecation supports our upgrade path to 6.0.

Also, when testing the deprecated behavior, please make sure to add it to the legacy group:

/** * @group legacy */publicfunction testSomeOldBehavior(){

This way, the deprecation will not cause the test to fail and we know which tests can be removed on 6.0.

Comment on lines 45 to 47
if (isset($options['allowNullAndEmptyString'])) {
trigger_deprecation('symfony/validator', '5.4', sprintf('The "allowNullAndEmptyString" option of the "%s" constraint is deprecated.', self::class));
}
Copy link
Member

Choose a reason for hiding this comment

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

This parameter is our forward compatibility layer and it allows us to opt-in to the behavior we want to have in 6.0. An application that does not explicitly enable that new behavior will potentially break when upgrading to Symfony 6.

What we should deprecate is not explicitly setting it totrue.

Suggested change
if (isset($options['allowNullAndEmptyString'])) {
trigger_deprecation('symfony/validator','5.4',sprintf('The"allowNullAndEmptyString"optionof the "%s" constraint is deprecated.',self::class));
}
if (!$this->allowNullAndEmptyString) {
trigger_deprecation('symfony/validator','5.4','Not setting"allowNullAndEmptyString" of the "%s" constraintto "true"is deprecated.',__CLASS__);
}

Copy link
Contributor

@HeahDudeHeahDude left a comment

Choose a reason for hiding this comment

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

Thanks for that PR. What about fixinghttps://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/Validator/Constraints/ExpressionValidator.php#L42 as well, could be done in the same PR I guess.

$this->allowNullAndEmptyString = $allowNullAndEmptyString ?? $this->allowNullAndEmptyString;

if (!$this->allowNullAndEmptyString) {
trigger_deprecation('symfony/validator', '5.4', 'Not setting "allowNullAndEmptyString" of the "%s" constraint to "true" is deprecated.', __CLASS__);
Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to be more explicit on the upgrade path here, something likeValidating empty expressions with "%s" constraint is deprecated. Set "allowNullAndEmptyString" option to "true" instead and add explicit constraint like NotNull or NotBlank.'

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@HeahDude I think you're right, I"ve just base the message on other validator that do the same. But be more explicit is maybe better for final user.

@@ -39,6 +39,10 @@ public function validate($expression, Constraint $constraint): void
throw new UnexpectedTypeException($constraint, ExpressionLanguageSyntax::class);
}

if (true === $constraint->allowNullAndEmptyString && (null === $expression || '' === $expression)) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

The deprecation should be triggered here instead of the constraint constructor to ensure runtime context, otherwise it may be lost in the app cache warm up, something like:

if (null ===$expression ||'' ===$expression) {if ($constraint->allowNullAndEmptyString) {return;    }trigger_deprecation(...);}

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I'll wait for other comments about it, because all triggered deprecations in Constraints are in the Constraint class and not in the Validator (eg: Length, Range)

Copy link
Contributor

@HeahDudeHeahDudeAug 27, 2021•
edited
Loading

Choose a reason for hiding this comment

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

I would say that it should never be the case, otherwise it may end up assymfony/symfony-docs#14785 (refsymfony/symfony-docs#14785 (comment)).

@jderusse
Copy link
Member

However, changing its behavior is technically a BC break, so we should probably add a config flag and a deprecation layer for your change.

I disagree with this.

This validator is theonly one that throws an exception when the value is null:

IMHO, because of such inconsistency with the rest of the code, this is a bug and could not be an expected behavior.

@derrabus
Copy link
Member

IMHO, because of such inconsistency with the rest of the code, this is a bug and could not be an expected behavior.

I agree with that assessment. However, it's the current behavior of stable Symfony releases, even if it's an unexpected one. If we simply change it, we might break existing validation logic of apps.

We've had a similar situation with theLengthValidator where it did not no-op on empty strings like all other validators do. This has been solved in a simlar way.

HeahDude and OskarStark reacted with thumbs up emoji

@fabpot
Copy link
Member

Would be great to have this in 5.4.

pille1842 and OskarStark reacted with thumbs up emoji

@fabpotfabpot modified the milestones:5.4,6.1Nov 4, 2021
@mpiotmpiotforce-pushed theexpression-language-allow-null branch from7ec63ba to15fac91CompareNovember 15, 2021 07:33
@mpiot
Copy link
ContributorAuthor

I've rebase on 5.4 branch, but change nothing in the code (divergence in the discussion).

Copy link
Member

@jderussejderusse left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Ok for me (just a minor comment about BC),
Then in 6.1 we could deprecate that new param


public function __construct(array $options = null, string $message = null, string $service = null, array $allowedVariables = null, array $groups = null, $payload = null)
public function __construct(array $options = null, string $message = null, string $service = null, array $allowedVariables = null,bool $allowNullAndEmptyString = null,array $groups = null, $payload = null)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we move the parameter at the end for BC? (I wonder if it matters for annotations 🤔)

@fabpot
Copy link
Member

6.0 cannot contain deprecations, any deprecation should be in 6.1 instead.

@mpiot
Copy link
ContributorAuthor

mpiot commentedNov 26, 2021•
edited
Loading

Can we can add the deprecation message in 5.4, and remove it and the alternative logic for 6.0 branch (2nd PR) ?
As version 6 is the same as version 5.4 without deprecations.

@stof
Copy link
Member

it is too late to add new deprecations in 5.4. We are already in RC. the feature freeze was more than 1 month ago.

@nicolas-grekas
Copy link
Member

I think adding argument$allowNullAndEmptyString does not make sense.

Instead of the current approach which is trying to fix a broken behavior ofExpressionLanguageSyntax, I propose to deprecate it and replace it with a new constraint namedExpressionSyntax.

@mpiot up to implement this?

@mpiot
Copy link
ContributorAuthor

mpiot commentedFeb 26, 2022•
edited
Loading

@nicolas-grekas yes, I can create a new validator instead of update this one and deprecate the current validator.

Just a question about the part:
public const EXPRESSION_LANGUAGE_SYNTAX_ERROR = '1766a3f3-ff03-40eb-b053-ab7aa23d988a';

What is the way to “create” this part ? UUID ?

@nicolas-grekas
Copy link
Member

Replaced by#45623

nicolas-grekas added a commit that referenced this pull requestMar 4, 2022
…ntax", use "ExpressionSyntax" instead (mpiot)This PR was merged into the 6.1 branch.Discussion----------[Validator] Deprecate constraint "ExpressionLanguageSyntax", use "ExpressionSyntax" instead| Q             | A| ------------- | ---| Branch?       | 6.1| Bug fix?      | yes| New feature?  | yes| Deprecations? | yes| Tickets       | Fix| License       | MIT| Doc PR        |Like other validators, ExpressionLanguageSyntax validator should be ignored when the value is null or receive an empty string, add NotNull or NotBlank if needed. In#41329 `@nicolas`-grekas proposed to create a new one called `ExpressionSyntax` and deprecate `ExpressionLanguageSyntax` validator.Commits-------ceb94ca [Validator] Deprecate constraint "ExpressionLanguageSyntax", use "ExpressionSyntax" instead
@mpiotmpiot deleted the expression-language-allow-null branchMarch 4, 2022 09:10
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@HeahDudeHeahDudeHeahDude left review comments

@derrabusderrabusderrabus requested changes

@jderussejderussejderusse approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
6.1
Development

Successfully merging this pull request may close these issues.

9 participants
@mpiot@carsonbot@Andrej-in-ua@derrabus@jderusse@fabpot@stof@nicolas-grekas@HeahDude
[8]ページ先頭
©2009-2025 Movatter.jp