10000 [Validator] Add "is_valid" function to the Expression constraint by fancyweb · Pull Request #33829 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Validator] Add "is_valid" function to the Expression constraint #33829

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Validator] Add "is_valid" function to the Expression constraint #33829

wants to merge 1 commit into from

Conversation

fancyweb
Copy link
Contributor
@fancyweb fancyweb commented Oct 3, 2019 •
edited by javiereguiluz
Loading
Q A
Branch? 4.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #11940
License MIT
Doc PR TODO

The main benefit of this feature is to be able to support "or" constraints but it generally open the usage of this constraint to many cool cases.

is_valid accepts an unlimited number of arguments of 3 kinds:

  1. Constraints (resolved in any way)
/**
 * @Expression("is_valid(a) or is_valid(b)", values={"a": @Type('string'), "b": @Type('int')})
 */
/**
 * @Expression("is_valid(this.method(), a)", values={"a": @Type('string')})
 */

// ...
public function method() { return new IdenticalToo('foo'); }
  1. Array of constraints (resolved in any way)
/**
 * @Expression("is_valid(a) or ...", values={"a": {@Type('int'), @GreaterThan(10)}})
 */
  1. Strings that represents properties when validating an object
/**
 * @Expression("not is_valid("foo") or not is_valid("bar")})
 */

// ...
public $foo;
public $bar;
8000

@fancyweb fancyweb force-pushed the validator-expression-is-valid branch from 69ecdc4 to 324a374 Compare October 3, 2019 11:42
@yceruto yceruto mentioned this pull request Oct 3, 2019
Merged
@yceruto yceruto added this to the next milestone Oct 3, 2019
yceruto
yceruto requested changes Oct 3, 2019
View reviewed changes
src/Symfony/Component/Validator/Constraints/ExpressionValidator.php
return;
}

$this->expressionLanguage->register('is_valid', function () {
Copy link
Member
@yceruto yceruto Oct 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This code should be moved to a dedicated class ExpressionLanguageProvider by implementing ExpressionFunctionProviderInterface and later $this->expressionLanguage->registerProvider(...).

@jvasseur
Copy link
Contributor
jvasseur commented Oct 3, 2019 •
edited
Loading

The API feals really strange, I would expect that calling is_valid(a) will test if a is valid instead of expecting a to be a constraint.

We could expose an API similar to the ValidatorInterface::validate function: is_valid(value, constraint(s) = null)

With this API your use cases would become:

/**
 * Constraints:
 * @Expression("is_valid(value, a) or is_valid(value, b)", values={"a": @Type('string'), "b": @Type('int')})
 *
 * Properties:
 * @Expression("not is_valid(value.foo) or not is_valid(value.bar)")
 */

It's a bit more verbose but I think it's a lot more clear what is validated and against which constraints.

@fancyweb
Copy link
Contributor Author
fancyweb commented Oct 3, 2019 •
edited
Loading

is_valid(value, a)

I agree it's more understandable but the first argument will always be value then 😕 And it opens the ability to validate other values of objects (eg: is_valid(this.getFoo(), a). I guess it's not the goal of such a feature.

is_valid(value.foo)

Not possible because the function would get the resolved value, not the property. The passed arg has to be a string. is_valid("this.foo)" could be an alternative.

@jvasseur
Copy link
Contributor
jvasseur commented Oct 3, 2019

And it opens the ability to validate other values of objects (eg: is_valid(this.getFoo(), a). I guess it's not the goal of such a feature.

I don't see why it shouldn't be allowed.

Not possible because the function would get the resolved value

That's what I wanted to do, the function would get the resolved value and validated it. But I guess what you wanted is validate it against the constraints defined in the object (the same way validateProperty works).

Maybe we could allow a is_valid(object, propertyName) form to allow validating properties.

@fancyweb
Copy link
Contributor Author
fancyweb commented Oct 3, 2019 •
edited
Loading

@jvasseur After some more reflexion I think you are right: is_valid(value, ...constraints) looks better for DX, especially since value is already a value already available in the expression. And I think we can actually support both ways thanks to variadic arguments by checking if the first one is an instance of constraint. I drop support for array of constraints -> it can be replaced by the All constraint.

That's what I wanted to do, the function would get the resolved value and validated it

For objects we can but for anything else, there would be no constraints to check against, isn't it?

I guess what you wanted is validate it against the constraints defined in the object (the same way validateProperty works).

Yes!

Maybe we could allow a is_valid(object, propertyName) form to allow validating properties.

Or we can just have another function is_property_valid(object, ...property) with object being optional (so defaulting to the object being validated) by checking the first argument again.

fabpot added a commit that referenced this pull request Oct 3, 2019
@fabpot
bug #33831 [Validator] Fix wrong expression language value (yceruto)
496346c 
This PR was merged into the 4.4 branch.

Discussion
----------

[Validator] Fix wrong expression language value

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

https://github.com/symfony/symfony/blob/766162c4c790cc0b91185963b9c36b842bf2eb49/src/Symfony/Component/Validator/Constraints/ExpressionValidator.php#L28

```php
(new ExpressionValidator($propertyAccessor))->validate($object, $constraint);
```
Based on the previous method signature (4.3 above), that code would result in an exception in 4.4:
```
Call to undefined method Symfony\Component\PropertyAccess\PropertyAccessor::evaluate()
```
Spotted by @fancyweb in #33829 (comment)

Fixed here and updated test case to avoid regression.

Commits
-------

4288f1c Fix wrong expression language value
@fabpot
Copy link
Member
fabpot commented Aug 18, 2020

@fancyweb I see that you changed your mind about how to solve the problem. So you want to update this PR accordingly?

@fancyweb
Copy link
Contributor Author

I need to refocus on this work as I left it "on the side". I'm closing and I will maybe reopen a new PR in the future.

@fancyweb fancyweb closed this Aug 18, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yceruto yceruto yceruto requested changes

Assignees
No one assigned
Labels
Feature Status: Needs Work Validator
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.

[Validator] Validate from within Expression
6 participants
@fancyweb @jvasseur @fabpot @yceruto @nicolas-grekas @carsonbot
0