Skip to content

[Validator] Deprecated comparison constraints #12389

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
wants to merge 1 commit into from

Conversation

webmozart
Copy link
Contributor

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets partially #12300
License MIT
Doc PR -

I deprecated the constraints which can be easily replaced using the Expression constraint. Using just the Expression constraint means less constraints to learn while at the same time having more power, e.g. arithmetic or logical operators.

@weaverryan
Copy link
Member

Because we will be able to show a ton of really nice common examples in the docs (similar to what Bernhard has in the UPGRADE-2.6), I like it. I think it does look simpler if you use the expressions.

@henrikbjorn
Copy link
Contributor

This forces users to use the Expression language, imo this is a bad thing shoving another component down someones throath if they want to do fairly simple validations.

@weaverryan
Copy link
Member

I wasn't thinking about the Validation component as a standalone use-case - that would be an added dependency indeed, no?

Btw, nothing stops us from recommending that Expression language in the FW docs immediately and regardless of this PR.

@javiereguiluz
Copy link
Member

I don't know if other people agree, but don't you find the @Expression(...) annotation too generic?

When I see the @Expression("value > 0") annotation I think: OK, that's an expression, and what? Maybe a name like @Validation(...) could be better?

 /**
  * // Original
  * @GreaterThan(18, message="You must be 18 years old or older.")
  *
  * // Proposals
  * @Expression("value > 18", message="You must be 18 years old or older.")
  * @Validation("value > 18", message="You must be 18 years old or older.")
  */
private $age;

@henrikbjorn
Copy link
Contributor

The Expression evaluation also have some performance penalty. It is not even possible to inject an instance of ExpressionLanguage directly with caching. Also generally against having code places where i can't run checks on it (xml files, anotations etc)

@fabpot
Copy link
Member

fabpot commented Nov 3, 2014

I like the idea of simplifying the learning curve and having less constraints is going in that direction. It also makes constraints based on several comparisons easier (less complexity.)

There is one downside though: It becomes much harder to do static code analysis on an expression.

@javiereguiluz
Copy link
Member

I know that you are discussing about higher level things, but if DX may be considered, I'd like to keep proposing some ideas:

1. What about calling this new constraint simply Assert. This is a very common name for this kind of checks for languages that support them, such as Python and Java.

class Person(object):
    # ...

    @age.setter
    def age(self, age):
        assert age >= 18, "You must be 18 years or older"
        # ...
class Person {
    // ...

    public void setAge(int age) {
        assert age >= 18: "You must be 18 years or older";
        // ...
    }
}

2. I like the behavior of Twig that allows to use positional and/or named arguments in filters and functions. Could we also allow the use of positional arguments for annotations? That would allow us to make them a bit more concise for the common case:

 /**
  * // named
  * @Assert(expression = "value > 18", message="You must be 18 years old or older.")
  *
  * // mixed
  * @Assert("value > 18", message="You must be 18 years old or older.")
  *
  * // positional
  * @Assert("value > 18", "You must be 18 years old or older.")
  */
private $age;

@boekkooi
Copy link
Contributor

I personally really don't like the removal of the 'comparison constraints' for the reason stated by @henrikbjorn and also because I need to learn yet another DSL.

@javiereguiluz Well I really don't get the problem that you are having with @Expression being to generic. Why don't you do something like this (if you need it to be less generic):

use Symfony\Component\Validator\Constraints as Assert;
....
/**
 * @Assert\Expression("value > 18")
 */
private $age;

@javiereguiluz
Copy link
Member

@boekkooi in my opinion, here we are doing validations and asserts, and not expressions. In fact, expressions are just the thing we use to do what we really want to do (validate or assert values). A related example would be to use regular_expressions instead of requirements for the name of the route option.

@webmozart
Copy link
Contributor Author

I think this discussion has derailed a little. This PR is about deprecating existing constraints only, and if there aren't any major concerns, I'm going to merge this into 2.8 as soon as development starts.

It's true that this means you need to use symfony/expression-language if you want to use expression constraints in your code. The same is already true if you want to formulate more complex logical rules (e.g. with and, or) - so we really just change the use cases for which this external dependency is required.

Other discussions belong to separate tickets, namely:

  • Whether or not Expression should be renamed.
  • How to improve ExpressionValidator to allow for caching.

@Tobion
Copy link
Contributor

Tobion commented Apr 22, 2015

I see one major disadvantage. Using all the existing concrete constraints also gives you concrete error messages like "This value should not be null" without configuring it. And they are also translated automatically. Using the expression constraint, this is not possible anymore and you would also need to give error messages explicitly and translate them all manually.

@boekkooi
Copy link
Contributor

👎 I'm totally against this PR since I see no advantage in removing concrete constraints.

Here are some reasons:

Also it would probably destroy bundles like BoekkooiJqueryValidationBundle and FpJsFormValidatorBundle that rely on the concrete constraint.

@nicolas-grekas
Copy link
Member

Before merging and if that should happen, the deprecation process is now a bit more strict than just adding @deprecated tags: all the code base has to be updated to use the new way, legacy tests need to be tagged as such, and runtime notices have to be added.

@SoboLAN
Copy link

SoboLAN commented May 2, 2015

I doubt a vote from a simple user counts, but if it would, then it would be a 👎 from me as well, citing the same reasons listed by @boekkooi above.

If I have some time in the next few days, I'll do some performance benchmarks, to see how it behaves from this point-of-view. Stay tuned :) .

@SoboLAN
Copy link

SoboLAN commented May 3, 2015

OK, I wrote a benchmark and ran the numbers.

You can see the benchmark here: https://github.com/SoboLAN/sf-validator-expression-performance

I ran it 5 times with 7500 iterations each and got these results:

Time with "classic" constraints Time with Expression constraints Performance Loss
2.9 3.252 12.13%
2.904 3.232 11.29%
2.897 3.249 12.15%
2.898 3.228 11.38%
2.905 3.254 12.01%

As a graph:

Results as Graph

The values are expressed in seconds and they seem pretty stable/consistent, so I guess I did something right :) .

Setup:
OS: Windows 7
PHP version: 5.5.19
Symfony version: 2.6.6 (just like it says in composer.json)


As you can see, the code that uses ExpressionLanguage is indeed slower, but I'm actually pleasantly surprised by the small difference (11 % - 12 % is quite low). There may be 2 reasons for this:

1). The expressions are actually quite simple, so easy to parse
2). ExpressionLanguage or ExpressionValidator caches the result of the parsing operation. I'm not at all familiar with its internals, so I really don't know, maybe someone else can clarify it for us.

With this occasion, we have a nice comparison which shows how these constraints will be used if we really deprecate them. See the file here: https://github.com/SoboLAN/sf-validator-expression-performance/blob/master/src/EntityExpression.php

As you can see, it's not quite pretty, especially in the case of False and True. By switching to using only Expression constraint, you lose A LOT semantic value.

@dosten
Copy link
Contributor

dosten commented May 3, 2015

@SoboLAN In fact, ExpressionLanguage caches the result.

@weaverryan
Copy link
Member

I've thought about this again, and I'm 👎. In my experience, people never have much trouble understanding how to use the validation component, so I'm not convinced there's a problem that needs solving. This is probably in part because it's easy just to show them the validation constraints reference page, and they instantly see a big menu of things that can do.

The one thing I do like is that showing them the Expression component teaches them a tool that can be used beyond the "canned" constraints. But we could also accomplish that in the docs - e.g. add an Expression Equivalent row in the constraints table with an example of the @Expression version (e.g. on these pages http://symfony.com/doc/current/reference/constraints/NotNull.html).

Thanks!

@webmozart webmozart closed this May 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants