Skip to content

Add an addRule #13

Closed
harikt opened this Issue Mar 14, 2013 · 16 comments

2 participants

@harikt
Aura for PHP member
harikt commented Mar 14, 2013

I would like to add an addRule, so that we can add multiple error messages.

This is from the discussion in irc http://colabti.org/irclogger/irclogger_log/auraphp?date=2013-03-13 . For a quick reference I am keeping.

17:04   harikt  1 qn , currently the filter can only hold one filter for the field, why not add it for multiple ones?
17:04   harikt  I mean for the Aura.Input Filter
17:04   harikt  not Aura.Filter
17:14   pmjones     oh, so, on Aura.Input filter, it's intended as a placeholder for a "real" filter system
17:14   pmjones     it's the simplest thing that can possibly work as a filter system, but it's clearly not very robust
17:15   pmjones     and doing it this way means anyone can add any filter system, and add on the very basic Aura.Input setRule() method
17:16   pmjones     hope that helps explain
17:29   harikt  not sure whether you understood me clearly .
17:30   harikt  pmjones, if you look into this https://github.com/harikt/Aura.Input/blob/6456c130e9c7007badadfd8630102c29101a9419/src/Aura/Input/Filter.php
17:30   harikt  You can see the rules field I added as array .
17:31   harikt  I agree it is just a place holder, but it can act more powerful like adding more filters than a single rule
17:31   harikt  Try adding setRule twice on a field.
17:32   harikt  only the last one will be taken , this way we can add multiple rules
18:20   pmjones     harikt: yes, that's correct, only the last one gets used.
18:20   pmjones     it's "set" rule, not "add" rule
18:21   pmjones     and because the rule is a closure, it means you can add any logic you want, meaning you can add multiple checks inside the closure
18:21   pmjones     thus, there's no need for multiple rules when using setRule()
18:21   pmjones     you add whatever rules you want inside the closure

Yes I noticed the set and add, so I guess we need a addRule for the setRule can only set one error message. But it will be good if we have multiple error messages.

I also agree with your point of multiple rules inside the closure, but error message is same makes me feel not a good idea.

Say you need to check the max string length, and whether it is string. So the user should know from the error what the error is. Not just it is not a string or the entered value is incorrect. But a clear understanding of what made wrong.

I also agree this is a simple filter and users can use other filtering / validation system. But I prefer to add it in this simple filter. It is not that much ;-)

@pmjones
Aura for PHP member
pmjones commented Mar 14, 2013

Aura\Input\Filter is not intended as a final solution for filtering. It is the barest, most basic possible implementation of a minimal interface that allows filtering. Every added method means one more method that implementors need to add. As such, I'm not inclined to create an "addRule()" method.

@harikt
Aura for PHP member
harikt commented Mar 14, 2013

if something exists it should be good, else none is better :) . So I strongly feel to add :-) . What are the limitations you feel not adding it other than final solution for filtering ?

@pmjones
Aura for PHP member
pmjones commented Mar 14, 2013

The primary reason is that anyone who wants to implement it will need to add the additional method. For example, Aura.Filter (which had addSoftRule(), addHardRule(), and addStopRule() to begin with) added setRule() so that it could implement Aura\Input\Filter. There's no point in creating addRule() as well, when a closure can encapsulate as much logic as one wants.

@harikt
Aura for PHP member
@pmjones
Aura for PHP member
pmjones commented Mar 14, 2013

Maybe the thing to do is replace "setRule()" with "addRule()", leave the signature the same, and thereby allow for multiple rules. I don't really like it, but it does address your concern about separate messages.

@harikt
Aura for PHP member
harikt commented Mar 14, 2013

You can keep the setRule there itself. I have no concern about setRule , but adding an addRule will change the implementation of the value method

<?php
    public function addRule($field, $message, \Closure $closure)
    {
        $this->rules[$field][] = [$message, $closure];
    }

    /**
     *
     * Filter and Validate the data
     *
     * @param mixed $values The value
     *
     * @return bool
     *
     */
    public function values(&$values)
    {
        // reset the messages
        $this->messages = [];

        // go through each of the rules
        foreach ($this->rules as $field => $rules) {
            // A field can have more rules
            foreach ($rules as $rule) {
                // get the closure and message
                list($message, $closure) = $rule;

                // apply the closure to the data and get back the result
                $passed = $closure($values[$field]);

                // if the rule did not pass, retain a message for the field.
                // note that it is in an array, so that other implementations
                // can allow for multiple messages.
                if (! $passed) {
                    $this->messages[$field][] = $message;
                }
            }
        }

        // if there are messages, one or more values failed
        return $this->messages ? false : true;
    }
@pmjones
Aura for PHP member
pmjones commented Mar 14, 2013

If there's an addRule() then there's no need for a setRule(); you can use addRule() once and get the same effect. As long as there's only one method to implement relating to rules.

@pmjones
Aura for PHP member
pmjones commented Mar 14, 2013

Yeah, the more I think about this, the more I think replacing setRule() with addRule(), and multiple rules, is the way to go. Thanks for bringing this up. :-)

@harikt
Aura for PHP member
@pmjones
Aura for PHP member
pmjones commented Mar 14, 2013

"Bribe" means you gave me money (or other compensation) to change my mind. What you did was "nag" me; you will have a wife soon and learn what that means by experience. ;-)

@harikt
Aura for PHP member
@pmjones
Aura for PHP member
pmjones commented Mar 14, 2013

Not at all, sir; it was a joke. You help keep me on the right path. :-)

@harikt
Aura for PHP member
@pmjones
Aura for PHP member
pmjones commented Mar 16, 2013

I tried this out, and it just doesn't "feel" right when combined with Aura.Filter. The problem is this:

  • setRule() sets one rule for the field, and sets one message for the field.

  • addRule() adds a rule for the field (possibly one of many), and sets on message for the field, but if another rule gets added using addRule(), its message overrides the message from the previous addRule().

Of the two, I think using setRule() as it is causes less confusion and integrates better across the board.

Thanks for suggesting it, regardless.

@pmjones pmjones closed this Mar 16, 2013
@harikt
Aura for PHP member
harikt commented Mar 16, 2013

then you need to revert this commit I guess. The merge of issue #11 already has it . See https://github.com/auraphp/Aura.Input/pull/11/files#L1R38

@harikt
Aura for PHP member
harikt commented Mar 16, 2013

By the way I have not tried it to reply back, nor I don't think I will get some time to check. So if I see at some point I will reply :+1:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.