New Concrete API - Githubissues

henriquemoody commented 9 years ago

Hey, guys! The new Concrete API for Respect\Validation is almost ready then we can release our first major version (see #258). This issue was created only just to explain and discuss about what I'm working on.

There will be no changes on our Fluent API, I took care to never break the agility we have creating validation chains.

First of all I wanna be clear about the comments below, Validation is, IMHO, the best validation library ever written in PHP and I always say, @alganet is a genius for creating such an amazing library! The reason I wanna create a new Concrete API is simple, the requirements changed since the library was created, eventually code rusts and that's the normal cycle of software development.

Problems

All rules depend on `AbstractRule`

Yes, we have the Validatable interface, but let's be honest, we never create a rule from scratch and instead we just extend AbstractRule to create new rules.

That's a problem because all Rules are based on an implementation, if we have to change AbstractRule all rules will be changed as well.

`Validatable` interface has too many methods

That's the reason why we extend AbstractRule instead, this interface has 7 methods and all rule must do the same in some of them.

The methods in Validatable are really necessary to do what Validation does, I fell like all methods where created really by necessity:

assert()
check($input)
getName()
reportError($input, array $relatedExceptions = [])
setName($name)
setTemplate($template)
validate($input)
Decentralized validation

Each rule performs the validation to the input with no intermediate and this is a problem we are facing for more than one year (see #205 and #235).

Since we cannot intercept the input to check if it is or isn't optional, the only way to check optional inputs on all rules we have is changing all rules we have.

Currently the methods assert(), check() and __invoke() accept optional inputs as valid but the validate() method doesn't. Each rule has its own way to validate the input, so it can or cannot accept optional values and interpret whatever input as optional or valid.

NotEmpty rule

That's rule as optional value checking has been a PITA for a long time (see #244, #154, #196, #173). Also the name Empty should imply on using only the PHP empty() language constructor but it doesn't do that.

Nested exceptions

Also a problem. When using check() method it works like a charm, but wen you use assert() with key(), for example, it doesn't work properly (see #179). I've tried several times to fix this issue on the current code-base but the fixes were always a poor workaround. Of course, working with recursion is almost always ugly, but I think we can do better on the new Concrete API.

Nested exception messages

Currently, when using assert() we can just get the full-message as string, we cannot get all messages in other format to format it as we want.

Exception message translation

We must be able to translate the exception messages in a easy way.

Cannot access `Factory` inside rules

Some rules depend on other rules, would be nice if we can use the same factory we already have to create them.

Proposal

I simplified the rules on a single interface with one method only:

interface RuleInterface
{
    public function apply(Context $context);
}

That's all we need, but you may ask me a few questions.

Why it's called `apply()`?

I'm opened to other names, I think it makes sense since we are talking about Rules.

What is the `Context` object?

Currently, a chain of validators use the Composite pattern and that's the only way to make nested validations, but we don't need to use the rules for that. The Context object uses the Composite pattern and every context has its own Rule, validation result and parameters.

The Context object allows us to define parameters to be used on exceptions, change its mode or even define the message to be displayed when an exception is thrown. By default, it defines the input to be used on some rules and also defines isValid, which indicates if the rule was successful applied or not (TRUE is the default value).

Why we don't have a `$input` parameter?

Firstly, because we don't need it every time, some rules just don't use the $input, secondly because Context already has the input and we can get it when we need it.

I cannot use a Rule as a single validation anymore?

That's true. I mean, you can but you have to create a Context object and then check if it isValid or not.

Any way, it will not make a difference if you use the Fluent API.

How about `check()`, `assert()`, `validate()` and `__invoke()` methods?

These methods are available on the Validator class, so you can still use them in the same way you already do it, like:

v::int()->positive()->assert($input);
v::int()->positive()->check($input);
v::int()->positive()->validate($input);

$rule = v::int()->positive();
$rule($input);

The only change I've made to them: the assert() and check() methods don't return any value anymore. On our current API these methods can return just TRUE or throw an exception, so it doesn't make any sense to keep this behavior, IMO.

How do you deal with optional values?

All rules are applied by Context object.

There is an Interface called RuleRequiredInterface and any rule which implements this interface will be applied no matter if the value is optional or not (rules like NotOptional, AllOf, AlwaysInvalid, AlwaysValid). Actually, it was @qrazi's idea to create this interface (see #336).

There is a trait NotOptionalTrait which performs the validation on NotOptional rule and on Context. If the value is optional and the rule doesn't implement RuleRequiredInterface, I don't even apply the rule to the Context.

What else?

Exception messages using Markdown by default
The Factory is now accessible on all Rules by Context object
The RecursiveContextIterator class
Exceptions, Rules and Contexts are created by Factory
PHP 5.6 or higher (HHVM and PHP 7 included), only
Replace {{name}} for {{placeholder}} on exception messages
Full PSR-1, PSR-2, PSR-4 (and PSR-5) support
Show me the code!

Check out the new_api branch.

PS.: Just remember it's a work in progress

Updates:

Result was replaced by Context
isValid() method was removed

henriquemoody commented 9 years ago

I've spoken to @alganet about this new API and he likes it but I would like to know from you guys of @Respect/troubleshooters and all the community.

augustohp commented 9 years ago

I like the idea and the changes on internal design is much needed, I still need to digest the changes and will probably not have the time in the next two weeks.

One thing I would point as a bummer on the current branch is that it trashes the whole history of changes. I would rather pay the price of incrementally refactor the actual code and still have their history of changes.

henriquemoody commented 9 years ago

I totally agree with you.

I created an orphan branch in order to make clean what is in the new API and also makes easy to test only what it has. My plans are to keep the history we have.

alganet commented 9 years ago

I like these changes a lot. One thing is missing though, the callable API. It allows me to decouple validator itself from userland software projects:

class MyUserlandClass
{
    private $validator;
    public function setValidator(callable $validator)
    {
        $this->validator = $validator;
    }
    public function prove()
    {
        call_user_func($this->validator, $this->something);
    }
    // ...
}

$userlandRaw = new MyUserlandClass();
$userlandRaw->setValidator('is_object');
$userlandV = new MyUserlandClass();
$userlandV->setValidator(new \Respect\Validation\Rules\Object());

// Edit sample (see below)
$userlandv = new MyUserlandClass();
$userlandv->setValidator(\Respect\Validation\Validator::object());

I'm not sure how to implement this though, or even if its usage is popular, so I'm ok if it doesn't make it through a final version. (Edit: Perhaps only the Validator class could have the callable interface, that would make things much simpler.)

Changes were trashed when we moved from PSR-1 to PSR-4. They're only visible with --follow on the client side. Not sure if it is possible to see this on GitHub itself.

When we merge this into our main branch, if we add -all or equivalent at some point, many files will appear as moved (because the implementation still shares many lines in common), and then we'll keep the same behavior (deep after-move changes are visible using --follow). Maybe 1.0 would be a good time to rebase this to unify history, but I wouldn't know how to do that.

augustohp commented 9 years ago

Changes were trashed when we moved from PSR-1 to PSR-4. They're only visible with --follow on the client side. Not sure if it is possible to see this on GitHub itself.

I really don't care about GitHub itself, the history of each file is still there: our contributors and when changes were made.

When we merge this into our main branch, if we add -all or equivalent at some point, many files will appear as moved (because the implementation still shares many lines in common), and then we'll keep the same behavior (deep after-move changes are visible using --follow). Maybe 1.0 would be a good time to rebase this to unify history, but I wouldn't know how to do that.

Me neither, that is why I am concerned about it.

henriquemoody commented 9 years ago

One thing is missing though, the callable API

You're right. I just fixed it on Validator class.

When we merge this into our main branch

This branch was created only as a POC, it's an orphan branch because I didn't want any code that was not from the new Concrete API on it.

If you guys take a look on the branch's code you will see there is only the necessary code and the most important rules:

AllOf (as AbstractComposite)
Key (as AbstractRelated)
Match (a better name for Regex, IMO)
Not
NotBlank (see commend on #173)
NotEmpty (see commend on #173)
NotOptional (see commend on #173)

We're not going to merge it, we really need to refactor the current code using the idea of this branch, if you guys accept this proposal, of course. In spite of the Lazy Consensus we've already agreed on, since it's a big change, I will wait more than usual to start changing our code-base until you guys accept that.

Besides, the branch still needs some refactoring, but the idea is clear already.

nelsonsar commented 9 years ago

I'm using this package now and a think this is a huge change and may break the coolest thing that it has: simplicity when using it.

There are design problems. But I think that you can make something less drastic.

First, I would go for the problems that I see you're describing:

1) All rules depending on AbstractRule: Use interface segregation to define new interfaces to it and new features using new interfaces or "helper" classes. Of course, I don't know all rules and never created a new one, so I may be simplist.

2) Error handling: This is a problem I'm facing too and again I don't know all the problems, but, this can be done (maybe?) by creating a new method that do things better or that make more sense to it, avoiding breaking things in a hard way. Something change getIterator method to return a sane output.

I will work in exception part this holiday anyway I'll try to make a PR soon.

henriquemoody commented 9 years ago

The Fluent API will not change at all, just the Concrete API and not too much, so we will still be able to do:

$validator = v::alnum()->length(1,15);
$validator->validate($input); // Or just $validator($input)

Or:

$validator = new v();
$validator->alnum();
$validator->length(1, 15);
$validator->check($input);

Or even:

$validator = new Validator();
$validator->addRule(new Alnum());
$validator->addRule(new Length(1, 15));
$validator->assert($input);

The only think we will not be able to do in the new Concrete API is to use a single rule without the Validator, like:

$alnum = new Alnum();
$alnum->validate($input); // nor check(), assert() or __invoke();

I may be wrong, but the last example above doesn't represent the majority of Respect\Validation use-cases. The problem is that for this specific case we have the more critical problems in our library, which can be summarized to decentralized validation.

For every rule we have to:

Skip validation when value is optional
Validate the input with validate() method
Validate the input with __invoke() method
Throw exception on check() when input is not valid and return true when it is valid
Throw exception on assert() when input is not valid and return true when it is valid
Define and retrieve the rule's name
Be able to define the template message for exception (this changes the state of the Rule itself, which is really bad)

In the way library was designed, almost all methods of the Validatable are necessary to all rules.

Using the new Concrete API, we need the Context object. See how it works:

$result = new Context(new Alnum(), ['input' => $input], new Factory());
$result->applyRule(); // Centralized validation is here
$result->isValid;

Of course there are pros and cons, but that's the way I found to make a centralized validation, don't change the rules' state, allow a rule to define parameters (like exception templates), make nested validations (Composite) and still keep the Fluent API we have.

We can stick with the design we have and fix the exception handling, we can use Template Method to centralize the validations but we will depend even more on AbstractRule.

nelsonsar commented 9 years ago

Hmm, as I said I don't know much about the library itself. But, I thought something like:

interface Reportable
{
    public function reportError($input, array $relatedException = array());
}

And then

interface Validatable extends Reportable
{
    public function assert($input);
    public function check($input);
    public function validate($input);
}

Accessors (setName, getName, setTemplate) can be implemented in AbstractRule directly and overriden in child class. Later, you can change Rules to receive a name in constructor and the same thing for message.

Later you can also delegate error reporting to another class or trait (last we'll be easier) and isolate this part/responsibility. Then, remove reportError from Validatable.

All behavior that need to be extended can be extracted to another "helpers".

Some thoughts from someone that don't know much but like the lib a lot :)

henriquemoody commented 9 years ago

I really appreciate your comments, Nelson, actually I'm very happy to see you guys around!

I'm more concerned about avoiding these 3 methods (assert(), check() and check()) in the Validatable interface than fixing the interface segregation problem.

See these methods in these rules:

They have different implementations because they need different implementations, but the validation itself follows the same principle in all methods, which we can solve on the RuleInterface::apply(Result $result).

If you take a look on the AbstractRule you will the the only method that wasn't implemented from the Validatable interface is the validate() method. As I said already, we could intercept the validation on AbstractRule with a Template Method to fix the optional input problem, something like:

// ...
    abstract protected function primitiveValidate($input);

    public function validate($input)
    {
        if ($this->isOptional($input) && !$this->isRuleRequired()) {
            return true;
        }

        return $this->primitiveValidate($input);
    }

Yet we didn't fix all our problems.

augustohp commented 9 years ago

I'm more concerned about avoiding these 3 methods (assert(), check() and check()) in the Validatable interface than fixing the interface segregation problem.

I think you are talking about the same thing.

They have different implementations because they need different implementations, but the validation itself follows the same principle in all methods, which we can solve on the RuleInterface::apply(Result $result).

That is actually a solution segregating two different interfaces. I am of the opinion (I do not have strong tech arguments against it) that this proposed API is far too similar to Symfony\Validation API for us to ignore and not use it.

I am a :+1: for the effort and problems we are trying to solve but thinking again about it, maybe a good objective to have in mind is to keep the easy to use stand alone validation class which we have today 0 which implies on the easy to create validation rules. It is very easy to $email = new Respect\Validation\Rules\Email; $email->assert($something); we still don't need to provide all 3 validation methods on all rules, just one should do the trick.

henriquemoody commented 9 years ago

I am of the opinion (I do not have strong tech arguments against it) that this proposed API is far too similar to Symfony\Validation API for us to ignore and not use it.

I'm not sure either but I think it isn't. However I don't see how could it be a problem since our focus is on the Fluent API and my proposal aims to fix internal problems.

I am a :+1: for the effort and problems we are trying to solve but thinking again about it

That's sounds good to me, I'm open for discussions. This API was my idea of how to fix them and also get some improvements.

maybe a good objective to have in mind is to keep the easy to use stand alone validation class which we have today

If is that your guys which, let's keep it, despite I don't think that's the majority of use-cases, as I already said (see the date of #294 and when @alganet's article was add to our official documentation).

which implies on the easy to create validation rules.

TBH, I don't think creating rules using my proposal is more difficult than the current design we have. Actually, thinking about the implementation of a Rule it's much easier (talking about Validatable vs RuleInterface).

It is very easy to $email = new Respect\Validation\Rules\Email; $email->assert($something); we still don't need to provide all 3 validation methods on all rules, just one should do the trick

As we have today, not all rules need to provide all 3 validation methods, but just a few of them when extending the AbstractRule class and creating validate() method? I was trying to remove this abstract class; you may say we can use traits, but that's almost the same (like cheating :stuck_out_tongue_closed_eyes:), we're depending on implementations not on interfaces and that bothers me.

Today I was trying to fix the optional input issue on branch 1.0 using the current Concrete API we have and then I realize I need to change all rules and many unit tests because some methods in some rules consider optional values on its validation - I don't think it should be the rule's responsibility.

TL; DR Any way, I understand the feeling you guys have about standalone rules, despite I see no problem to live without them. I've talked to @alganet once about this subject and I felt the same feeling on him.

You guys already know mine, so, what more suggestions do you have?

alganet commented 9 years ago

What about two interfaces?

interface RuleInputInterface
{
    // Apply rule to a mixed®
    public function validate($input);
}

interface RuleInterface extends RuleInputInterface
{
    // Apply rule to a Respect\Validation® Result®
    public function apply(Result $result);
}

Irrelevant PS:

That also covers one edge case when self-testing a Result object (but not using it in the engine, > using it as an object target).

Downside: two methods to implement. Good thing that in most cases one can reuse another.

henriquemoody commented 9 years ago

@alganet, just a couple questions came to my mind:

Does RuleInputInterface::validate(mixed $input) have to deal with optional inputs?
Are we going to have AbstractRule to implement RuleInterface::apply(Result $result)?

PS.: @augustohp's example uses the assert() method, I'm not sure if he was talking about all the 3 validation methods when he said standalone validator or wasn't.

augustohp commented 9 years ago

I'm not sure either but I think it isn't. However I don't see how could it be a problem since our focus is on the Fluent API and my proposal aims to fix internal problems.

I would not pose it as a problem, as I said before I fell the other way around towards your actual API. But if you extract the fluent interface and apply it to another validation engine (it can be Symfony, Hamcrest, etc) we have the same effect without having to actually implement a lot of the rules.

I am lazy, so.. call me Mr. Maybe. :stuck_out_tongue_winking_eye:

TBH, I don't think creating rules using my proposal is more difficult than the current design we have. Actually, thinking about the implementation of a Rule it's much easier (talking about Validatable vs RuleInterface).

By saying "more difficult" I don't mean impossible neither a trouble to implement. As I said (and had being forced to use in the latest developments), Symfony has the very same approach of accumulating validation results inside a context.

This eases (a LOT) the problem of catching and gathering exceptions and localizing them (along with the vertical class tree we have today), but you can't compare the creation of a method that returns a boolean to a method requiring a method call on a peer and say the have the same complexity.

In my current knowledge of the case I am not even willing to discuss the impact of returning a boolean vs calling a peer method have on the overall design, since calling a method will get rid of an required generalization which is always welcome. I would rather like to discuss if we could find ways like proposed by @alganet of having the best of both worlds (since we are into it).

As we have today, not all rules need to provide all 3 validation methods, but just a few of them when extending the AbstractRule class and creating validate() method? I was trying to remove this abstract class; you may say we can use traits, but that's almost the same (like cheating :stuck_out_tongue_closed_eyes:), we're depending on implementations not on interfaces and that bothers me.

I completely agree with you, I would not call it cheating since it is one of the only cases where I see a horizontal inheritance case but I can't argue that we would exchange a design for another very similar.

When I said about the assert implementation I was thinking about implementing methods that throw exceptions instead of only returning booleans. If every simple, stand alone, rule would implement a single-method interface where they throw exceptions, we can provide ways to intercept them and still have our fluent API without the need of an AbstractRule.

Rughly, somthing like this:

class Int implements RuleInterface
{
    public function assert($input)
    {
        if (false === is_numeric($input)) {
            throw new \Exception('{{input}} is not a number.');
        }

        $integer = (int) $input;
        if ($integer != $input) {
            throw new \Exception('{{input}} is not an integer.');
        }

        return true;
    }
}

I haven't nurtured the idea inside my head enough, but my first impression is that we still will have all the benefits you want to achieve, allowing the usage of a Rule class completely outside the library. Which will undoubtedly improve integration.

This still wields the benefit of explanation of the rules, very similar to the benefit you would have using the Result object. Making complex rules a lot more clearer.

henriquemoody commented 9 years ago

Now I see...

We need to create a POC but I'm almost sure that's a way to go with no need of the Result (which in fact is better named as Context).

With your proposal we can intercept these rules on the methods validate(), __invoke() and check() on the Validator class in the same way I'm doing on my API. Besides the exceptions' behavior would be more clear because there will be no need of create too generic ones.

I just strongly disagree with the idea of throwing an exception or returning a value (which is only true), I think is better we just throw an exception or not.

I'm :+1: for @augustohp's suggestion.

How about you, @alganet and @nelsonsar?

We will have to change all rules, any way, but that's for a good cause.

henriquemoody commented 9 years ago

The only problem on this suggestion is that I'm not sure about how to deal with optional input in this scenario, any thoughts, @augustohp?

nelsonsar commented 9 years ago

I don't think that it's a problem not depending on interfaces when you are rewriting something or even refactoring it, the approach of trying to model something from another thing can (and as Murphy's Law says will) make things harder, but, this is your call - I'm here just trying to not break the API :P

I like the idea of throwing an exception and :+1: with @henriquemoody about return a boolean value when not throwing an exception. Can you guys give a case of optional validation?

augustohp commented 9 years ago

We need to create a POC but I'm almost sure that's a way to go with no need of the Result (which in fact is better named as Context).

No doubts about it.

The only problem on this suggestion is that I'm not sure about how to deal with optional input in this scenario, any thoughts, @augustohp?

Me neither, but I think it is imperative that every rule becomes required since we will always want the exceptions. Maybe we can create an OptionalInput rule which filter exceptions.

Can you guys give a case of optional validation?

Currently, every input on a form is treated as optional so for every chain of rules you have to explicitly use notEmpty.

henriquemoody commented 9 years ago

@nelsonsar, every rule should accept optional values by default (BTW I need all you guys thoughts about some comment of mine), so:

v::int()->validate(''); // Must return true
v::int()->positive()->validate(null); // Must return true
v::key('email', v::email())->validate(['email' => null]); // Must return true

Edit:

v::notOptional()->int()->positive()->validate(null); // Must return false

augustohp commented 9 years ago

@nelsonsar, every rule should accept optional values by default

So your example would become:

v::optional()->int()->validate(''); // Must return true
v::optional()->key('email', v::email())->validate(['email' => null]); // Must return true

nelsonsar commented 9 years ago

Yeah, I use When to have this behavior :P

When(NotEmpty, YourValidatorHere, AlwaysValid)

But @augustohp's idea is great!

henriquemoody commented 9 years ago

It's not really a problem but optional() rule must always be the first in the chain, I guess.

augustohp commented 9 years ago

It's not really a problem but optional() rule must always be the first in the chain, I guess.

I think so, at least in front of the rules considered optional. As I thought, it would act as an AllOf rule.

henriquemoody commented 9 years ago

I think would be easier to have, this:

v::optional(v::int()->positive());

Instead of:

v::optional()->int()->positive();

What would you think?

augustohp commented 9 years ago

I think would be easier to have, this:

v::optional(v::int()->positive());

Instead of:

v::optional()->int()->positive();

What would you think?

The first way is much more clear.

henriquemoody commented 9 years ago

I've started to write a POC for @augustohp's suggestion but I will not have time to keep it in the next days, so, if someone whats to keep working on that, the branch is new_api_2.

henriquemoody commented 9 years ago

Take a look in the branch new_api_2, it's fully operational.

henriquemoody commented 9 years ago

The problems I've seen in new_api_2 branch are:

We cannot inject the Factory on rules (see Not)
We cannot inject dependencies on exceptions, only the rule itself
Because we're creating exceptions in the rules (see Match), we cannot inject default properties (like translator, filter, label, etc) to the exceptions so we have to do it statically or with some workaround.
We cannot use rules as optional, unless we use optional() rule that was proposed
It's harder to handle nested exceptions than Proposal 1
We cannot track all assertions. Like in the not() rule, if the proxied rule is a composite, we need to assert() the entire chain again to see what rule pass and what true doesn't pass and reproduce the composite behaviour (allOf, noneOf, oneOf, etc), think on v::not(v::not(v::oneOf(v::int(), v::float())))

I'm not 100% comfortable because of these problems, but it seems fair since you guys want the standalone rules.

About the complexity of the rules, some got better and some others got worst, so it's even with new_api branch -- which I still prefer.

How about you guys, any thoughts?

Edit:

As a solution for the static Factory I created FactoryAware interface, see 09353e8.

I've performed some benchmarks, running this code:

$time = microtime(true);
for ($i=0; $i < 100000; $i++) {
    v::match('/^[0-9]+$/')->validate((string) $i); // regex() when in current master
}
$time = microtime(true) - $time;

echo $time.PHP_EOL;

The results were:

branch	time
master	8.1487798690796
new_api_2	8.2870280742645
new_api	17.462069988251

henriquemoody commented 9 years ago

I'm not sure if you guys took a look in the branch's code, but, any way, I was thinking about changing the default interface:

interface Assertable 
{
    public function assert($input, array $context = array());
}

We will still be able to use a rule as stand alone with assert($input), in the other hand we can define properties from the Factory to the exceptions, like the label and translator/filter.

What you guys think?

Also we can take advantage of that, like:

v::int()->assert($input, ['message' => 'It is not an integer']);

The ValidationException::__construct will probably be:

public function __construct($input, array $context, ValidationExceptionCollection $children = null)

UPDATE

This is not viable, because it leads with some code duplication with proxy rules.

henriquemoody commented 9 years ago

It's been quite of time since you guys gave me some feedback on that, I think I can assume I can go on with the interface on my last comment.

henriquemoody commented 9 years ago

@alganet, @augustohp and @nelsonsar, have you guys seem my last commend. I must say I have strong feelings on keeping Proposal 1, yet. I need some feedback.

henriquemoody commented 8 years ago

Hi guys, after 9 months I have another idea for our Concrete API thanks to all this conversation and some chats with @lcobucci and @batusa.

I'm going to summarize the problems we have:

We have too many methods on the Validatable interface;
We have duplicated code among the rules because it's hard to reuse rules properly without inheritance;
We have problems with some nested validations because our lack of granularity control;
We need to create 2 classes for every rule (the rule itself and the exception class);
We cannot access the Factory inside the rules;
We want to keep standalone rules.

Also I have to mention that the inputs are not optional anymore since the release of 1.0 version - which is a really good thing.

After thinking about that I came to this RuleInterface:

interface RuleInterface
{
    /**
     * Validates the rule against the given input.
     *
     * @param mixed $input
     *
     * @return ResultInterface
     */
    public function validate($input): ResultInterface;
}

Every validate() method will receive an $input, as today, but they will return a ResultInterface instead of a boolean value. So if you want to use a rule without the Validator class:

$rule = new Email();
$result = $rule->validate('example@example.com');
if ($result->isValid()) {
    // Do something
}

With the Validator class we can do something like this:

if (v::email()->isValid('example@example.com')) {
    // Do something
}

The ResultInterface will provide us the granularity control we need to create complex messages for rules like allOf(), key(), domain() and not() - but now with a real nested support.

From the result we need 4 things:

Tell us if it's valid or not (true or false);
Access to the input to use on messages;
Access to the rule, then we known what type of exception we should thrown;
Access to custom properties to be used on the messages.

About the Factory, which is necessary to create rules inside rules in order to reuse them, I thought about creating an empty FactoryAwareInterface and on Facetory::createRule() we pass the Factory instance as the last constructor's argument of these rules.

There is also another great benefit about this API, see:

final class Image implements RuleInterface
{
    private $factory;

    public function __construct(Factory $factory)
    {
        $this->factory = $factory;
    }

    public function validate($input): ResultInterface
    {
        $fileRule = $this->factory->createRule('file');
        $fileResult = $fileRule->validate($input);
        if (!$fileResult->isValid()) {
            return $fileResult;
        }

        // Check if the file is an image..
    }
}

We not only reused the file rule but its result as well. So when using check() or assert() method we may see the message "filename" must be a valid file because it's the result of the File rule. Of course, there will be some times that it will not be a good idea, but it's an awesome possibility, IMO.

We also can have TemplateInterface to define the templates in the same class of the rules and with that we can even consider if we want to have one exception for each rule:

interface TemplateInterface
{  
      const MODE_AFFIRMATIVE = 1;
      const MODE_NEGATIVE = 0;

      const TEMPLATE_STANDARD = 0;

      /**
       * Returns the available template messages for the rule.
       *
       * @return array
       */
      public function getTemplates(): array;
}

Here a rule as example:

final class NotEmpty implements TemplateInterface, RuleInterface
{
    /**
     * {@inheritdoc}
     */
    public function getTemplates(): array
    {
        return [
            self::MODE_AFFIRMATIVE => [
                self::TEMPLATE_STANDARD => '{{placeholder}} must not be empty',
            ],
            self::MODE_NEGATIVE => [
                self::TEMPLATE_STANDARD => '{{placeholder}} must be empty',
            ],
        ];
    }

    /**
     * {@inheritdoc}
     */
    public function validate($input): ResultInterface
    {
        return new Result(!empty($input), $this, $input);
    }
}

I'm looking forward for your feedback on this one too.

The POC is ready on new_api_3 but there is a lot to improve.

FAQ

Why don't we just have an isValid(mixed $input): bool method on RuleInterface?

Because on rules like AllOf we're going to get only one return and:

We won't known what rules did pass or not;
We will have to keep the public properties on rules because some messages need more data than just the input

So, why don't we just have an isValid(mixed $input): bool method on RuleInterface and use the Mediator pattern to perform the validation and then known which rule fails or succeed?

It's impossible to create such API without the Composite pattern and using the Mediator inside rules like AllOf, OneOf, NoneOf, Key, etc, will just cause headache.

Why don't we just thrown an exception when the rule fails, having RuleInterface only an assert(mixed $input): void method?

Because we need the result, no matter if it's a failure or success. Rules like not() depend on that. Actually the not() doesn't work properly on our current API, see not_with_recursion.phpt.

Few days after this comment I saw Nelson's tweet which made me think I'm in the right track. Maybe you guys would like to read Martin Fowler's the article.

henriquemoody commented 4 years ago

This is quite old, and I decided to old issues like this. If we ever think about something like that, let's reopen or create another issue for this.

Thank you very much for all your comments! 🐼

Respect / Validation

New Concrete API #354

Problems

All rules depend on `AbstractRule`

`Validatable` interface has too many methods

Decentralized validation

NotEmpty rule

Nested exceptions

Nested exception messages

Exception message translation

Cannot access `Factory` inside rules

Proposal

Why it's called `apply()`?

What is the `Context` object?

Why we don't have a `$input` parameter?

I cannot use a Rule as a single validation anymore?

How about `check()`, `assert()`, `validate()` and `__invoke()` methods?

How do you deal with optional values?

What else?

Show me the code!

FAQ

Respect / Validation

New Concrete API #354

Problems

All rules depend on AbstractRule

Validatable interface has too many methods

Decentralized validation

NotEmpty rule

Nested exceptions

Nested exception messages

Exception message translation

Cannot access Factory inside rules

Proposal

Why it's called apply()?

What is the Context object?

Why we don't have a $input parameter?

I cannot use a Rule as a single validation anymore?

How about check(), assert(), validate() and __invoke() methods?

How do you deal with optional values?

What else?

Show me the code!

FAQ

All rules depend on `AbstractRule`

`Validatable` interface has too many methods

Cannot access `Factory` inside rules

Why it's called `apply()`?

What is the `Context` object?

Why we don't have a `$input` parameter?

How about `check()`, `assert()`, `validate()` and `__invoke()` methods?