Skip to content

[Validator] Support logical constraints/expression language #5749

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

Closed
webmozart opened this issue Oct 14, 2012 · 34 comments
Closed

[Validator] Support logical constraints/expression language #5749

webmozart opened this issue Oct 14, 2012 · 34 comments

Comments

@webmozart
Copy link
Contributor

I would like to see logical constraints or an expression language to be supported by the validator.

<?php
class Entity
{
    /** @Or({ @Equals(10), @LessThan(10) }) */
    public $property;
}

The problem here is that "and", "or" and "xor" are PHP keywords, so we need to find a different name for these constraints:

  • or:
    • LOr (logical or)
    • AnyOf
    • Any
    • ... ?
  • and
    • LAnd
    • AllOf
    • All (not BC! redefines current All constraint)
    • ... ?
  • xor
    • LXor
    • EitherOf
    • Either
    • ... ?
  • not
    • Not
    • LNot
    • ... ?

An alternative approach is to introduce an expression language:

<?php
class Entity
{
    Alternative 1:
    /** @Assert\Expr({@Assert\Min(10), "or", @Assert\Type("integer")}) */
    public $property1;

    Alternative 2:
    /** @Assert\Expr("@Assert\Min(10) or @Assert\Type('integer')") */
    public $property2;
}

I don't know if Alternative 2 is feasible at all, because the annotation parser would have to be run on the expression with the scope of the file where the expression is defined. This is impossible to determine (the expression could be assembled dynamically and used to construct an Expr constraint at runtime).

P.S.: If "Any" and "All" are our best guesses, a solution would be to only introduce "Any" for now, deprecate "All" and

  1. merge it into "Collection", or
  2. introduce a new name "Each"

Once the deprecation phase of "All" is over (2.3?) we can remove it and introduce a new "All" constraint with the "and" semantics.

<?php
class Entity
{
    Alternative 1:
    /**
     * @Assert\Collection(each = {
     *     @Assert\Min(10),
     *     @Assert\Type("integer"),
     * })
     */
    public $property1;

    Alternative 2:
    /**
     * @Assert\Each({
     *     @Assert\Min(10),
     *     @Assert\Type("integer"),
     * })
     */
    public $property2;
}
@jfsimon
Copy link
Contributor

jfsimon commented Oct 14, 2012

+1 for the @Assert\Each and @Assert\Any syntax.
We could also have @Assert\None and @Assert\OnlyOne.

@stof
Copy link
Member

stof commented Oct 14, 2012

@bschussek the alternative 2 for the expression cannot be used, unless you write your own annotation parser instead of the Doctrine one (and it would then cause issues if you apply the doctrine parser on the same class). So -1 for it.

I'm also 👍 for Each and Any.

@wouterj
Copy link
Member

wouterj commented Oct 14, 2012

👍 Each and Any.

@marijn
Copy link

marijn commented Oct 14, 2012

Each and Any 👍

@gunnarlium
Copy link
Contributor

How about applying something like Mongo does? $or, $and, etc. Maybe with another sign than $, if $ causes problems with PHP.

@marijn
Copy link

marijn commented Oct 14, 2012

How about applying something like Mongo does? $or, $and, etc. Maybe with another sign than $, if $ causes problems with PHP.

I'm not sure I understand your suggestion. Why are you proposing to do so, other than the fact that Mongo does it?

@webmozart
Copy link
Contributor Author

Using Each and Any instead of And and Or was not quite my suggestion, but I like it anyway :)

So that would be

<?php

AND semantics:
/** @Assert\Each({ @Foo, @Bar, @Baz }) */

OR semantics:
/** @Assert\Any({ @Foo, @Bar, @Baz }) */

XOR semantics:
/** @Assert\ExactlyOne({ @Foo, @Bar, @Baz }) */

optional: NOT semantics?
/** @Assert\Not(@Foo) */

The only problem I see with these names is that they are not very intuitive. If I want AND semantics, I first have to think about how AND is called in the Validator.

@gunnarlium The only special character allowed at the beginning of class names is underscore. I'm not a fan of "_And", "_Or" etc.

@KrzysztofZalasa
Copy link

Maybe we can use prefixes like:
BoolAnd
BoolOr
BoolXor
etc.

@wouterj
Copy link
Member

wouterj commented Oct 15, 2012

@KrzysztofZalasa I don't like the Bool prefix. And, Or, Xor are not booleans. If you want a prefix, use something like Logical or Logic or L (as @bschussek suggests):

LogicAnd
LogicOr
LogicXor

LogicalAnd
LogicalOr
LogicalXor

LAnd
LOr
LXor

@webmozart
Copy link
Contributor Author

I tend to agree that I prefer "L" prefix, simply because the meaning of the operators is not lost ("and" vs. "each"):

<?php

AND semantics:
/** @Assert\LAnd({ @Foo, @Bar, @Baz }) */

OR semantics:
/** @Assert\LOr({ @Foo, @Bar, @Baz }) */

XOR semantics:
/** @Assert\LXor({ @Foo, @Bar, @Baz }) */

optional: NOT semantics?
/** @Assert\Not(@Foo) */

@haswalt
Copy link
Contributor

haswalt commented Oct 15, 2012

I feel that Logical makes the most sense as a prefix.

This keeps the naming clean and obvious and describes properly what they are. If you want to use LOr you could always alias this in your use.

@marijn
Copy link

marijn commented Oct 15, 2012

I agree with Harry: people can configure an alias themselves.

@webmozart
Copy link
Contributor Author

@marijn That argument is void if the default name is so bad that everyone aliases it. We need a good default.

@haswalt
Copy link
Contributor

haswalt commented Oct 15, 2012

I don't think it's a large use case that involves aliasing it. My point was that naming should be clear and Logical makes the most sense since that's what they are.

My point about alias is one that to prevent the search for a shorthand that a few people like the focus should be for a name that makes it easy to understand what they do.

A new programmer picking up LXor may think this a different operator or misunderstand its just a Xor altogether and think it something completely different.

Same applies for Each and Any, although nice don't convey fully what they do.

I think also prefixing with something like $ is a bad move. We already have a syntax in place, lets not muddy the water by adding new ones.

On 15 Oct 2012, at 18:12, "Bernhard Schussek" <notifications@github.commailto:notifications@github.com> wrote:

@marijnhttps://github.com/marijn That argument is void if the default name is so bad that everyone aliases it. We need a good default.


Reply to this email directly or view it on GitHubhttps://github.com//issues/5749#issuecomment-9452893.

@KrzysztofZalasa
Copy link

@wouterj you are right, Logical prefix is much better, it would be coherently with PHP manual.

I don't like short "L" prefix, because Validator is independent component and using it in non Symfony projects should be easy.

@marijn
Copy link

marijn commented Oct 15, 2012

For the same reasons I still agree with Harry. Naming should be clear and disambiguous

@KrzysztofZalasa
Copy link

I can see no difference between

/** @Assert\LogicalAnd({ @Assert\Foo, @Assert\Bar, @Assert\Baz }) */

and

/** 
* @Assert\Foo
* @Assert\Bar
* @Assert\Baz
 */

Any practical use case of LogicalAnd?

@webmozart
Copy link
Contributor Author

@KrzysztofZalasa There is none. LogicalAnd is needed for composite expressions.

/**
 * @Assert\LogicalOr({
 *     @Assert\Foo,
 *     @Assert\LogicalAnd({
 *         @Assert\Bar,
 *         @Assert\Baz,
 *     })
 * })
 */

@KrzysztofZalasa
Copy link

@bschussek "composite expressions" looks like programing in annotations :) I prefer write new validator for complex cases. I can't remind similar use case in Java or C#, but maybe someone can do this.

@webmozart
Copy link
Contributor Author

@KrzysztofZalasa This is basically the question behind all of this ticket. Do we need logical expressions? PR #790 suggests that yes.

If we support them, we should support them completely (=boolean operators + xor for convenience reasons), not just some arbitrary subset.

@schmittjoh
Copy link
Contributor

You could also use an expression language that is not so heavily tied to annotations:

@Assert\Expr("isFoo() or (isBar() and isBaz())")

Security has a similar expression language which is very powerful. However, given how many constraints we already have in the validator, we would need to come up with a straightforward mapping of expression function to constraint class if we don't want to create an entirely independent new approach to validation.

Another alternative is that we simply say that you need to create your own validator if you need such complex constraints, which also might make sense because it then allows to re-use them.

@stof
Copy link
Member

stof commented Oct 16, 2012

@schmittjoh to be exact, the Security componet does not have it. JMSSecurityExtraBundle does (and btw, why is it not in core, given it is more powerful and more efficient than the core system ?).

@mvrhov
Copy link

mvrhov commented Oct 16, 2012

@stof I'm going to guess that's because if you want to use the expressions in annotations, then you also need DiExtra schmittjoh/JMSSecurityExtraBundle#80

@stof
Copy link
Member

stof commented Oct 16, 2012

@mvrhov Expression-based checks and annotation-based checks are 2 different topics. You can use expressions without the annotations, and you can use annotations to check based on roles instead of expressions. DIExtraBundle is used for annotation-based checks where generating a proxy is needed

@webmozart
Copy link
Contributor Author

Please move offtopic discussions to the ML ;)

@KrzysztofZalasa
Copy link

@bschussek at the first sight it seems to be useful and it looks great, but after reading your last example I think, we shouldn't do it in core. Annotations contains metadata so it will be better to avoid programming in it like Java, C#, Action Script do it.

-1 to logical constraints or an expression language in core

@webmozart
Copy link
Contributor Author

@KrzysztofZalasa Have you read the related ticket? This ticket is the generalization of the need for a GreaterThanOrEquals constraint.

@KrzysztofZalasa
Copy link

@bschussek I have read #790 , GreaterThanOrEquals can be done with something like Range validator (without data type check). When or is needed it looks like new validator should be written - it is easy and much better in reading metadata in annotations.

@webmozart
Copy link
Contributor Author

I don't have the feeling that this is really needed for now nor that we know how to implement this, so I'll close this ticket.

@bitraft
Copy link

bitraft commented Sep 23, 2013

I suggestion to add alias or map support for Doctrine annotation, for example:

<?php
namespace App\AdminBundle\Admin\Annotation ;

/**
 * @Annotation(alials="Or")
 * @Target({"PROPERTY"})
 */
class OrExpression
{
    /** @var string */
    public $name ;
}

after that, you can use it like this:

<?php
use App\AdminBundle\Admin\Annotation ;
class Entity
{
    /** @Or( .... ) */
    public $property;
}

@mvrhov
Copy link

mvrhov commented Sep 23, 2013

IMO with the inclusion of Expression language component into the symfony stack this whole RFC should be reconsidered.

@hacfi
Copy link
Contributor

hacfi commented Sep 23, 2013

@changloong I don’t understand what you want to do with Doctrine here. Can you explain it a bit more please?

@webmozart
Copy link
Contributor Author

@mvrhov Yes. With the addition of the new Expression constraint in #8913, this is in fact solved.

@stof
Copy link
Member

stof commented Sep 24, 2013

@bschussek not exactly. You cannot do an OR between several constraints as expressions cannot reuse constraints. the only case which can be replaced is an OR between 2 True constraints on methods

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

No branches or pull requests