[Validator] Expression language allow null #41329

mpiot · 2021-05-20T12:39:34Z

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	yes
Tickets
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.

carsonbot · 2021-05-21T08:44:38Z

Hey!

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

Cheers!

Carsonbot

Andrej-in-ua · 2021-05-21T09:29:11Z

Looks like correct fix. Thanks!

mpiot · 2021-05-23T10:03:54Z

Thanks for your review 🙂

derrabus · 2021-05-23T19:48:42Z

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 · 2021-05-25T09:01:47Z

@derrabus I've added an allowNullAndEmptyString 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 using allowNullAndEmptyString but for the 5.4 version that do not exists, it trigger a Deprecation that we can't solve.

derrabus

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
 */
public function testSomeOldBehavior()
{

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

derrabus · 2021-06-23T08:42:27Z

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

+        if (isset($options['allowNullAndEmptyString'])) {
+            trigger_deprecation('symfony/validator', '5.4', sprintf('The "allowNullAndEmptyString" option of the "%s" constraint is deprecated.', self::class));
+        }


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 to true.

Suggested change

if (isset($options['allowNullAndEmptyString'])) {

trigger_deprecation('symfony/validator', '5.4', sprintf('The "allowNullAndEmptyString" option of the "%s" constraint is deprecated.', self::class));

}

if (!$this->allowNullAndEmptyString) {

trigger_deprecation('symfony/validator', '5.4', 'Not setting "allowNullAndEmptyString" of the "%s" constraint to "true" is deprecated.', __CLASS__);

}

HeahDude

Thanks for that PR. What about fixing https://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.

HeahDude · 2021-06-28T09:57:13Z

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

+        $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__);


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

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

HeahDude · 2021-06-28T10:08:13Z

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

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


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(...); }

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)

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

jderusse · 2021-06-30T08:25:41Z

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 the only 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 · 2021-06-30T10:11:37Z

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 the LengthValidator where it did not no-op on empty strings like all other validators do. This has been solved in a simlar way.

fabpot · 2021-10-30T09:45:17Z

Would be great to have this in 5.4.

mpiot · 2021-11-15T07:37:27Z

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

jderusse

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

jderusse · 2021-11-22T06:56:34Z

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


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


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

fabpot · 2021-11-22T11:18:32Z

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

mpiot · 2021-11-26T13:56:32Z

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 · 2021-11-26T14:18:26Z

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 · 2022-02-26T09:17:28Z

I think adding argument $allowNullAndEmptyString does not make sense.

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

@mpiot up to implement this?

mpiot · 2022-02-26T13:04:26Z

@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 · 2022-03-04T08:46:53Z

Replaced by #45623

…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

carsonbot added Status: Needs Review Bug labels May 20, 2021

mpiot force-pushed the expression-language-allow-null branch from 16c2086 to 44c0094 Compare May 20, 2021 12:40

mpiot changed the title ~~[Constrainst] Expression language allow null~~ [Validator] Expression language allow null May 20, 2021

mpiot force-pushed the expression-language-allow-null branch 3 times, most recently from 45769db to 18aa137 Compare May 20, 2021 12:49

derrabus added this to the 5.2 milestone May 20, 2021

derrabus added the Validator label May 20, 2021

mpiot force-pushed the expression-language-allow-null branch 2 times, most recently from df753f7 to 10218cf Compare May 25, 2021 13:39

derrabus requested changes Jun 23, 2021

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jun 23, 2021

derrabus modified the milestones: 5.2, 5.4 Jun 23, 2021

derrabus added the Deprecation label Jun 23, 2021

HeahDude reviewed Jun 28, 2021

View reviewed changes

HeahDude mentioned this pull request Jun 29, 2021

[Validator] Ignore validation of ExpressionLanguageSyntax against empty string #41904

Closed

xabbuh mentioned this pull request Oct 29, 2021

ExpressionLanguageSyntax constraint doesn't allow null value #43826

Closed

fabpot modified the milestones: 5.4, 6.1 Nov 4, 2021

mpiot added 4 commits November 15, 2021 08:31

[Constrainst] Expression language allow null

470b9fa

Add allowNullAndEmptyString option

984d86b

Add test groups and change deprecation message + trigger

1af79b7

More explicit deprecation message

15fac91

mpiot force-pushed the expression-language-allow-null branch from 7ec63ba to 15fac91 Compare November 15, 2021 07:33

jderusse approved these changes Nov 22, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Nov 22, 2021

mpiot mentioned this pull request Mar 3, 2022

[Validator] Deprecate constraint "ExpressionLanguageSyntax", use "ExpressionSyntax" instead #45623

Merged

nicolas-grekas closed this Mar 4, 2022

mpiot deleted the expression-language-allow-null branch March 4, 2022 09:10


		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)

Uh oh!

[Validator] Expression language allow null #41329

[Validator] Expression language allow null #41329

Uh oh!

Conversation

mpiot commented May 20, 2021 • edited by derrabus Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carsonbot commented May 21, 2021

Uh oh!

Andrej-in-ua commented May 21, 2021

Uh oh!

mpiot commented May 23, 2021

Uh oh!

derrabus commented May 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpiot commented May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derrabus left a comment

Choose a reason for hiding this comment

Uh oh!

derrabus Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

HeahDude left a comment

Choose a reason for hiding this comment

Uh oh!

HeahDude Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

mpiot Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

HeahDude Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

mpiot Jun 29, 2021

Choose a reason for hiding this comment

Uh oh!

HeahDude Aug 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jderusse commented Jun 30, 2021

Uh oh!

derrabus commented Jun 30, 2021

Uh oh!

fabpot commented Oct 30, 2021

Uh oh!

mpiot commented Nov 15, 2021

Uh oh!

jderusse left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jderusse Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

fabpot commented Nov 22, 2021

Uh oh!

mpiot commented Nov 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stof commented Nov 26, 2021

Uh oh!

nicolas-grekas commented Feb 26, 2022

Uh oh!

mpiot commented Feb 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Mar 4, 2022

Uh oh!

Uh oh!

mpiot commented May 20, 2021 •

edited by derrabus

Loading

derrabus commented May 23, 2021 •

edited

Loading

mpiot commented May 25, 2021 •

edited

Loading

HeahDude Aug 27, 2021 •

edited

Loading

jderusse left a comment •

edited

Loading

mpiot commented Nov 26, 2021 •

edited

Loading

mpiot commented Feb 26, 2022 •

edited

Loading